From a1fd9988fdf5770dfc7d4c5f71d676643cc9748d Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Sat, 18 Jul 2026 21:16:10 +0100 Subject: [PATCH] Remove redundant bound-method-field check in GlobalOpLowering The #246 isBoundMethodTupleConstant special case in GlobalOpLowering decided whether a global's initializer needs the constructor path based on whether any ConstantOp in it had a bound-method-shaped tuple field. Since PR #249 boxes every method-bearing object literal, such a literal's initializer always contains a NewOp (the boxing recipe), which the walk's existing NewOp case already forces onto the constructor path -- making the bound-method check redundant. Verified via a diagnostic probe at that call site across every test file (zero hits) before removing it for real. Investigated but did NOT remove needsIdentityStorage/boundRefMaterializedCache: contrary to the original plan (docs/object-literal-boxing-design.md), these are still load-bearing. Whenever a boxed literal is unboxed back into a plain tuple to match a declared/inferred type (annotated const, parameter, interface-cast fallback, generic intersection/inference), the resulting tuple's method field is still bound-method-shaped even though the tuple itself is a plain value -- that machinery is what makes calling through it work correctly instead of crash. Root-caused via site-tagged diagnostic probes across the full test corpus; documented in the design doc so this isn't re-attempted without re-deriving the same finding. Full suite green: 353/353 JIT + 357/357 AOT. Co-Authored-By: Claude Sonnet 5 --- tslang/docs/object-literal-boxing-design.md | 48 ++++++++++++++++----- tslang/lib/TypeScript/LowerToLLVM.cpp | 24 ----------- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/tslang/docs/object-literal-boxing-design.md b/tslang/docs/object-literal-boxing-design.md index 3eb7b59da..eb12e5a47 100644 --- a/tslang/docs/object-literal-boxing-design.md +++ b/tslang/docs/object-literal-boxing-design.md @@ -1,7 +1,8 @@ # Object literals with methods as reference types (`ObjectType`): design -Status: **PR A merged (#248, main@9a7bd71e); PR B implemented, full suite -green (353/353 JIT + 357/357 AOT)** — generalizes the generator-wrapper +Status: **PR A merged (#248, main@9a7bd71e); PR B merged (#249, +main@9f0d1a6a); PR C implemented (narrower than planned — see §5), full +suite green (353/353 JIT + 357/357 AOT)** — generalizes the generator-wrapper boxing of PR #245 (`docs/generator-object-wrapper-design.md`) from "literals carrying the `BoxAsObject` flag" to **every object literal that has at least one method or accessor**. Pure-data literals (`{ x: 1, y: 2 }`) deliberately @@ -191,13 +192,37 @@ Things that consume the literal's *value type* and must tolerate object, accessor mutating `this`, global `const` literal with mutating method (generalizes the #246 repro), conditional-expression merge of two same-shape literals. Full suite green: 353/353 JIT + 357/357 AOT. -3. **PR C — cleanup** (only after B is green on main): remove - `needsIdentityStorage` (`MLIRGenImpl.h:788-794`, - `MLIRGenVariables.cpp:102-113`), `boundRefMaterializedCache` - (`MLIRGenImpl.h:10797+`), the #246 `isBoundMethodTupleConstant` check in - `GlobalOpLowering` (`LowerToLLVM.cpp:3696-3731`), and stale comments — - each removal proven dead by the flip, verified by full suite. Keeping - them through B preserves bisectability. +3. **PR C — cleanup, narrower than originally planned** (implemented): the + assumption that `needsIdentityStorage` and `boundRefMaterializedCache` + become fully dead after the flip was **wrong** — verified empirically + (not by reasoning) via temporary `report_fatal_error` probes at + `hasBoundMethodField` and both `isBoundRef` call sites + (`MLIRCodeLogic.h`'s `Tuple()`/`TupleNoError()`), full suite rerun: 8 of + 10 originally-regressed-then-fixed tests plus 2 more (`00question_question`, + `00type_aliases_in_generics`) still hit them. Root cause: the **annotated + tuple-type boundary** noted below in §6 is not just a documented + value-semantics limitation — the code path that makes it *work correctly* + (rather than crash) is exactly `isBoundReference`/`boundRefMaterializedCache` + in `Tuple()`. Whenever a boxed `ObjectType` literal is unboxed back into a + plain tuple to match a declared/inferred type (an annotated `const`, a + function parameter, an interface-cast fallback, a generic + intersection/parameter-inference result — anywhere PR A's gap-3 unboxing + cast or an equivalent fires), the resulting tuple's method field is still + *shaped* like a bound method (first input `ObjectType`) even though the + tuple itself is now a plain value. `Tuple()`'s `isBoundRef` branch is what + correctly materializes `this` for a call through that field. **Kept as + permanent, still-necessary machinery, not scheduled for removal.** + + The one piece confirmed genuinely dead: the #246 + `isBoundMethodTupleConstant` check in `GlobalOpLowering` + (`LowerToLLVM.cpp`, formerly ~3696-3731) — removed. Every boxed literal's + initializer region unconditionally contains a `NewOp` (the boxing + recipe), which the walk's existing `isa(op)` case already + forces onto the global-constructor path; the bound-method-field check + never changed the outcome. Verified by running every test file directly + through the JIT with a diagnostic (non-fatal) probe at that specific call + site — zero hits across the full ~369-file test corpus — before removing + it for real and reconfirming 353/353 JIT + 357/357 AOT green. ## 6. Risks / open items @@ -206,7 +231,10 @@ Things that consume the literal's *value type* and must tolerate tuple at the annotation boundary, losing aliasing for that binding. The eventual fix is in *type resolution* (map method-bearing type literals to `ObjectType`), which is a bigger, separate change — out of scope; document - the limitation in the test. + the limitation in the test. **This is also why `needsIdentityStorage` / + `boundRefMaterializedCache` can't be removed (§5 PR C)**: the unboxed + tuple's method field still has a bound-method *shape*, and that machinery + is what makes calling through it work correctly rather than crash. - **Function-expression / arrow fields** (`{ f: () => ... }`, `{ f: function () {...} }`) are plain fields, not `methodInfos` entries — they do not trigger boxing. Matches the "methods only" rule; TS's diff --git a/tslang/lib/TypeScript/LowerToLLVM.cpp b/tslang/lib/TypeScript/LowerToLLVM.cpp index ea06be3dc..03aad3f23 100644 --- a/tslang/lib/TypeScript/LowerToLLVM.cpp +++ b/tslang/lib/TypeScript/LowerToLLVM.cpp @@ -3681,23 +3681,6 @@ struct GlobalOpLowering : public TsLlvmPattern LLVMCodeHelper lch(globalOp, rewriter, getTypeConverter(), tsLlvmContext->compileOptions); - // A ConstantOp whose *TupleType/ConstTupleType result* has a bound-method - // field (e.g. a plain `const obj = { ..., method() {} }` at global scope) - // lowers its method field via LLVM::AddressOfOp + LLVM::InsertValueOp (see - // LLVMCodeHelper::getTupleFromArrayAttr) -- not valid inside a static LLVM - // global initializer region, only inside real code (a function), so such a - // global must go through the constructor path below. Deliberately narrower - // than "any ConstantOp whose attribute contains a FlatSymbolRefAttr": that - // broader check also caught MSVC RTTI/EH type-descriptor globals (e.g. - // `??_R0PEAD@8`, which reference the `type_info` vtable symbol) that are - // NOT mlir_ts tuple types and were already lowering correctly as static - // data -- forcing those through the constructor path broke JIT symbol - // materialization for the whole module. - MLIRTypeHelper mth(rewriter.getContext(), tsLlvmContext->compileOptions); - auto isBoundMethodTupleConstant = [&](mlir_ts::ConstantOp constantOp) { - return mth.hasBoundMethodField(constantOp.getType()); - }; - auto createAsGlobalConstructor = false; // TODO: we need to write correct attributes to Ops and detect which ops should be in GlobalConstructor auto visitorAllOps = [&](Operation *op) { @@ -3722,13 +3705,6 @@ struct GlobalOpLowering : public TsLlvmPattern createAsGlobalConstructor = true; } } - else if (auto constantOp = dyn_cast(op)) - { - if (isBoundMethodTupleConstant(constantOp)) - { - createAsGlobalConstructor = true; - } - } // TODO: error in try-catch // if it has memory side effect - can't be in global init.