From 11d69b603ca4008e70a93532958f65e440c936e0 Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Sat, 18 Jul 2026 14:00:30 +0100 Subject: [PATCH] Box the generator wrapper as a heap-allocated ObjectType instead of a value tuple Object literals in this compiler compile to value-typed tuples by default, even though the generator wrapper ({step, next()}) has mutable identity that must be shared across aliases. This meant passing a generator to a function, capturing it in a closure, or reassigning it (const b = a) all silently copied its state instead of aliasing it. Add InternalFlags::BoxAsObject to mark the synthetic wrapper literal built by buildGeneratorWrapperDeclaration, and heap-box it (NewOp + StoreOp + CastOp, the same recipe castTupleToInterface already uses) into ObjectType instead of leaving it as a tuple. Property access on ObjectType already emits a PropertyRefOp directly on the pointer, so .next() now mutates shared storage for every alias. Supersedes the const-storage-only fix in #244 for generators specifically, and fixes the parameter-aliasing bug that fix explicitly left open. Co-Authored-By: Claude Sonnet 5 --- .../docs/generator-object-wrapper-design.md | 110 ++++++++++++ tslang/docs/generator-param-by-ref-design.md | 159 ++++++++++++++++++ tslang/lib/TypeScript/MLIRGenExpressions.cpp | 35 +++- tslang/lib/TypeScript/MLIRGenFunctions.cpp | 4 + .../tester/tests/00generator_manual_next2.ts | 41 +++-- tslang/ts-new-parser/enums.h | 6 +- 6 files changed, 340 insertions(+), 15 deletions(-) create mode 100644 tslang/docs/generator-object-wrapper-design.md create mode 100644 tslang/docs/generator-param-by-ref-design.md diff --git a/tslang/docs/generator-object-wrapper-design.md b/tslang/docs/generator-object-wrapper-design.md new file mode 100644 index 000000000..17af7ec45 --- /dev/null +++ b/tslang/docs/generator-object-wrapper-design.md @@ -0,0 +1,110 @@ +# Generator wrapper as a reference type (`ObjectType`): design + +Status: **implemented and verified** — 350/350 JIT + 354/354 compile on the +first full-suite round, plus new aliasing regression coverage (parameter, +closure capture, plain assignment) added to +`test/tester/tests/00generator_manual_next2.ts`. The chronic "losing this +reference" warning on generator tests is gone, confirming the value/`this` +representation mismatch theory (§3). Implementation matched the plan in §4 +exactly; step 4 (for...of) required no changes — iteration discovers `next` +via `evaluateProperty`, which flows through the generic property-access +machinery that already handles `ObjectType`. +Supersedes the rejected `docs/generator-param-by-ref-design.md` (RefType +parameters). Proposed by the user; verified feasible by code inspection. + +## 1. The idea + +Stop representing the generator wrapper (`{ step, next() {...} }`, built by +`buildGeneratorWrapperDeclaration`, `MLIRGenFunctions.cpp:531-678`) as a +value-typed tuple. Make it a **reference type** — `mlir_ts::ObjectType`, a +pointer to heap storage — the same representation class instances already +have. Anything with identity in JavaScript is a reference type; the wrapper +has mutable identity (`step` advanced by `next()`), so it should be one too. + +## 2. Why this beats the RefType-parameter approach + +| Concern | RefType params (rejected) | ObjectType wrapper (this) | +| --- | --- | --- | +| Parameter aliasing | fixed only at fn boundaries, via ABI change | fixed — callee copies the *pointer* | +| `const g = gen(); g.next()` | needs PR #244's storage fix | works even for a bare SSA const (pointer has identity) | +| Closure capture | needs by-ref capture flag | pointer copied by value is already correct | +| Function-type equality (`TestFunctionTypesMatch`, `canMatch`, generics, `ReturnType<>`) | must treat `T`/`RefType` as equal at every comparison site — scattered, unbounded checklist | untouched — the wrapper is *one* type everywhere | +| Cost | none | one GC heap alloc per generator instantiation (semantically correct; it's what JS engines do) | + +## 3. Verified enabling facts (file:line) + +- **Boxing recipe already exists**: `castTupleToInterface` + (`MLIRGenCast.cpp:1574-1579`) boxes a tuple with exactly + `NewOp(ValueRefType)` + `StoreOp` + `CastOp` → `ObjectType`. `NewOp` + heap allocation is GC-managed (same as class instances) — solves the + escapes-`gen()`-frame lifetime question. +- **Property access on `ObjectType` is already correct**: the TypeSwitch case + (`MLIRGenAccessCall.cpp:302`) → `MLIRPropertyAccessCodeLogic::Object` + → `RefLogic` (`MLIRCodeLogic.h:1623-1671`) emits `PropertyRefOp` **directly + on the pointer** — a real field address into shared storage. No fresh + alloca, no pristine-copy seeding, no `boundRefMaterializedCache`. The + entire bug family lives only in the tuple access path (`Tuple`/ + `TupleNoError`), which `ObjectType` never enters. +- **Methods already expect an `ObjectType` `this`**: object-literal codegen + builds `oli.objThis = getObjectType(objectStorageType)` + (`MLIRGenExpressions.cpp:1298-1301`) as the `this` type for the literal's + methods (the `USE_BOUND_FUNCTION_FOR_OBJECTS` mechanism; + `isBoundReference`, `MLIRTypeHelper.h:203-219`, recognizes first-param + `ObjectType` as bound). Only the produced *value* is currently a tuple + (`MLIRGenExpressions.cpp:1330-1342`) — the value/this representation + mismatch is likely also the source of the persistent "losing this + reference" warnings on every generator test. +- **Marker mechanism exists**: synthetic AST already carries codegen hints in + `internalFlags` (`VarsInObjectContext` set at `MLIRGenFunctions.cpp:598`, + `ThisArgAlias` at `:607`; enum at `ts-new-parser/enums.h:544-560`, bits + free from `1 << 11`). +- **Return-type inference needs no help**: the wrapper function has no + explicit return-type node; `discoverFunctionReturnTypeAndCapturedVars` + takes the type of the returned value. Box the literal → the wrapper's + return type *becomes* `ObjectType` automatically, consistently, everywhere + (including pass-2 method-prototype registration from PR #243, which runs + the same discovery). +- **`ObjectType` → interface casts already exist** (`castObjectToInterface`, + `MLIRGenCast.cpp:1584+`), so iterable-as-interface usage keeps working. + +## 4. Implementation plan + +1. Add `InternalFlags::BoxAsObject = 1 << 11` (`ts-new-parser/enums.h`). +2. `buildGeneratorWrapperDeclaration` (`MLIRGenFunctions.cpp:618`): set the + flag on the synthetic `generatorObject` literal — one line. +3. `mlirGen(ObjectLiteralExpression)` (`MLIRGenExpressions.cpp:1270-1343`): + when the flag is set, after building the (const-)tuple value: cast + const-tuple → mutable tuple (`convertConstTupleTypeToTupleType` — `step` + must be mutable at runtime), then box via `NewOp` + `StoreOp` + `CastOp` + to `oli.objThis` (the *named* `ObjectType` the methods' + `this` already expects — not an anonymous `ObjectType::get(tupleType)`), + and return the pointer value. +4. Check `for...of` / iterator-protocol lowering: find where `next` is + discovered on the iterated expression's type (search `ITERATOR_NEXT` + consumers); if it inspects tuple fields directly, teach it to look through + `ObjectType::getStorageType()`. Same check for `MLIRTypeHelper::getFields` + (`MLIRTypeHelper.h:2133-2220`) if anything getFields-based touches the + wrapper. +5. Build + test rounds (proven workflow): target repros first + (`00generator_manual_next.ts`, `00generator_manual_next2.ts`, + `00generator7.ts`, plus **extend** `00generator_manual_next2.ts`'s `main2` + to finally drive `it` past `drainTwo` — the case it deliberately avoided — + and a new closure-capture-mutation case), then the fragile set + (`00disposable`/`01`/`02`, `00spread`, `01symbol`), then full ctest suite. +6. If green: the `[value, done]` result tuple stays a value tuple + (deliberately — it has no mutable identity); PR #244's + `needsIdentityStorage` machinery and the `boundRefMaterializedCache` + become dead for generators — leave both in place, remove in a later + cleanup PR (same bisectability reasoning as before). + +## 5. Risks / open items + +- `for...of`/spread lowering shape (step 4) is the main unknown — not yet + read. +- Generator methods in classes (`class C { *gen(){} }`) and object literals + (`{ *gen(){} }`) share `buildGeneratorWrapperDeclaration`, so they change + uniformly — but class-method `this` interplay (`fixThisReference` path, + `MLIRGenFunctions.cpp:534-546, 604-614, 628-637`) needs a test pass. +- Compile-output tests that print the wrapper's type will change text. +- Async generators / `for await` (`ForAwait` flag) — check whether that path + builds its own wrapper or shares this one. diff --git a/tslang/docs/generator-param-by-ref-design.md b/tslang/docs/generator-param-by-ref-design.md new file mode 100644 index 000000000..b38dc2ee4 --- /dev/null +++ b/tslang/docs/generator-param-by-ref-design.md @@ -0,0 +1,159 @@ +# Generator (bound-method-typed) function parameters: pass-by-reference design + +Status: **REJECTED — superseded by `docs/generator-object-wrapper-design.md`.** +The RefType-parameter approach documented below was analyzed and dropped: the +user proposed the better alternative of making the generator wrapper itself a +reference type (`ObjectType`) instead of a value tuple, which fixes parameter +aliasing, const storage, and closure capture at the root with none of the +function-type-equality blast radius described in §3 below. This document is +kept as the record of why the RefType path was not taken. + +Branch: `generator-param-by-ref`. See `docs/const-let-storage-design.md` §3b +for the bug this addresses and the [[generator-param-value-semantics-bug]] +memory for the original repro. + +## 1. The bug, restated precisely + +`const-let-storage-rework` (merged, PR #244) gave `const` bindings whose type +has a bound-method field (currently: generator wrapper objects) real storage +at declaration time — fixing state loss for `.next()` calls made *within the +same function*. It explicitly did not fix the case where such a value is +passed as a function **parameter**: `.next()` calls made inside the callee +still don't advance the caller's binding, because the value is copied at the +call boundary, not aliased. + +## 2. Why there is no way to avoid touching the function's ABI type + +Confirmed by tracing the pipeline (agent research, not yet re-verified line +by line at implementation time — recheck before coding): + +- `mlirGenFunctionParams` (`MLIRGenFunctions.cpp:1026-1070`) wraps every + incoming block argument in `mlir_ts::ParamOp`, unconditionally built as + `RefType::get(param->getType())` seeded from `arguments[index]` + (`MLIRGenFunctions.cpp:1057-1058`). +- `ParamOpLowering` (`LowerToAffineLoops.cpp:183-193`) lowers `ParamOp` to a + **fresh** `mlir_ts::VariableOp` (a new alloca), always — it does not matter + what `arguments[index]`'s type is; a new box is minted and the incoming + value is stored into it as the initializer. +- `arguments` (the entry block's arguments) are not built independently — + they come from `FunctionOpInterface::addEntryBlock()`, which derives one + block argument per input type in the `FuncOp`'s **current `FunctionType`** + (`MLIRGenFunctions.cpp:1208`, `:1317`). Confirmed no separate/parallel + argument-list construction exists. + +Consequence: **the only way a genuine pointer can arrive in the callee's +block argument is for the function's declared `FunctionType` to say the +parameter's type is `RefType`, not `T`.** There is no codegen-only trick +(no "pass the ref through some side channel") that bypasses this — the ABI is +fixed by the type the `FuncOp` was built with. This is different from the +const-storage fix, which only had to change a *local declaration's* storage +decision and never touched a cross-function-boundary type. + +## 3. The blast radius this creates + +Once a parameter's registered type becomes `RefType` instead of `T` +(for `T` = a `TupleType`/`ConstTupleType` with a bound-method field, per the +existing `MLIRTypeHelper::hasBoundMethodField` predicate from the merged +const-storage fix), every place that compares function *types* structurally +for equality now sees a mismatch against "the same" function's other +appearances (e.g. a `let` variable of function type, a generic instantiation, +a `ReturnType` query, an assignability check at a call site +against a differently-sourced signature of the same shape). Found so far: + +- `MLIRTypeHelper::TestFunctionTypesMatch` (`MLIRTypeHelper.h:905-934`): raw + `inInputs[i] != resInputs[i]` per-parameter equality (line 922). Callers at + `MLIRTypeHelper.h:1136, 1368, 1411, 1474, 1723` — assignability/overload/ + generic-instantiation matching all funnel through this one function, so a + fix localized here (e.g. treat `T` and `RefType` as equal specifically + when `hasBoundMethodField(T)`) would cover all of these callers uniformly. +- A **second**, separate comparator exists nearby (`MLIRTypeHelper.h` around + line 1209-1216) using a recursive `canMatch(location, ...)` instead of raw + `!=` for a different function-type-matching path (different call sites, + different `startParam`/opaque-`this`-skipping logic). Not yet confirmed + whether `canMatch` already tolerates a `RefType` wrapper or would also need + a fix. **This must be checked before implementation** — if `canMatch` + already unwraps refs generically (plausible, since it's used for broader + structural compatibility, e.g. object/unknown per its inline comment), + this path may already be fine; if not, it needs the same treatment as + `TestFunctionTypesMatch`. +- Not yet audited: generic type-parameter inference/matching (`MLIRGenGenerics.cpp`, + `tryInferTupleFields`-family in `MLIRTypeHelper.h`), and whatever backs + `ReturnType`-style type queries — these may do their own + independent structural comparison rather than funneling through either + matcher above. Must be enumerated before implementation, not discovered + reactively via ctest failures. + +This is a materially bigger blast radius than the const-storage fix, which +touched exactly 3 files and ~70 lines. The precedent that this *kind* of +special-casing is tractable exists — `OptionalType` is special-cased at +similar density throughout this file (e.g. `MLIRGenImpl.h:1690`) — but the +touch points here are scattered across a type-matching subsystem that has no +single choke point guaranteeing full coverage the way `registerVariable` was +a single choke point for the storage decision. + +## 4. Proposed approach (once implementation starts) + +1. **Enumerate every structural function-type comparison site first**, + exhaustively, before writing the parameter-type change — grep for + `FunctionType` type-equality comparisons (`!=`, `==`) and every + `canMatch`/`TestFunctionTypesMatch` caller, not just the ones surfaced by + one research pass. Build a checklist; don't rely on ctest to find the + rest, since the original const-storage fix already took 3 rounds of full + suite runs to surface all issues on a *much smaller* change. +2. Add a single normalization helper, e.g. + `MLIRTypeHelper::stripIdentityRef(mlir::Type t)` — returns `t`'s element + type if `t` is `RefType` and `hasBoundMethodField(U)`, else `t` + unchanged. Use it to normalize both sides immediately before every + structural comparison found in step 1, rather than special-casing each + comparator's internals differently. +3. Change parameter type registration (`mlirGenFunctionSignaturePrototype`, + `MLIRGenImpl.h:1660-1701`, feeding `getFunctionType` via + `mlirGenFunctionPrototype`, `MLIRGenFunctions.cpp:249-330`) to wrap a + bound-method-bearing parameter's type in `RefType` when building `argTypes` + for the `FuncOp`'s `FunctionType` — mirroring how optional params are + special-cased at `MLIRGenImpl.h:1690-1693`. +4. Change `mlirGenFunctionParams` (`MLIRGenFunctions.cpp:1026-1070`): when + `param->getType()` is already the caller-visible `RefType` (i.e. this + parameter took the new path), do NOT wrap it in another `ParamOp`→fresh + `VariableOp`; bind the variable directly to the incoming block argument + (which is already the caller's storage pointer) instead of allocating and + copying. +5. Change call-site operand building + (`mlirGenAdjustOperandTypes`, `MLIRGenImpl.h:6394-6455`, specifically the + `value.getType() != argTypeDestFuncType` branch around line 6445): when the + destination type is `RefType` for a `hasBoundMethodField` `T`, obtain + the operand's *reference* instead of loading — `MLIRCodeLogic::GetReferenceFromValue` + (`MLIRCodeLogic.h:122-155`) already unwraps a `LoadOp` back to its + `.getReference()`, and `resolveIdentifierAsVariable` + (`MLIRGenVariables.cpp:919`) already emits that `LoadOp`, so the ref is + recoverable at this point without restructuring identifier resolution. +6. Test incrementally, per the const-storage fix's proven workflow: target + repro first (`00generator_manual_next2.ts`'s `drainTwo` case, extended to + continue driving `it` after the call — this is exactly the case that file + currently deliberately avoids exercising), then the same previously-fragile + set (`00disposable`/`01disposable`/`02disposable`, `00spread`, `01symbol`, + any test exercising function-type assignability or generics with + function-typed values), then full suite. Expect multiple rounds. + +## 5. Open questions to resolve before coding starts + +- Does `canMatch` (§3, second comparator) already tolerate `RefType` + wrappers generically? If yes, step 1's checklist shrinks by one entry. +- Are there other constructs beyond generators that already have + `hasBoundMethodField(T) == true` today, or could soon (e.g. an object + literal with a bound method, not just the generator wrapper)? If so, this + fix benefits them for free, but the checklist in step 1 must consider + those call shapes too, not just `function* gen(){}`. +- Is there a case where a bound-method-bearing value is passed *by value on + purpose* (e.g. intentionally snapshotting a generator's current state into + a helper that must not mutate the caller's copy)? TypeScript itself has no + such distinction (objects are always reference semantics at the language + level) — but confirm no existing test relies on the current (arguably + accidental) copy-by-value behavior as if it were a feature. + +## 6. Non-goals (unchanged from the merged fix) + +- No change to `const`/`let` reassignment semantics. +- No storage changes for classes/arrays/plain objects — still unaffected, + still already pointer-like. +- No `DominanceInfo` introduction. diff --git a/tslang/lib/TypeScript/MLIRGenExpressions.cpp b/tslang/lib/TypeScript/MLIRGenExpressions.cpp index 6b96e82ed..8c464bd11 100644 --- a/tslang/lib/TypeScript/MLIRGenExpressions.cpp +++ b/tslang/lib/TypeScript/MLIRGenExpressions.cpp @@ -1332,14 +1332,43 @@ namespace mlirgen auto arrayAttr = mlir::ArrayAttr::get(builder.getContext(), oli.values); auto constantVal = builder.create(location, constTupleTypeWithReplacedThis, arrayAttr); - if (oli.fieldsToSet.empty()) + + auto boxAsObject = + (objectLiteral->internalFlags & InternalFlags::BoxAsObject) == InternalFlags::BoxAsObject; + + if (oli.fieldsToSet.empty() && !boxAsObject) { return V(constantVal); } auto tupleType = mth.convertConstTupleTypeToTupleType(constantVal.getType()); - auto tupleValue = mlirGenCreateTuple(location, tupleType, constantVal, oli.fieldsToSet, genContext); - return V(tupleValue); + + mlir::Value tupleValue; + if (!oli.fieldsToSet.empty()) + { + tupleValue = mlirGenCreateTuple(location, tupleType, constantVal, oli.fieldsToSet, genContext); + } + else + { + CAST_A(castedValue, location, tupleType, constantVal, genContext); + tupleValue = castedValue; + } + + if (!boxAsObject) + { + return V(tupleValue); + } + + // this literal has mutable identity (e.g. a generator wrapper whose `step` + // is advanced by next()) -- box it on the GC heap and hand out a + // reference-typed ObjectType so every alias (const binding, parameter, + // closure capture) shares the same state; same recipe as castTupleToInterface + auto objType = mlir_ts::ObjectType::get(tupleType); + auto valueAddr = + builder.create(location, mlir_ts::ValueRefType::get(tupleType), builder.getBoolAttr(false)); + builder.create(location, tupleValue, valueAddr); + auto objValue = builder.create(location, objType, valueAddr); + return V(objValue); } ValueOrLogicalResult MLIRGenImpl::mlirGen(Identifier identifier, const GenContext &genContext) diff --git a/tslang/lib/TypeScript/MLIRGenFunctions.cpp b/tslang/lib/TypeScript/MLIRGenFunctions.cpp index d070eccd8..116b770bb 100644 --- a/tslang/lib/TypeScript/MLIRGenFunctions.cpp +++ b/tslang/lib/TypeScript/MLIRGenFunctions.cpp @@ -616,6 +616,10 @@ namespace mlirgen generatorObjectProperties.push_back(nextMethodDecl); auto generatorObject = nf.createObjectLiteralExpression(generatorObjectProperties, false); + // the generator object has mutable identity (`step` advanced by next()); + // it must be a reference type so aliases (params, closures, const bindings) + // share state -- box it on the GC heap instead of the default value tuple + generatorObject->internalFlags |= InternalFlags::BoxAsObject; // copy location info, to fix issue with names of anonymous functions generatorObject->pos = functionLikeDeclarationBaseAST->pos; diff --git a/tslang/test/tester/tests/00generator_manual_next2.ts b/tslang/test/tester/tests/00generator_manual_next2.ts index 38bebcbc7..598c6adb0 100644 --- a/tslang/test/tester/tests/00generator_manual_next2.ts +++ b/tslang/test/tester/tests/00generator_manual_next2.ts @@ -35,17 +35,11 @@ function main1() { assert(rb.done); } -// manually drain a generator entirely from inside a plain (non-generator) helper -// function that receives it as a parameter. -// -// NOTE: driving the SAME iterator further from the caller after it has been passed -// into and mutated by a helper function is a known, separate bug (not covered here): -// generator objects have value semantics and are copied across a function-parameter -// boundary, so .next() calls made inside the callee do not advance the caller's -// binding. That is unlike a same-function const local (see main1/00generator_manual_next.ts), -// which works via an alloca-caching mechanism scoped to a single function body. Fixing -// this would require pass-by-reference semantics for generator-typed parameters at the -// ABI level in mlirGenFunctionParams -- out of scope for this regression file. +// manually drain a generator partially from inside a plain (non-generator) helper +// function that receives it as a parameter, then continue driving the SAME iterator +// from the caller. The generator wrapper is a reference type (heap-boxed ObjectType), +// so the callee's .next() calls advance the caller's binding too -- regression +// coverage for the former value-semantics copy bug at the function-parameter boundary. function drainTwo(it: ReturnType) { const first = it.next(); const second = it.next(); @@ -58,6 +52,31 @@ function main2() { const [v0, v1] = drainTwo(it); assert(v0 == 5); assert(v1 == 6); + + // the caller's binding must observe the callee's two next() calls + let r = it.next(); + assert(r.value == 7); + assert(!r.done); + + r = it.next(); + assert(r.value == 8); + + r = it.next(); + assert(r.done); + + // plain assignment aliases the same generator state + const a = gen(0, 5); + const b = a; + a.next(); // consumes 0 + const rb = b.next(); + assert(rb.value == 1); + + // closure capture aliases the same generator state + const it2 = gen(100, 3); + const drainOne = () => { it2.next(); }; + drainOne(); + const r2 = it2.next(); + assert(r2.value == 101); } // generator closing over outer mutable state; manual .next() calls interleaved with diff --git a/tslang/ts-new-parser/enums.h b/tslang/ts-new-parser/enums.h index 021da2a76..5f01aff0e 100644 --- a/tslang/ts-new-parser/enums.h +++ b/tslang/ts-new-parser/enums.h @@ -556,7 +556,11 @@ enum class InternalFlags : number DllExport = 1 << 7, DllImport = 1 << 8, IsPublic = 1 << 9, - GenerationProcessed = 1 << 10 + GenerationProcessed = 1 << 10, + // object literal must be heap-boxed into a reference-typed ObjectType value + // instead of the default value-typed tuple (used for generator wrappers, + // whose mutable `step` state must be shared across aliases) + BoxAsObject = 1 << 11 }; ENUM_OPS(InternalFlags)