diff --git a/tslang/docs/interface-vtable-simplification-design.md b/tslang/docs/interface-vtable-simplification-design.md new file mode 100644 index 000000000..d559bd2c7 --- /dev/null +++ b/tslang/docs/interface-vtable-simplification-design.md @@ -0,0 +1,263 @@ +# 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. + +## 1. Current architecture: what a vtable slot means + +A vtable global exists per (implementer type, interface) pair — +`interfaceVTableNameForClass` / `interfaceVTableNameForObject`. Layout is +extends-recursive, then the interface's own methods in declaration order, then +its own fields in declaration order (`assignCanonicalVirtualIndexes`, +`MLIRGenStore.h`, added in #252). A slot holds one of three things: + +1. **Class method** → constant function-pointer symbol. The class builder + resolves the implementing method via `classInfo->findMethod(name)` (which + knows its `funcName`) and emits `SymbolRefOp(funcName)` straight into the + global's initializer (`MLIRGenClasses.cpp:1590-1597`). Fully constant, zero + runtime work. +2. **Field** (class or object) → byte offset of the field within the + implementer, encoded as a pointer via the GEP-on-null trick: cast `NullOp` + to the implementer type, take `&(null)->field` + (`MLIRGenClasses.cpp:1562-1586`, `MLIRGenInterfaces.cpp:304-343`). + Position-independent and constant. +3. **Object-literal method** → the odd one out, detailed in §2. + +Access site (`InterfaceSymbolRefOpLowering`, `LowerToLLVM.cpp:4851-4939`): +`VTableOffsetRefOp` **loads the slot value** (`LowerToLLVM.cpp:4746-4770`); +the `BoundFunctionType` branch then uses that value directly as the method's +function pointer (4881-4891), while the field branch computes +`ptrtoint(thisVal) + ptrtoint(slotValue)` (`calcFieldTotalAddrFunc`, 4894-4904). +Optional members are handled at *runtime* by comparing the slot value against +`-1` (4907-4927); the `-1` sentinel is written into the slot by the builder for +members missing on the cast target (`MLIRGenInterfaces.cpp:348-351`). + +Consequence worth stating explicitly: because the same interface access site +must serve both class-backed and literal-backed values, **slot semantics must +be uniform per slot across all implementers** — a method slot must always hold +a callable function pointer. This rules out "store the method-field's offset +and dereference at the access site" as a unification strategy (classes don't +store methods in instances), and it is why §3 keeps the class convention and +brings object literals to it, not the other way round. + +## 2. Why object-literal methods are special (and expensive) + +For a boxed literal (`docs/object-literal-boxing-design.md`), methods live *in +the object* as func-typed fields. For capture-free methods the field's +initializer is already a compile-time symbol — `addObjectFuncFieldInfo` pushes +`FlatSymbolRefAttr(funcName)` into the literal's const-tuple values +(`MLIRGenImpl.h:7411-7429`, name obtained from the lifted `funcOp` in +`processObjectFunctionLikeProto`, 7517). Methods **with captures** instead go +through `methodInfosWithCaptures` → `fieldsToSet` (7419-7426): the field holds +a per-instance closure/trampoline pointer that does not exist at compile time. + +The vtable builder for objects, however, only sees the implementer's *type*. +A tuple field of `FunctionType` carries no symbol name (unlike +`classInfo->findMethod`), so the builder cannot emit `SymbolRefOp` — its +method branch is literally `llvm_unreachable("not implemented yet")` +(`MLIRGenInterfaces.cpp:359`) and methods are resolved *as fields* +(`methodsAsFields = true`, `MLIRTypeHelper.h:1350-1439`). The shared global +therefore gets an **offset** in each method slot — which the `BoundFunctionType` +access branch would misinterpret as a function pointer. That is papered over at +every cast by `mlirGenCreateInterfaceVTableForObject` +(`MLIRGenInterfaces.cpp:183-262`): clone the global onto the GC heap +(`NewOp`+`LoadOp`+`StoreOp`, the #251 fix), then for each interface method +`LoadSaveOp` the function-pointer *value* out of the object's field into the +heap vtable's slot (229-252; `LoadSaveOp` = load-src-store-dst, +`LowerToLLVM.cpp:4137-4148`). So the patch is load-bearing: the unpatched +global is never valid for a method-bearing interface. + +Cost and bug tally of this path: a heap allocation plus O(methods) code per +cast; PR #251 (patched vtable was a stack alloca dangling out of a global's +`__cctor`); PR #252 (a module that never casts never runs the patch machinery's +index-assignment side effects — see §4); and the latent optional-member bug +(§5). + +## 3. Change 1: constant vtables for capture-free literal methods + +Key observation: for capture-free methods the patched content is a +**per-type constant**. Every instance of a given literal type carries the same +lifted-method pointers (the lifted function is per-literal-expression, and each +literal expression gets its own location-hashed `ObjectStorageType`). The +per-cast patch recomputes the same values every time. Fix: record the symbol at +literal-creation time and emit it like the class path does. + +- Add a side table on `MLIRGenImpl` (parallel to `fullNameClassesMap`): + `objectStorageName → (fieldId → funcName)`. Populate it in + `addObjectFuncFieldInfo` — it has all three in hand (`oli.objThis`'s storage + name, `fieldId`, `funcName`) — for the capture-free branch only. +- In `mlirGenObjectVirtualTableDefinitionForInterface`'s initializer lambda: + when a vtable member is func-typed and the (storage name, fieldId) lookup + hits, emit `SymbolRefOp(funcName)` exactly as + `MLIRGenClasses.cpp:1590-1597` does, instead of the offset placeholder. +- In `mlirGenCreateInterfaceVTableForObject`: if **all** interface methods were + emitted as symbols, `return globalVTableRefValue` unconditionally — the same + path method-less interfaces already take (line 258). The clone+patch block + (215-255) is skipped entirely; the vtable global becomes genuinely constant. + +The runtime-patch path **stays as fallback** for the two cases where the +constant is unknowable: + +| case | why | behavior | +|---|---|---| +| method with captures | field holds a per-instance closure pointer | keep per-object heap clone + patch (current, and genuinely required — a shared global would be wrong here) | +| cast of an *imported* object-typed value | type reconstructed from `@dllimport` declaration text (`mlirGenImportSharedLib`, `MLIRGenModule.cpp`); no local `funcOp`, side-table lookup misses | keep runtime patch — it loads the pointer out of the object itself, which is position-independent and works cross-module today | + +Semantic note to accept consciously: the constant vtable snapshots the method +at **compile time**, the current patch snapshots at **cast time**. They differ +only if a method-typed field is reassigned between object creation and the +cast — aligning with class semantics (where the vtable is always compile-time) +seems right, but it is a decision, not an accident. + +Wins: casts become O(1) with no heap allocation (less GC pressure, less code); +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. + +## 4. Change 2: a single writer for `virtualIndex` + +Slot numbering is currently embodied in four places: + +1. registration-time assignment via `getNextVTableMemberIndex()` — raw + interleaved declaration order, i.e. the **wrong** layout + (`mlirGenInterfaceAddFieldMember`, `MLIRGenInterfaces.cpp:620`, and + `addInterfaceMethod`); since #252 it is dead weight, immediately + overwritten; +2. `getVirtualTable()` — re-assigns `virtualIndex` as a *side effect* of + building a vtable for one particular cast target + (`MLIRGenStore.h:346,356,375,385,404,414`); +3. `getVTableSize()` — implicit count whose contract is documented only by the + comment *"as I remember methods are first in interfaces"* + (`MLIRGenStore.h:538`); +4. `assignCanonicalVirtualIndexes()` — the canonical pass added by #252. + +Plan: make (4) the only writer. Registration initializes `virtualIndex = -1`; +`getVirtualTable()` becomes read-only with respect to member infos (it builds +its rows by iterating the same canonical order — factor a shared +`forEachVTableSlot(callback)` used by both `assignCanonicalVirtualIndexes` and +`getVirtualTable` so the two can never diverge again); `getVTableSize` derives +from the same helper and the narrative comment goes away. An assert that the +canonical pass ran (any member with `-1` outside the optional-missing case) +catches ordering mistakes early. + +### Implementation notes (PR 1, as landed) + +The plan above undersold the blast radius by one layer: `getVirtualTable()`'s +per-cast mutation wasn't only correcting for "no cast happened yet" (§4's +framing) - for `interface D extends A, B { ... }`, it was also the *only* +mechanism computing each BASE interface's slot position **within the derived +interface's combined vtable**. `A`'s fields have one `virtualIndex` when `A` +is used standalone (0, 1, ...) and a *different* one when accessed through +`D` (offset by however many slots `D`'s other extends-parents contribute +first) - a single mutable field on the shared `InterfaceFieldInfo` cannot +hold both, and the old code "solved" it by last-writer-wins mutation, correct +only for whichever root interface was cast most recently (`00interface_object4.ts`, +`00interface_conjunction.ts`'s `t2 extends F1, F2` both regressed on the first +version of this change until this was accounted for). + +Fix actually shipped: `findField`/`findMethod` (MLIRGenStore.h) grew an +offset-accumulating overload - `findField(id, int &vtableOffset)` - that walks +the `extends` chain and sums each hop's `recalcOffsets()`-computed +`std::get<0>(extent)` (the base interface's slot-block start within *its* +parent) as it unwinds the recursion. `InterfaceFieldAccess`/ +`InterfaceMethodAccess` (MLIRGenImpl.h) gained a `vtableOffset` parameter, +added to the member's own (now genuinely standalone-canonical) `virtualIndex`. +Every call site that resolves a field/method for an actual access +(`InterfaceMembers`'s dispatcher, plus the get/set method lookups inside +`InterfaceAccessorAccess` and `InterfaceIndexAccess`) was updated to capture +and pass the offset. Call sites that only need the field/method for a type +check (`castInterfaceToTuple`, safe-cast narrowing, etc.) keep using the +plain no-offset overload - they never build an `InterfaceSymbolRefOp`. + +Second, unrelated bug surfaced by the same removal: `mergeInterfaces()` +(MLIRGenTypes.cpp, used only by intersection-type synthesis - `type t = A & B +& { c: number }`) had a pre-existing mismatched aggregate initializer - +`{id, type, isConditional, getNextVTableMemberIndex()}` against +`InterfaceFieldInfo{id, type, isConditional, interfacePosIndex, virtualIndex}`, +silently landing the computed index in `interfacePosIndex` (read only for +methods, never for fields, so inert) and leaving `virtualIndex` +zero-initialized. Masked for years by the same getVirtualTable() mutation. +Fixed as part of this PR; the intersection-type synthesis path +(`getIntersectionType`, MLIRGenTypes.cpp) also needed its own +`assignCanonicalVirtualIndexes()` call - it builds `InterfaceInfo` directly +rather than through `mlirGen(InterfaceDeclaration)`'s AST walk, so it never +picked up §4's fix at its original call site. + +Net: two pre-existing bugs (both papered over by the mutation this PR +removes) had to be fixed to keep the regression suite green, beyond what §4's +original text anticipated. Regression coverage: +`00interface_optional_cast_order.ts` (§5), plus the pre-existing +`00interface_object4.ts` (single-level `extends`) and +`00interface_conjunction.ts` (intersection types, both the `interface t2 +extends F1, F2` and `type t = F1 & F2 & {...}` forms) now exercise the +extends-offset path that had none before. + +## 5. Change 3: fix the latent optional-member cast-order miscompile + +`getVirtualTable()` writes `virtualIndex = -1` into the **shared** +`InterfaceInfo` whenever the particular cast target lacks an optional member +(`MLIRGenStore.h:346,375,404`). `InterfaceFieldAccess` then branches on that at +*compile time* and emits `OptionalUndefOp` (`MLIRGenImpl.h:5507-5520`). +Sequence that miscompiles today (untested, by inspection): + +```ts +interface I { a: number; m?: number; } +let x: I = { a: 1 }; // cast target lacks m -> m.virtualIndex = -1 +let y: I = { a: 2, m: 5 }; +print(y.m); // compiled AFTER the x cast: sees -1, + // emits OptionalUndef -> undefined, silently +``` + +Same disease family as #252 (per-cast mutation of interface-wide state), just +silent instead of crashing. The robust mechanism already exists and is fully +sufficient: the *slot value* is `-1` for a missing member +(`MLIRGenInterfaces.cpp:348-351`) and the lowering checks it at runtime +(`LowerToLLVM.cpp:4907-4927`). Fix falls out of §4: stop writing `-1` to +`InterfaceInfo`; `InterfaceFieldAccess` keys the optional-typed load off +`isConditional` alone and always emits the `InterfaceSymbolRefOp`. Write the +regression test *first* (e.g. `00interface_optional_cast_order.ts`) to confirm +the analysis before changing behavior. + +## 6. Non-goals / rejected + +- **Offsets for method slots** (unify with fields, dereference at access): + rejected — see §1's uniformity constraint; class-backed values share the + access site. +- **Patch-once-into-a-shared-global** (memoized runtime patch): superseded by + §3, which gets a stronger result (true constant) for the same cases; the + captures case can't use a shared global anyway (per-instance pointers). +- **Deduplicating diamond-extends slots**: `Point3d extends Point, Point` + currently yields a `{x,y,x,y,z}` vtable (observed in `--emit=mlir`). + Harmless — layout is deterministic on both sides and `findField` resolves to + the first copy — but wasteful. Deferred: it changes the vtable ABI, so it + must ship with cross-module tests, and the payoff is small. + +## 7. Implementation order + +1. **PR 1 (correctness, small)**: §5 regression test, then §4 single-writer + refactor which removes the compile-time `-1` path. Pure `MLIRGenStore.h` / + `MLIRGenInterfaces.cpp` change, no lowering changes. +2. **PR 2 (simplification, medium)**: §3 side table + symbol-emitting builder + + collapse of the patch path to the fallback cases. Expect a net-negative + diff. +3. **PR 3 (optional)**: revisit whether the #251 `NewOp` is still needed on + the fallback path (it is, whenever a captures-bearing literal is cast + inside a global initializer — keep unless proven otherwise). + +Test matrix per PR: full suite (714 at time of writing); the +`export-import-object-literal-with-interface` pair; a **new** cross-module test +casting an *imported object-typed value* to an interface in the importer +(exercises the §3 fallback — currently uncovered); a captures-bearing literal +cast to an interface (check whether covered; add if not); the §5 cast-order +test. + +Known risks: two-pass compilation (`Stages::Discovering`) — the side table +must be populated consistently in whichever pass builds the vtable initializer; +and `@dllimport` type reconstruction must keep *missing* from the side table +(never a stale hit) so imported types deterministically take the fallback. diff --git a/tslang/include/TypeScript/MLIRLogic/MLIRGenStore.h b/tslang/include/TypeScript/MLIRLogic/MLIRGenStore.h index a52dd9170..92f0f0997 100644 --- a/tslang/include/TypeScript/MLIRLogic/MLIRGenStore.h +++ b/tslang/include/TypeScript/MLIRLogic/MLIRGenStore.h @@ -310,6 +310,18 @@ struct InterfaceInfo return mlir::success(); } + // builds the vtable CONTENTS (constant function-pointer symbols, GEP-on-null field + // offsets, or an explicit -1 sentinel for an optional member the specific cast target + // doesn't provide) for ONE implementer of this interface. Does NOT assign virtualIndex - + // slot numbering is a pure function of the interface's own declaration + // (assignCanonicalVirtualIndexes() is the sole writer) and must stay identical across + // every implementer, including ones this method is never called for (e.g. a module that + // only reads an already-typed interface value it imported never calls this at all - see + // docs/interface-vtable-simplification-design.md §4). Earlier versions of this method + // wrote virtualIndex here as a side effect of resolving ONE implementer, which corrupted + // the shared InterfaceInfo for every OTHER implementer's already-compiled or + // yet-to-compile field/method access sites whenever presence of an optional member + // differed between casts (§5) - do not reintroduce that. mlir::LogicalResult getVirtualTable( llvm::SmallVector &vtable, std::function(mlir::Attribute, mlir::Type, bool)> resolveField, @@ -343,7 +355,6 @@ struct InterfaceInfo MethodInfo missingMethod; missingMethod.name = method.name; missingMethod.funcType = method.funcType; - method.virtualIndex = -1; vtable.push_back({missingMethod, true}); } else @@ -353,7 +364,6 @@ struct InterfaceInfo } else { - method.virtualIndex = vtable.size(); vtable.push_back({fieldInfo}); } } @@ -372,7 +382,6 @@ struct InterfaceInfo MethodInfo missingMethod; missingMethod.name = method.name; missingMethod.funcType = method.funcType; - method.virtualIndex = -1; vtable.push_back({missingMethod, true}); } else @@ -382,7 +391,6 @@ struct InterfaceInfo } else { - method.virtualIndex = vtable.size(); vtable.push_back({classMethodInfo}); } } @@ -401,7 +409,6 @@ struct InterfaceInfo if (field.isConditional) { mlir_ts::FieldInfo missingField{field.id, field.type, false, mlir_ts::AccessLevel::Public}; - field.virtualIndex = -1; vtable.push_back({missingField, true}); } else @@ -411,7 +418,6 @@ struct InterfaceInfo } else { - field.virtualIndex = vtable.size(); vtable.push_back({fieldInfo}); } } @@ -443,8 +449,20 @@ struct InterfaceInfo return (signed)dist >= (signed)accessors.size() ? -1 : dist; } - InterfaceFieldInfo *findField(mlir::Attribute id) + // vtableOffset accumulates the position, within the vtable of the interface this call + // started on (the "root" - what the access site's InterfaceType actually is), where the + // DECLARING interface's own slots begin. A field/method's own virtualIndex + // (assignCanonicalVirtualIndexes()) is only correct standalone - t2 extends F1, F2 + // means F2's fields sit at vtableOffset = F1's slot count within t2's combined vtable, + // not at F2's own standalone index 0. The access site must add the two together + // (see InterfaceFieldAccess/InterfaceMethodAccess). Earlier code baked this offset + // directly into the shared InterfaceFieldInfo/InterfaceMethodInfo via a getVirtualTable() + // side effect keyed to whichever cast ran most recently - correct only by accident for + // whichever root interface was cast last; see docs/interface-vtable-simplification-design.md. + InterfaceFieldInfo *findField(mlir::Attribute id, int &vtableOffset) { + vtableOffset = 0; + auto index = getFieldIndex(id); if (index >= 0) { @@ -453,9 +471,9 @@ struct InterfaceInfo for (auto &extent : extends) { - auto field = std::get<1>(extent)->findField(id); - if (field) + if (auto *field = std::get<1>(extent)->findField(id, vtableOffset)) { + vtableOffset += std::get<0>(extent); return field; } } @@ -466,8 +484,16 @@ struct InterfaceInfo return nullptr; } - InterfaceMethodInfo *findMethod(mlir::StringRef name) + InterfaceFieldInfo *findField(mlir::Attribute id) + { + int vtableOffset; + return findField(id, vtableOffset); + } + + InterfaceMethodInfo *findMethod(mlir::StringRef name, int &vtableOffset) { + vtableOffset = 0; + auto index = getMethodIndex(name); if (index >= 0) { @@ -476,8 +502,9 @@ struct InterfaceInfo for (auto &extent : extends) { - if (auto *method = std::get<1>(extent)->findMethod(name)) + if (auto *method = std::get<1>(extent)->findMethod(name, vtableOffset)) { + vtableOffset += std::get<0>(extent); return method; } } @@ -485,6 +512,12 @@ struct InterfaceInfo return nullptr; } + InterfaceMethodInfo *findMethod(mlir::StringRef name) + { + int vtableOffset; + return findMethod(name, vtableOffset); + } + InterfaceAccessorInfo *findAccessor(mlir::StringRef name) { auto index = getAccessorIndex(name); diff --git a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp index 9b9968bf2..101f00104 100644 --- a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp +++ b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp @@ -788,18 +788,20 @@ namespace mlirgen return InterfaceIndexAccess(interfaceInfo, location, interfaceValue, argument, genContext); } - // check field access - if (auto fieldInfo = interfaceInfo->findField(id)) + // check field access + int fieldVTableOffset; + if (auto fieldInfo = interfaceInfo->findField(id, fieldVTableOffset)) { - return InterfaceFieldAccess(location, interfaceValue, fieldInfo); + return InterfaceFieldAccess(location, interfaceValue, fieldInfo, fieldVTableOffset); } // check method access if (nameAttr) { - if (auto methodInfo = interfaceInfo->findMethod(nameAttr.getValue())) + int methodVTableOffset; + if (auto methodInfo = interfaceInfo->findMethod(nameAttr.getValue(), methodVTableOffset)) { - return InterfaceMethodAccess(location, interfaceValue, methodInfo); + return InterfaceMethodAccess(location, interfaceValue, methodInfo, methodVTableOffset); } if (auto accessorInfo = interfaceInfo->findAccessor(nameAttr.getValue())) diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index 331aacafe..176466941 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -5501,26 +5501,28 @@ class MLIRGenImpl mlir::Value InterfaceMembers(mlir::Location location, mlir::Value interfaceValue, mlir::StringRef interfaceFullName, mlir::Attribute id, mlir::Value argument, const GenContext &genContext); - mlir::Value InterfaceFieldAccess(mlir::Location location, mlir::Value interfaceValue, InterfaceFieldInfo *fieldInfo) + // vtableOffset is the declaring interface's slot-block position within the root + // interface's combined vtable (0 unless fieldInfo was found through an `extends` chain + // - see InterfaceInfo::findField's doc comment, MLIRGenStore.h). + mlir::Value InterfaceFieldAccess(mlir::Location location, mlir::Value interfaceValue, InterfaceFieldInfo *fieldInfo, int vtableOffset = 0) { auto fieldRefType = mlir_ts::RefType::get(fieldInfo->type); - if (fieldInfo->virtualIndex == -1) - { - // no data for conditional interface; - if (!fieldInfo->isConditional) - { - emitError(location, "field '") << fieldInfo->id << "' is not conditional and missing"; - return mlir::Value(); - } - - auto actualType = isa(fieldRefType.getElementType()) - ? fieldRefType.getElementType() - : mlir_ts::OptionalType::get(fieldRefType.getElementType()); - return builder.create(location, actualType); - } + // fieldInfo->virtualIndex is assigned once, canonically, by + // InterfaceInfo::assignCanonicalVirtualIndexes() when the interface declaration + // resolves - it is never -1 here. Whether THIS PARTICULAR interface value's + // underlying object actually provides an optional member is a runtime property (it + // can differ between implementers of the same interface), not something this call + // site can know at compile time - InterfaceSymbolRefOpLowering's isOptional branch + // (LowerToLLVM.cpp) already checks the loaded slot against the -1 sentinel and + // produces OptionalUndef at runtime when appropriate. An earlier version of this + // function special-cased virtualIndex == -1 here to bypass the runtime read - that + // relied on InterfaceInfo::getVirtualTable() mutating the SHARED virtualIndex to -1 + // as a side effect of whichever cast last happened to be missing this member, which + // corrupted access sites for OTHER, unrelated implementers that do provide it. See + // docs/interface-vtable-simplification-design.md §5. assert(fieldInfo->virtualIndex >= 0); - auto vtableIndex = fieldInfo->virtualIndex; + auto vtableIndex = vtableOffset + fieldInfo->virtualIndex; auto interfaceSymbolRefValue = builder.create( location, fieldRefType, interfaceValue, builder.getI32IntegerAttr(vtableIndex), @@ -5552,10 +5554,11 @@ class MLIRGenImpl return value; } - mlir::Value InterfaceMethodAccess(mlir::Location location, mlir::Value interfaceValue, InterfaceMethodInfo *methodInfo) + // see InterfaceFieldAccess's vtableOffset doc comment above. + mlir::Value InterfaceMethodAccess(mlir::Location location, mlir::Value interfaceValue, InterfaceMethodInfo *methodInfo, int vtableOffset = 0) { assert(methodInfo->virtualIndex >= 0); - auto vtableIndex = methodInfo->virtualIndex; + auto vtableIndex = vtableOffset + methodInfo->virtualIndex; auto effectiveFuncType = getBoundFunctionType(methodInfo->funcType); @@ -5575,9 +5578,10 @@ class MLIRGenImpl mlir::Value setMethodInfoValue; if (!accessorInfo->getMethod.empty()) { - if (auto getMethodInfo = interfaceInfo->findMethod(accessorInfo->getMethod)) + int vtableOffset; + if (auto getMethodInfo = interfaceInfo->findMethod(accessorInfo->getMethod, vtableOffset)) { - getMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, getMethodInfo); + getMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, getMethodInfo, vtableOffset); } else { @@ -5592,9 +5596,10 @@ class MLIRGenImpl if (!accessorInfo->setMethod.empty()) { - if (auto setMethodInfo = interfaceInfo->findMethod(accessorInfo->setMethod)) + int vtableOffset; + if (auto setMethodInfo = interfaceInfo->findMethod(accessorInfo->setMethod, vtableOffset)) { - setMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, setMethodInfo); + setMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, setMethodInfo, vtableOffset); } else { @@ -5644,9 +5649,10 @@ class MLIRGenImpl mlir::Value setMethodInfoValue; if (!indexInfo->getMethod.empty()) { - if (auto getMethodInfo = interfaceInfo->findMethod(indexInfo->getMethod)) + int vtableOffset; + if (auto getMethodInfo = interfaceInfo->findMethod(indexInfo->getMethod, vtableOffset)) { - getMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, getMethodInfo); + getMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, getMethodInfo, vtableOffset); } else { @@ -5661,9 +5667,10 @@ class MLIRGenImpl if (!indexInfo->setMethod.empty()) { - if (auto setMethodInfo = interfaceInfo->findMethod(indexInfo->setMethod)) + int vtableOffset; + if (auto setMethodInfo = interfaceInfo->findMethod(indexInfo->setMethod, vtableOffset)) { - setMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, setMethodInfo); + setMethodInfoValue = InterfaceMethodAccess(location, interfaceValue, setMethodInfo, vtableOffset); } else { diff --git a/tslang/lib/TypeScript/MLIRGenTypes.cpp b/tslang/lib/TypeScript/MLIRGenTypes.cpp index 3b3e077bd..5c99a21ff 100644 --- a/tslang/lib/TypeScript/MLIRGenTypes.cpp +++ b/tslang/lib/TypeScript/MLIRGenTypes.cpp @@ -3048,6 +3048,13 @@ namespace mlirgen newInterfaceInfo->recalcOffsets(); + // canonical (extends, then own methods, then own fields) slot numbering for the + // synthesized interface's OWN members - see InterfaceInfo::assignCanonicalVirtualIndexes + // (MLIRGenStore.h) and mlirGen(InterfaceDeclaration)'s equivalent call + // (MLIRGenInterfaces.cpp), which this construction path (intersection types) bypasses + // entirely, being a separate programmatic InterfaceInfo builder rather than an AST walk. + newInterfaceInfo->assignCanonicalVirtualIndexes(); + return newInterfaceInfo->interfaceType; } @@ -3323,7 +3330,14 @@ namespace mlirgen // TODO: use it to merge with TupleType for (auto &item : src.getFields()) { - dest->fields.push_back({item.id, item.type, item.isConditional || conditional, dest->getNextVTableMemberIndex()}); + // InterfaceFieldInfo is {id, type, isConditional, interfacePosIndex, virtualIndex} - the + // getNextVTableMemberIndex() value belongs in virtualIndex, not interfacePosIndex (a + // separate, unrelated field only meaningful for methods - MLIRGenClasses.cpp). This + // mismatch previously left virtualIndex zero-initialized for every field merged in from + // an intersection type's own `{ ... }` member (e.g. `F1 & F2 & { c: number }`'s `c`), + // masked only because getVirtualTable() used to overwrite virtualIndex as a side effect + // of every cast - see docs/interface-vtable-simplification-design.md §4. + dest->fields.push_back({item.id, item.type, item.isConditional || conditional, 0, dest->getNextVTableMemberIndex()}); } return mlir::success(); diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index e98769902..5f7fa107e 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -284,6 +284,7 @@ add_test(NAME test-compile-00-interface-global-method COMMAND test-runner "${PRO add_test(NAME test-compile-00-interface-conjunction COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_conjunction.ts") 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-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") @@ -634,6 +635,7 @@ add_test(NAME test-jit-00-interface-global-method COMMAND test-runner -jit "${PR add_test(NAME test-jit-00-interface-conjunction COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_conjunction.ts") 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-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") diff --git a/tslang/test/tester/tests/00interface_optional_cast_order.ts b/tslang/test/tester/tests/00interface_optional_cast_order.ts new file mode 100644 index 000000000..895f4cb6b --- /dev/null +++ b/tslang/test/tester/tests/00interface_optional_cast_order.ts @@ -0,0 +1,33 @@ +// regression test: InterfaceInfo::getVirtualTable() (MLIRGenStore.h) marks an +// optional interface member's virtualIndex as -1 on the SHARED InterfaceInfo +// whenever it builds a vtable for an object that doesn't provide that member +// -- a side effect of resolving that ONE specific cast target, not a property +// of the interface itself. InterfaceFieldAccess (MLIRGenImpl.h) then reads +// that shared, mutable virtualIndex at COMPILE TIME: if it is -1 it bakes in +// OptionalUndefOp directly, bypassing the runtime InterfaceSymbolRefOp read +// entirely (InterfaceSymbolRefOpLowering already has a correct per-object +// runtime slot==-1 check -- LowerToLLVM.cpp -- but this compile-time shortcut +// never reaches it). +// +// So: cast an object that DOES provide the optional member, then cast a +// DIFFERENT object of the same interface that does NOT provide it, then +// access the member on the FIRST (providing) object -- the access is +// compiled after the second cast clobbered the shared virtualIndex to -1, +// so it wrongly resolves to "undefined" instead of reading the real value. +// See docs/interface-vtable-simplification-design.md §5. + +interface Box { + a: number; + m?: number; +} + +const present: Box = { a: 2, m: 5 }; +const missing: Box = { a: 1 }; + +function main() { + assert(missing.m == undefined); + print(present.m); + assert(present.m == 5); + + print("done."); +}