Constant vtables for object-literal methods, skipping the per-cast heap patch

tslang/docs/interface-vtable-simplification-design.md

@@ -1,13 +1,13 @@
 # Interface vtable simplification: design
 
-Status: **PR 1 (§4 + §5) implemented, full suite green (716/716)**. §3
-(constant vtables for capture-free literal methods) not yet started. Follow-up
-to PR #251 (heap-allocated patched vtable) and PR #252 (canonical slot
-numbering, `fix-interface-vtable-index-mismatch@be2c9620`). All anchors below
-verified by code inspection on that branch, except where PR 1's implementation
-notes (end of §4) correct them. Goal: remove the last *runtime-patched* vtable
-path for the common case, make slot numbering single-sourced, and fix one
-latent cast-order miscompile found during review.
+Status: **PR 1 (§4 + §5) and PR 2 (§3) both implemented, full suite green
+(718/718)**. Follow-up to PR #251 (heap-allocated patched vtable) and PR #252
+(canonical slot numbering, `fix-interface-vtable-index-mismatch@be2c9620`).
+All anchors below verified by code inspection at the time each PR started;
+see each section's "Implementation notes" subsection for what changed, or was
+found to be wrong, while building it. Goal: remove the last *runtime-patched*
+vtable path for the common case, make slot numbering single-sourced, and fix
+latent bugs found during review and implementation.
 
 ## 1. Current architecture: what a vtable slot means
 
@@ -120,6 +120,78 @@ the #251 heap machinery becomes dead on the main path (kept for the fallback);
 the vtable global can be const-qualified; cross-module behavior stops depending
 on which module happened to run a cast.
 
