Box object literals with methods/accessors as reference-typed ObjectType

tslang/docs/object-literal-boxing-design.md

@@ -1,11 +1,72 @@
 # Object literals with methods as reference types (`ObjectType`): design
 
-Status: **proposed** — 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 stay value
-tuples. Follows the same investigate-first format; all anchors below verified
-by code inspection on main@172e013b.
+Status: **PR A merged (#248, main@9a7bd71e); PR B implemented, 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
+stay value tuples. Follows the same investigate-first format; all anchors
+below verified by code inspection on main@172e013b.
+
+## PR B implementation notes (the flip + fallout)
+
+The flip itself (§3 gap 4) was a 2-line change
+(`MLIRGenExpressions.cpp:1343-1345`), but it exercises `ObjectType` through
+every code path that previously only ever saw tuples for a literal with
+methods — each missing `ObjectType` case in those paths surfaced as a crash,
+a wrong-answer, or a `never`-typed variable. Five fixes were needed beyond
+the flip itself, all following the same shape as PR A's gaps (an existing
+tuple-shaped dispatch missing an `ObjectType` case that looks through to
+`getStorageType()`):
+
+1. **Boxing seed value**: routing the `boxAsObject` branch's initial
+   const-tuple-to-mutable-tuple step through `mlirGenCreateTuple` (which
+   allocates + stores directly) instead of the generic `CAST_A` pipeline
+   (which re-reads every field, including method fields, individually) —
+   the latter triggered a spurious-but-harmless "losing this reference"
+   warning on every boxed literal, since it read a `this`-bound method value
+   off the source const-tuple only to immediately discard it.
+   (`MLIRGenExpressions.cpp:1352-1360`.)
+2. **Accessor get/set on a pointer operand**: `TupleGetSetAccessor`
+   (`MLIRCodeLogic.h`, shared by `Tuple()` for value tuples and `RefLogic()`
+   for pointers/objects) unconditionally used `ExtractPropertyOp`, whose
+   operand is restricted to value-typed `AnyStructLike` — not
+   `ObjectType`/`RefType`. Needed a `PropertyRefOp`+`LoadOp` branch for
+   ref-like operands, mirroring `RefLogic`'s own direct-field-access code
+   right below it. Root-caused via MLIR dump comparison, not guesswork: the
+   verifier error named the exact operand-type constraint.
+3. **`castObjectToInterface` missing the field-coercion fallback**:
+   `castTupleToInterface` has a "clone with interface's field types then box"
+   fallback for e.g. an inferred `si32` field vs. an interface declaring
+   `number` — necessary because that fallback only fires from
+   `castTupleToInterface`, which a literal already boxed to `ObjectType`
+   never reaches (PR A's dispatcher routes `ObjectType` straight to
+   `castObjectToInterface`). Duplicated the same clone-and-coerce logic
+   there, operating on the object's storage type. (`MLIRGenCast.cpp`.)
+4. **Element access** (`obj[Symbol.x]`, `obj["field"]`) had no `ObjectType`
+   case in `mlirGenElementAccess`'s dispatcher at all (`llvm_unreachable`
+   fallback) — added one that delegates to `mlirGenPropertyAccessExpression`
+   for a constant string/symbol key, same as the `ClassType`/`InterfaceType`
+   cases already there. (`MLIRGenAccessCall.cpp`.)
+5. **Generic intersection-type return values** (`D & M` where `M` infers to
+   a method-bearing literal's `ObjectType`): the tuple-merge loop inside
+   `getIntersectionType(IntersectionTypeNode, ...)` had no `ObjectType` case
+   and silently returned `NeverType` for one, breaking any generic function
+   returning an intersection that includes a boxed argument type. Also
+   generalized `tryInferTuple` (generic parameter inference) the same way,
+   though that one turned out not to be on the failing path for the actual
+   regression test — kept anyway since it's the same latent gap.
+   (`MLIRGenTypes.cpp`, `MLIRGenGenerics.cpp`.)
+
+None of these were guessed — each was root-caused via a crash/error message
+or an MLIR/LLVM IR dump diff against the pre-flip baseline (`git stash`).
+One red herring debugged and ruled out: an accessor (`get`/`set`) whose
+backing field infers `si32` but whose declared parameter type is `number`
+corrupts the write — reproduces identically on unmodified pre-PR-B `main`,
+so it's a pre-existing, unrelated bug, not something this change caused.
+Documented as a known limitation in `00object_ref_semantics.ts` (worked
+around there by using a float literal) rather than fixed, since it's out of
+scope for the boxing flip.
 
 ## 1. Goal and rationale
 
@@ -116,18 +177,20 @@ Things that consume the literal's *value type* and must tolerate
 
 ## 5. Staged PR plan (small, squash-merged, per the established workflow)
 
-1. **PR A — infrastructure, no behavior change**: gaps 1–3. Test via
-   generator objects, which are already boxed: spread a generator object,
-   `getFields`-driven paths on it, annotated-tuple assignment from one.
-   Full suite must stay 350/354 green.
-2. **PR B — the flip**: gap 4 + regression tests. New test
-   `00object_ref_semantics.ts`: alias-mutation via second binding
-   (`const b = a; b.inc(); a.get()`), parameter passing, closure capture,
-   array element, object nested in object, accessor mutating `this`,
-   global `const` literal with mutating method (generalizes the #246 repro),
-   conditional-expression merge of two same-shape literals. Then the known
-   fragile set (`00disposable*`, `00spread`, `01symbol`, generator suite) and
-   full ctest.
+1. **PR A — infrastructure, no behavior change** (#248, merged): gaps 1–3.
+   Test via generator objects, which are already boxed: spread a generator
+   object, `getFields`-driven paths on it, annotated-tuple assignment from
+   one. 352/352 JIT + 356/356 AOT green.
+2. **PR B — the flip** (implemented): gap 4 + regression tests, plus five
+   more `ObjectType`-look-through gaps discovered by actually running the
+   flip (see "PR B implementation notes" above — accessor get/set, interface
+   cast field coercion, element access, generic intersection-type merge,
+   generic parameter inference). New test `00object_ref_semantics.ts`:
+   alias-mutation via second binding (`const b = a; b.inc(); a.get()`),
+   parameter passing, closure capture, array element, object nested in
+   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`

tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h

@@ -1334,47 +1334,88 @@ class MLIRPropertyAccessCodeLogic
                 return mlir::failure();
             }
 
+            // `expression` is pointer-like (ObjectType, or a Ref/BoundRef reached via
+            // RefLogic) when this accessor lookup came from a boxed object literal
+            // (docs/object-literal-boxing-design.md) rather than a value-typed tuple:
+            // ExtractPropertyOp's operand is restricted to AnyStructLike (value tuples/
+            // storage types), which ObjectType/RefType are not part of, so such operands
+            // need PropertyRefOp + LoadOp instead (same recipe as RefLogic's own direct
+            // field access just above/below this helper).
+            auto expressionIsRefLike = isa<mlir_ts::ObjectType>(expression.getType())
+                || isa<mlir_ts::RefType>(expression.getType())
+                || isa<mlir_ts::BoundRefType>(expression.getType());
+
             mlir::Value getterValue;
             mlir::Value setterValue;
 
             if (getterIndex >= 0)
             {
-                getterValue = builder.create<mlir_ts::ExtractPropertyOp>(location, getterFuncType, expression, 
-                    MLIRHelper::getStructIndex(builder, getterIndex));
+                if (expressionIsRefLike)
+                {
+                    auto propRef = builder.create<mlir_ts::PropertyRefOp>(location, mlir_ts::RefType::get(getterFuncType),
+                        expression, builder.getI32IntegerAttr(getterIndex));
+                    getterValue = builder.create<mlir_ts::LoadOp>(location, getterFuncType, propRef);
+                }
+                else
+                {
+                    getterValue = builder.create<mlir_ts::ExtractPropertyOp>(location, getterFuncType, expression,
+                        MLIRHelper::getStructIndex(builder, getterIndex));
+                }
             }
             else
             {
-                getterValue = builder.create<mlir_ts::UndefOp>(location, 
+                getterValue = builder.create<mlir_ts::UndefOp>(location,
                     mlir_ts::FunctionType::get(
-                        builder.getContext(), 
-                        {mlir_ts::OpaqueType::get(builder.getContext())}, 
-                        {accessorResultType}, 
+                        builder.getContext(),
+                        {mlir_ts::OpaqueType::get(builder.getContext())},
+                        {accessorResultType},
                         false));
             }
 
             if (setterIndex >= 0)
             {
-                setterValue = builder.create<mlir_ts::ExtractPropertyOp>(location, setterFuncType, expression, 
-                    MLIRHelper::getStructIndex(builder, setterIndex));
+                if (expressionIsRefLike)
+                {
+                    auto propRef = builder.create<mlir_ts::PropertyRefOp>(location, mlir_ts::RefType::get(setterFuncType),
+                        expression, builder.getI32IntegerAttr(setterIndex));
+                    setterValue = builder.create<mlir_ts::LoadOp>(location, setterFuncType, propRef);
+                }
+                else
+                {
+                    setterValue = builder.create<mlir_ts::ExtractPropertyOp>(location, setterFuncType, expression,
+                        MLIRHelper::getStructIndex(builder, setterIndex));
+                }
             }
             else
             {
-                setterValue = builder.create<mlir_ts::UndefOp>(location, 
+                setterValue = builder.create<mlir_ts::UndefOp>(location,
                     mlir_ts::FunctionType::get(
-                        builder.getContext(), 
-                        {mlir_ts::OpaqueType::get(builder.getContext()), 
-                        accessorResultType}, 
-                        {}, 
+                        builder.getContext(),
+                        {mlir_ts::OpaqueType::get(builder.getContext()),
+                        accessorResultType},
+                        {},
                         false));
             }
 
-            auto refValue = getExprLoadRefValue(location);
-            if (!refValue)
+            mlir::Value refValue;
+            if (expressionIsRefLike)
+            {
+                // already a pointer (e.g. a boxed object literal's ObjectType) -- use it
+                // directly as `this`, same as RefLogic's own direct field access does;
+                // wrapping it in another VariableOp/RefType would add a spurious
+                // pointer-to-pointer indirection the accessor call doesn't expect.
+                refValue = expression;
+            }
+            else
             {
-                // allocate in stack
-                refValue = builder.create<mlir_ts::VariableOp>(
-                    location, mlir_ts::RefType::get(expression.getType()), expression);
-            }                    
+                refValue = getExprLoadRefValue(location);
+                if (!refValue)
+                {
+                    // allocate in stack
+                    refValue = builder.create<mlir_ts::VariableOp>(
+                        location, mlir_ts::RefType::get(expression.getType()), expression);
+                }
+            }
 
             auto thisValue = refValue;
 

tslang/lib/TypeScript/MLIRGenAccessCall.cpp

@@ -879,6 +879,24 @@ namespace mlirgen
         {
             return mlirGenElementAccessTuple(location, expression, argumentExpression, constTupleType);
         }
+        else if (auto objectType = dyn_cast<mlir_ts::ObjectType>(arrayType))
+        {
+            // boxed object literal (docs/object-literal-boxing-design.md): field access on
+            // the pointer already works via mlirGenPropertyAccessExpression's ObjectType
+            // case (cl.Object -> RefLogic), same recipe as the ClassType/InterfaceType
+            // cases below -- only computed string-key access (`obj[Symbol.x]`,
+            // `obj["field"]`) is supported, matching those cases.
+            if (auto fieldName = argumentExpression.getDefiningOp<mlir_ts::ConstantOp>())
+            {
+                auto attr = fieldName.getValue();
+                if (isa<mlir::StringAttr>(attr))
+                {
+                    return mlirGenPropertyAccessExpression(location, expression, attr, isConditionalAccess, genContext);
+                }
+            }
+
+            llvm_unreachable("not implemented (ElementAccessExpression)");
+        }
         else if (auto classType = dyn_cast<mlir_ts::ClassType>(arrayType))
         {
             if (auto fieldName = argumentExpression.getDefiningOp<mlir_ts::ConstantOp>())

tslang/lib/TypeScript/MLIRGenCast.cpp

@@ -1624,16 +1624,61 @@ namespace mlirgen
     ValueOrLogicalResult MLIRGenImpl::castObjectToInterface(mlir::Location location, mlir::Value in, mlir_ts::ObjectType objType,
                                     InterfaceInfo::TypePtr interfaceInfo, const GenContext &genContext)
     {
-        auto result = mlirGenCreateInterfaceVTableForObject(location, in, objType, interfaceInfo, genContext);
+        auto inEffective = in;
+        auto effectiveObjType = objType;
+
+        // same field-type-coercion fallback as castTupleToInterface (e.g. a literal
+        // integer field inferred as si32 vs an interface declaring `number`): an
+        // object literal with methods is boxed as ObjectType before ever reaching
+        // castTupleToInterface (see docs/object-literal-boxing-design.md), so that
+        // cast's clone-and-coerce step must be duplicated here rather than relying
+        // on falling through to it.
+        if (auto storageTuple = dyn_cast<mlir_ts::TupleType>(objType.getStorageType()))
+        {
+            if (mlir::failed(mth.canCastTupleToInterface(location, storageTuple, interfaceInfo, true)))
+            {
+                SmallVector<mlir_ts::FieldInfo> fields;
+                if (mlir::failed(interfaceInfo->getTupleTypeFields(fields, builder.getContext())))
+                {
+                    return mlir::failure();
+                }
+
+                // append all fields from original tuple
+                for (auto origField : storageTuple.getFields()) {
+                    if (std::find_if(
+                        fields.begin(),
+                        fields.end(),
+                        [&] (auto& item) {
+                            return item.id == origField.id;
+                        }) == fields.end())
+                    {
+                        fields.push_back(origField);
+                    }
+                }
+
+                auto newInterfaceTupleType = getTupleType(fields);
+
+                CAST_A(unboxed, location, newInterfaceTupleType, in, genContext);
+
+                auto valueAddr = builder.create<mlir_ts::NewOp>(location, mlir_ts::ValueRefType::get(newInterfaceTupleType), builder.getBoolAttr(false));
+                builder.create<mlir_ts::StoreOp>(location, unboxed, valueAddr);
+                effectiveObjType = mlir_ts::ObjectType::get(newInterfaceTupleType);
+                inEffective = builder.create<mlir_ts::CastOp>(location, effectiveObjType, valueAddr);
+
+                emitWarning(location, "") << "Cloned object is used. Ensure all types are matching to interface: " << interfaceInfo->fullName;
+            }
+        }
+
+        auto result = mlirGenCreateInterfaceVTableForObject(location, inEffective, effectiveObjType, interfaceInfo, genContext);
         EXIT_IF_FAILED_OR_NO_VALUE(result)
         auto createdInterfaceVTableForObject = V(result);
 
         LLVM_DEBUG(llvm::dbgs() << "\n!!"
                                 << "@ created interface:" << createdInterfaceVTableForObject << "\n";);
 
-        return V(builder.create<mlir_ts::NewInterfaceOp>(location, 
-            mlir::TypeRange{interfaceInfo->interfaceType}, in, createdInterfaceVTableForObject));
-    }    
+        return V(builder.create<mlir_ts::NewInterfaceOp>(location,
+            mlir::TypeRange{interfaceInfo->interfaceType}, inEffective, createdInterfaceVTableForObject));
+    }
 
     mlir_ts::CreateBoundFunctionOp MLIRGenImpl::createBoundMethodFromExtensionMethod(mlir::Location location, mlir_ts::CreateExtensionFunctionOp createExtentionFunction)
     {

tslang/lib/TypeScript/MLIRGenExpressions.cpp

@@ -1333,8 +1333,16 @@ namespace mlirgen
         auto constantVal =
             builder.create<mlir_ts::ConstantOp>(location, constTupleTypeWithReplacedThis, arrayAttr);
 
+        // box any literal with a method/accessor as a reference-typed ObjectType, not just
+        // synthetic wrappers carrying the explicit flag (see docs/object-literal-boxing-design.md
+        // §3 gap 4): a method can only mutate its object through `this`, and predicting which
+        // methods actually do so is interprocedural/undecidable, so the sound structural
+        // approximation -- "has any bound-method field" -- is used instead. Keep the flag check
+        // too: it still documents intent for synthetic wrappers (e.g. the generator wrapper) and
+        // is redundant-but-harmless once such wrappers also have methods.
         auto boxAsObject =
-            (objectLiteral->internalFlags & InternalFlags::BoxAsObject) == InternalFlags::BoxAsObject;
+            (objectLiteral->internalFlags & InternalFlags::BoxAsObject) == InternalFlags::BoxAsObject ||
+            !oli.methodInfos.empty() || !oli.methodInfosWithCaptures.empty();
 
         if (oli.fieldsToSet.empty() && !boxAsObject)
         {
@@ -1343,16 +1351,15 @@ namespace mlirgen
 
         auto tupleType = mth.convertConstTupleTypeToTupleType(constantVal.getType());
 
-        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;
-        }
+        // this branch is only reached when boxAsObject is true (see the early return above),
+        // so route it through mlirGenCreateTuple even with an empty fieldsToSet: it allocates
+        // storage and initializes it directly from constantVal, unlike the generic cast()
+        // pipeline (CAST_A) which re-reads every field individually -- for a method field that
+        // means reading a this-bound BoundFunctionType off the source const-tuple then casting
+        // it down to plain FunctionType, losing the binding ("losing this reference" warning)
+        // for a value that gets discarded anyway once boxed (the real `this` at call time comes
+        // from the boxed ObjectType pointer, not from this seed value).
+        mlir::Value tupleValue = mlirGenCreateTuple(location, tupleType, constantVal, oli.fieldsToSet, genContext);
 
         if (!boxAsObject)
         {