diff --git a/docs/MLIRGen-refactoring-review.md b/docs/MLIRGen-refactoring-review.md index 6b6012a36..b3d0a49eb 100644 --- a/docs/MLIRGen-refactoring-review.md +++ b/docs/MLIRGen-refactoring-review.md @@ -195,3 +195,25 @@ Proposed phases, each independently shippable: Order: 1 (trivial) → 3 (bounded signature ripple) → 2 (site-by-site, one PR per field group) → 4. *Status update:* phase 1 is done, plus a discovered **phase 0**: nine of the 31 casts were gratuitous — non-const method calls on the copyable `funcOp` op handle (`getSymName`/`getCallableResults`/`getCallableRegion`/`setPersonalityAttr`), a deref of the `inferTypes` pointer member (pointee was never const), and a cast on a local non-const context in `getConditionalType`. Remaining casts are all genuine mutations for phases 2–4: `thisType` ×2, `typeAliasMap` ×2, loop flags ×3 sites, `generatedStatements` ×4, `typeParamsWithArgs` ×3 (`zipTypeParameterWithArgument` at ~16587, `processConditionalForType`, mapped-type erase), plus `MLIRCodeLogic.h`. + +*Second status update:* phase 3 landed (#210 — inference family takes `GenContext&`; `getMappedType` copy also fixed a key-leak latent bug), and phase 2a landed (#211 — loop flags and `instantiateSpecializedFunction`'s `thisType` override scoped to local contexts; both were real caller-context leaks). + +### A7. The remaining casts are upward mailboxes — channel redesign needed (proposal) + +The 10 casts left (8 active in MLIRGen.cpp + 2 in `MLIRCodeLogic.h`; each now carries a `NOTE: upward mailbox` comment) all *intentionally* write into the caller's context so that **later** code observes the value. Copy-on-override cannot work, and `GenContext&` signatures would ripple through the whole statement chain. Worse, their correctness depends on `GenContext`'s value-copy semantics — a write is visible exactly to code whose context is (a copy of a copy of…) the mutated object made *after* the write: + +- **`generatedStatements`** (push ×2 in class-member processing, drain+clear ×2): class field initializers are queued, then drained once when the constructor body is generated; the per-copy `clear()` is what makes it process-once per subtree. +- **`typeAliasMap`** (insert ×2 in `checkSafeCastOne`): a synthesized safe-cast alias must resolve while compiling the *following* statements. Note the map legitimately serves a second, scoped role at ~15 other sites that set it on local copies. +- **`mlirGenParameters` `thisType`**: an explicit `this: T` parameter must reach the prototype chain above. +- **`usingVars = nullptr`** (~1557): process-once dispose marker. +- **`MLIRCodeLogic.h` `mlirGenSwitchState`**: arms `allocateVarsOutsideOfOperation`/`currentOperation` for the statements *following* the `__switchstate` call (the §4 plan misjudged these as parameterizable — they are out-channels, not in-params). + +Proposed redesign (one PR per channel, needs review before implementation): + +1. **`generatedStatements` → explicit channel object.** A `PendingStatements` struct owned by the class-processing scope; `GenContext` holds a plain pointer. Copies share the pointer, so "drain once" becomes a real property of the channel (swap-out) instead of an accident of which copy got cleared. This also removes the current ambiguity where clearing a copy may or may not shield sibling scopes. +2. **safe-cast aliases → per-function alias registry.** Split `typeAliasMap`'s two roles: keep the by-value map for scoped generic aliases; add a pointer to a function-scope `SmallVector`-backed registry for the synthesized safe-cast aliases (names are anonymous, collision-free). `getEmbeddedType`'s lookup checks both. +3. **`mlirGenParameters`** should stop poking and instead *return* the discovered `this` type (it already returns a tuple; the `this` param is in the returned params) — the caller decides what context to thread it into. Requires auditing what `mlirGenFunctionPrototype` and below read `genContext.thisType` for. +4. **`mlirGenSwitchState`** → return the armed state (`std::pair` or a small struct) from `callMethod`'s special-case path and let the call-expression handler apply it to a context it owns. Requires `callMethod` to distinguish "value result" from "context effect" results. +5. **`usingVars`** → same pointer-channel treatment as (1), or fold into the disposal walk's own state. + +Until a channel lands, its casts stay — documented and greppable via the `NOTE: upward mailbox` markers. diff --git a/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h b/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h index 8410ba1d1..405e3c9dd 100644 --- a/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h +++ b/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h @@ -892,6 +892,8 @@ class MLIRCustomMethods auto *block = builder.createBlock(builder.getBlock()->getParent()); switchStateOp.setSuccessor(block, 0); + // NOTE: upward mailbox: arms variable allocation placement for the statements following the + // __switchstate call in the caller's scope - see docs/MLIRGen-refactoring-review.md A7 const_cast(genContext).allocateVarsOutsideOfOperation = true; const_cast(genContext).currentOperation = switchStateOp; diff --git a/tslang/lib/TypeScript/MLIRGen.cpp b/tslang/lib/TypeScript/MLIRGen.cpp index 17fdb8159..41adaf576 100644 --- a/tslang/lib/TypeScript/MLIRGen.cpp +++ b/tslang/lib/TypeScript/MLIRGen.cpp @@ -1470,6 +1470,7 @@ class MLIRGenImpl std::back_inserter(generatedStatements)); // clean up + // NOTE: upward mailbox into caller context (process-once drain) - see docs/MLIRGen-refactoring-review.md A7 const_cast(genContext).generatedStatements.clear(); // auto generated code @@ -1554,6 +1555,7 @@ class MLIRGenImpl // remove when used if (disposeDepth == DisposeDepth::CurrentScope) { + // NOTE: upward mailbox into caller context (process-once) - see docs/MLIRGen-refactoring-review.md A7 const_cast(genContext)->usingVars = nullptr; } @@ -5106,6 +5108,7 @@ class MLIRGenImpl // special case, setup 'this' and type provided if (namePtr == THIS_NAME && type) { + // NOTE: upward mailbox: explicit this-parameter type must reach the prototype chain - see A7 const_cast(genContext).thisType = type; LLVM_DEBUG(dbgs() << "\n!! param " << THIS_NAME << " mapped to type " << type << "\n"); @@ -7624,6 +7627,7 @@ class MLIRGenImpl // enable safe cast found auto typeAliasNameUTF8 = MLIRHelper::getAnonymousName(loc_check(textRange), "ta_", getNamespaceName()); auto typeAliasName = convertUTF8toWide(typeAliasNameUTF8); + // NOTE: upward mailbox: alias must stay visible for following statements - see A7 const_cast(genContext) .typeAliasMap.insert({typeAliasNameUTF8, tupleType}); @@ -7646,6 +7650,7 @@ class MLIRGenImpl // enable safe cast found auto typeAliasNameUTF8 = MLIRHelper::getAnonymousName(loc_check(textRange), "ta_", getNamespaceName()); auto typeAliasName = convertUTF8toWide(typeAliasNameUTF8); + // NOTE: upward mailbox: alias must stay visible for following statements - see A7 const_cast(genContext) .typeAliasMap.insert({typeAliasNameUTF8, interfaceType}); @@ -8691,6 +8696,7 @@ class MLIRGenImpl } // clean up + // NOTE: upward mailbox into caller context (process-once drain) - see A7 const_cast(genContext).generatedStatements.clear(); } @@ -19817,6 +19823,7 @@ genContext); propertyDeclaration->initializer); auto expr_statement = nf.createExpressionStatement(_this_name_equal); + // NOTE: upward mailbox: drained when the constructor body is generated - see A7 const_cast(genContext).generatedStatements.push_back(expr_statement.as()); } @@ -19853,6 +19860,7 @@ genContext); nf.createBinaryExpression(_this_name, nf.createToken(SyntaxKind::EqualsToken), _name); auto expr_statement = nf.createExpressionStatement(_this_name_equal); + // NOTE: upward mailbox: drained when the constructor body is generated - see A7 const_cast(genContext).generatedStatements.push_back(expr_statement.as()); } }