+### Implementation notes (PR 2, as landed)
+
+Two corrections to the plan above, both found by testing rather than
+inspection - the "captures need a fallback" premise was actually wrong in the
+*helpful* direction (one fewer case to handle), and a second, unrelated
+distinction turned out to be load-bearing that the plan didn't mention at all.
+
+**Captures don't need the fallback.** Rereading `addObjectFuncFieldInfo`
+(`MLIRGenImpl.h`) shows the func-typed field's value is
+`FlatSymbolRefAttr(funcName)` unconditionally - captures-with-methods land in
+`methodInfosWithCaptures` *in addition to*, not *instead of*, the same
+symbol-valued field. What captures actually add is a separate `.captured`
+data field the (single, shared) lifted function reads via `this` at call time
+(`mlirGenObjectLiteralCaptures`) - not a different function per instance. So
+the side table (`objectLiteralMethodSymbolsMap`, keyed on the object literal's
+own `ObjectStorageType`, populated in `addObjectFuncFieldInfo` for every
+method, captures or not) needed no captures-awareness at all. The imported-
+object-type fallback (side-table miss, since there's no local `funcOp` to
+name) is the only case actually exercised in practice today - no test yet
+covers it explicitly (see §7's test matrix, still open).
+
+**A vtable-identity subtlety this plan didn't anticipate, but had to solve
+to key the side table at all:** vtable globals are named by
+`hash_value(objectType)` (`interfaceVTableNameForObject`) - structural, not
+nominal. The worry this raises - two differently-implemented, same-shape
+literals colliding on one vtable global and a baked-in constant only being
+correct for one of them - does not happen, verified by compiling a
+counterexample (`{count:0,inc(){...+1}}` vs `{count:0,inc(){...+100}}`):
+they get different vtable globals. Why: a method field's `FunctionType`
+carries `this` as an explicit first parameter typed `object<object_storage<@literal's-own-symbol>>`,
+and that symbol embeds the literal's own source location - so the "same
+shape" premise never actually holds once the method's own signature is
+counted. This is also why the side table has to be keyed on that *nested*
+`ObjectStorageType` (recovered at the vtable-build site by reading
+`FunctionType.getInputs().front()` off the object's own field, cast to
+`ObjectType`, `.getStorageType()`), not on the boxed `ObjectType` directly,
+since the boxed type wraps a converted plain `TupleType` (`mlirGen(ObjectLiteralExpression)`,
+`convertConstTupleTypeToTupleType`), a *different* MLIR `Type` value than
+`oli.objThis` captured when the field was added, even though one is derived
+from the other.
+
+**The bug that actually broke the regression suite (22 failures on the first
+pass): `PropertySignature` with a function type is not a `MethodSignature`.**
+`interface Something { toString: () => string; }` categorizes `toString` as
+an interface **field** (`InterfaceInfo::fields`), not a method
+(`InterfaceInfo::methods`) - `getVirtualTable`'s `methodsAsFields=true` only
+governs how the *object's own* func-typed field gets resolved into the local
+`virtualTable` snapshot; it collapses every entry to `isField=true`
+regardless of which list the *interface* actually declared the member in. The
+access site, however, dispatches on the interface's real categorization
+(`InterfaceMembers` tries `findField` before `findMethod`) - so
+`toString`'s access always goes through `InterfaceFieldAccess`
+(`thisVal + slotValue`, offset semantics), never `InterfaceMethodAccess`
+(raw function-pointer slot, `BoundFunctionType`), no matter how the object
+stores it. The first version of this change substituted a constant
+`SymbolRefOp` into *any* func-typed slot, including `toString`'s - handing a
+raw function pointer to code that adds it to `thisVal` expecting an offset,
+producing a garbage call target. Crash confirmed via WinDbg
+(`WinDbgX.exe -g -c ".dump /ma ..."` then offline `!analyze -v`; see
+[[windbg-available-for-debugging]]): faulting instruction `call qword ptr
+[rax+rdx]`, both registers holding `thisVal`/slot-sum garbage. Fix: gate the
+symbol substitution on `newInterfacePtr->findMethod(fieldName)` actually
+finding the member in the interface's own (extends-inclusive) method list,
+not merely on the vtable entry being func-typed
+(`mlirGenObjectVirtualTableDefinitionForInterface`). Regression test:
+`00interface_function_typed_field.ts` - `00interface_object.ts` (pre-existing)
+already covered this pattern and is what surfaced the bug, but a minimal,
+purpose-labeled test makes the invariant explicit for future readers.
+
+718/718 tests green (716 from PR 1 + this section's new regression test +1
+already counted, plus the incidental fix above).
+
 ## 4. Change 2: a single writer for `virtualIndex`
 
 Slot numbering is currently embodied in four places:

tslang/lib/TypeScript/MLIRGenImpl.h

@@ -7423,6 +7423,21 @@ class MLIRGenImpl
         oli.values.push_back(mlir::FlatSymbolRefAttr::get(builder.getContext(), funcName));
         oli.fieldInfos.push_back({fieldId, type, false, mlir_ts::AccessLevel::Public});
 
+        // record (this literal's object-storage type, field name) -> the lifted function's
+        // symbol so a later interface vtable build (mlirGenObjectVirtualTableDefinitionForInterface)
+        // can emit a constant SymbolRefOp for this method slot instead of the runtime
+        // load-from-object patch - see docs/interface-vtable-simplification-design.md §3.
+        // oli.objThis.getStorageType() is the SAME (hash-consed) ObjectStorageType that ends
+        // up embedded as the "this" parameter of this field's own FunctionType, which is how
+        // the vtable builder recovers this key later even though by then it only has the
+        // BOXED object type (whose top-level storage is a plain TupleType, not this
+        // ObjectStorageType directly - see the design doc's §3 implementation notes for why
+        // that indirection is necessary, not optional). Captures are fine here too: a
+        // captures-bearing method's field value is still this same compile-time-constant
+        // funcName (the per-instance data lives in a separate `.captured` field the function
+        // reads via `this`, not in a different function per instance).
+        objectLiteralMethodSymbolsMap[oli.objThis.getStorageType()][fieldId] = funcName;
+
         if (getCaptureVarsMap().find(funcName) != getCaptureVarsMap().end())
         {
             oli.methodInfosWithCaptures.push_back({funcName, oli.fieldInfos.size() - 1});
@@ -10852,6 +10867,43 @@ class MLIRGenImpl
 
     llvm::ScopedHashTable<StringRef, InterfaceInfo::TypePtr> fullNameInterfacesMap;
 
+    // (object literal's ObjectStorageType -> (field name -> lifted function symbol)) for
+    // capture-free-or-not object-literal methods; see addObjectFuncFieldInfo's doc comment
+    // and docs/interface-vtable-simplification-design.md §3. Not namespace-scoped: an
+    // ObjectStorageType's symbol already embeds the literal's source location, so it's
+    // globally unique regardless of which namespace declared it.
+    mlir::DenseMap<mlir::Type, mlir::DenseMap<mlir::Attribute, std::string>> objectLiteralMethodSymbolsMap;
+
+    // fieldType is a method-as-field's FunctionType, whose first input is the object literal's
+    // own "this" type (ObjectType wrapping the ObjectStorageType key used in
+    // objectLiteralMethodSymbolsMap - see addObjectFuncFieldInfo). Returns the empty string if
+    // fieldType isn't a function, "this" isn't an ObjectType (e.g. a class instance, or an
+    // imported object type reconstructed from a @dllimport declaration with no local funcOp),
+    // or no method was ever registered for this exact (object, field) pair.
+    std::string lookupObjectLiteralMethodSymbol(mlir::Type fieldType, mlir::Attribute fieldId)
+    {
+        auto funcType = dyn_cast<mlir_ts::FunctionType>(fieldType);
+        if (!funcType || funcType.getInputs().empty())
+        {
+            return {};
+        }
+
+        auto thisObjectType = dyn_cast<mlir_ts::ObjectType>(funcType.getInputs().front());
+        if (!thisObjectType)
+        {
+            return {};
+        }
+
+        auto symbolsForObject = objectLiteralMethodSymbolsMap.find(thisObjectType.getStorageType());
+        if (symbolsForObject == objectLiteralMethodSymbolsMap.end())
+        {
+            return {};
+        }
+
+        auto symbolIt = symbolsForObject->second.find(fieldId);
+        return symbolIt == symbolsForObject->second.end() ? std::string{} : symbolIt->second;
+    }
+
     llvm::ScopedHashTable<StringRef, GenericInterfaceInfo::TypePtr> fullNameGenericInterfacesMap;
 
     llvm::ScopedHashTable<StringRef, VariableDeclarationDOM::TypePtr> fullNameGlobalsMap;

tslang/lib/TypeScript/MLIRGenInterfaces.cpp

@@ -212,6 +212,37 @@ namespace mlirgen
                     return mlir::failure();
                 }
 
+                // methods this literal's own funcName is known for were already baked into
+                // globalVTableRefValue as constant SymbolRefOps by
+                // mlirGenObjectVirtualTableDefinitionForInterface (see there, and
+                // docs/interface-vtable-simplification-design.md §3) - only the rest (an
+                // imported object type reconstructed from a @dllimport declaration, with no
+                // local funcOp to name) still need their function pointer read out of the
+                // actual object `in` at cast time.
+                llvm::SmallVector<InterfaceMethodInfo *> methodsNeedingPatch;
+                for (auto& method : newInterfacePtr->methods)
+                {
+                    auto fieldId = builder.getStringAttr(method.name);
+                    auto index = mth.getFieldIndexByFieldName(storeType, fieldId);
+                    if (index == -1)
+                    {
+                        return mlir::failure();
+                    }
+
+                    auto fieldInfo = mth.getFieldInfoByIndex(storeType, index);
+                    if (lookupObjectLiteralMethodSymbol(fieldInfo.type, fieldId).empty())
+                    {
+                        methodsNeedingPatch.push_back(&method);
+                    }
+                }
+
+                if (methodsNeedingPatch.empty())
+                {
+                    // every method slot is already a compile-time constant - no per-cast
+                    // heap allocation needed, same footing as a method-less interface.
+                    return globalVTableRefValue;
+                }
+
                 // match VTable
                 // 1) clone vtable onto the GC heap (NOT a stack VariableOp): this per-object
                 // patched vtable is pointed at by the resulting interface value, and that
@@ -226,8 +257,9 @@ namespace mlirgen
                 builder.create<mlir_ts::StoreOp>(location, valueVTable, heapVTable);
                 auto varVTable = builder.create<mlir_ts::CastOp>(location, globalVTableRefValue.getType(), heapVTable);
 
-                for (auto& method : newInterfacePtr->methods)
+                for (auto* methodPtr : methodsNeedingPatch)
                 {
+                    auto& method = *methodPtr;
                     auto index = mth.getFieldIndexByFieldName(storeType, builder.getStringAttr(method.name));
                     if (index == -1)
                     {
@@ -246,13 +278,13 @@ namespace mlirgen
                     auto methodRefVT = builder.create<mlir_ts::PropertyRefOp>(location, fieldInfoVT.type, varVTable, method.virtualIndex);
 
                     LLVM_DEBUG(llvm::dbgs() << "\n!!\n\t vtable method: " << method.name
-                                            << "\n\t vtable method ref: " << V(methodRefVT) << "\n\n";);                    
-                    
-                    builder.create<mlir_ts::LoadSaveOp>(location, methodRefVT, methodRef);   
-                }       
+                                            << "\n\t vtable method ref: " << V(methodRefVT) << "\n\n";);
+
+                    builder.create<mlir_ts::LoadSaveOp>(location, methodRefVT, methodRef);
+                }
 
                 // patched VTable
-                return V(varVTable);         
+                return V(varVTable);
             }
 
             return globalVTableRefValue;
@@ -301,9 +333,56 @@ namespace mlirgen
                 {
                     if (methodOrField.isField)
                     {
-                        auto nullObj = builder.create<mlir_ts::NullOp>(location, getNullType());                        
+                        // an object-literal method is resolved via the FIELD path too
+                        // (methodsAsFields=true, getInterfaceVirtualTableForObject) since it's
+                        // literally stored as a func-typed field on the object - but if it's
+                        // capture-free-or-not-relevant (its funcName is compile-time constant
+                        // regardless of captures, see addObjectFuncFieldInfo), we already know
+                        // exactly which function it is and don't need to derive an object-relative
+                        // offset for it at all: emit the function symbol directly, same as the
+                        // class vtable path (mlirGenClassVirtualTableDefinitionForInterface) does
+                        // for its own (isField=false) methods. This makes the slot a genuine
+                        // compile-time constant, skipping mlirGenCreateInterfaceVTableForObject's
+                        // per-cast heap-clone-and-patch entirely for such methods (see there).
+                        // See docs/interface-vtable-simplification-design.md §3.
+                        //
+                        // Gate on the INTERFACE's own categorization, not merely "is this
+                        // vtable entry func-typed": `methodsAsFields=true` makes every entry
+                        // here isField=true regardless of whether the interface declared it as
+                        // a MethodSignature (`inc(): void`) or a PropertySignature with a
+                        // function type (`toString: () => string`) - only the former is read
+                        // through InterfaceMethodAccess (BoundFunctionType access, raw funcptr
+                        // slot semantics) at the access site; the latter goes through
+                        // InterfaceFieldAccess, which computes thisVal + slotValue expecting an
+                        // OFFSET. Substituting a raw function pointer into a
+                        // PropertySignature-with-function-type's slot corrupts that
+                        // computation (crashes on the resulting garbage address).
+                        std::string methodFuncName;
                         if (!methodOrField.isMissing)
                         {
+                            auto fieldNameAttr = dyn_cast<mlir::StringAttr>(methodOrField.fieldInfo.id);
+                            if (fieldNameAttr && newInterfacePtr->findMethod(fieldNameAttr.getValue()))
+                            {
+                                methodFuncName = lookupObjectLiteralMethodSymbol(methodOrField.fieldInfo.type, methodOrField.fieldInfo.id);
+                            }
+                        }
+
+                        if (!methodFuncName.empty())
+                        {
+                            auto methodConstName = builder.create<mlir_ts::SymbolRefOp>(
+                                location, mlir::cast<mlir_ts::FunctionType>(methodOrField.fieldInfo.type),
+                                mlir::FlatSymbolRefAttr::get(builder.getContext(), methodFuncName));
+                            auto methodConstNameRef = cast(location, mlir_ts::RefType::get(methodOrField.fieldInfo.type),
+                                                           methodConstName, genContext);
+
+                            vtableValue = builder.create<mlir_ts::InsertPropertyOp>(
+                                location, virtTuple, methodConstNameRef, vtableValue,
+                                MLIRHelper::getStructIndex(builder, fieldIndex));
+                        }
+                        else if (!methodOrField.isMissing)
+                        {
+                            auto nullObj = builder.create<mlir_ts::NullOp>(location, getNullType());
+
                             // TODO: test cast result
                             auto objectNull = cast(location, objectType, nullObj, genContext, true);
                             if (!objectNull)

tslang/test/tester/CMakeLists.txt

@@ -285,6 +285,7 @@ add_test(NAME test-compile-00-interface-conjunction COMMAND test-runner "${PROJE
 add_test(NAME test-compile-00-interface-partial COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_partial.ts")
 add_test(NAME test-compile-00-interface-optional COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional.ts")
 add_test(NAME test-compile-00-interface-optional-cast-order COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional_cast_order.ts")
+add_test(NAME test-compile-00-interface-function-typed-field COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_function_typed_field.ts")
 add_test(NAME test-compile-00-interface-generic COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_generic.ts")
 add_test(NAME test-compile-00-interface-new COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_new.ts")
 add_test(NAME test-compile-00-interface-indexer COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_indexer.ts")
@@ -636,6 +637,7 @@ add_test(NAME test-jit-00-interface-conjunction COMMAND test-runner -jit "${PROJ
 add_test(NAME test-jit-00-interface-partial COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_partial.ts")
 add_test(NAME test-jit-00-interface-optional COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional.ts")
 add_test(NAME test-jit-00-interface-optional-cast-order COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional_cast_order.ts")
+add_test(NAME test-jit-00-interface-function-typed-field COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_function_typed_field.ts")
 add_test(NAME test-jit-00-interface-generic COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_generic.ts")
 add_test(NAME test-jit-00-interface-new COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_new.ts")
 add_test(NAME test-jit-00-interface-indexer COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_indexer.ts")