From 7bb4fd5762d883de47e43c45942f4ae7b8bfa040 Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Thu, 23 Jul 2026 12:59:43 +0100 Subject: [PATCH] Fix accessor vtable dispatch through base-typed references Closes the "much bigger gap" flagged alongside super-accessor-crash-fix (PR #278): overriding a get/set accessor pair in a subclass and accessing it through a BASE-TYPED reference (an ordinary upcast, not `super`) always resolved statically to the base's own accessor instead of dispatching virtually to the override - a real correctness gap (this is standard OOP virtual dispatch, not an edge case), unlike ordinary methods which already dispatch through the vtable correctly. Root cause: an accessor's backing get/set functions are ALREADY registered as ordinary MethodInfo entries in ClassInfo::methods (same list ordinary methods use) with a real vtable slot assigned via registerClassMethodMember/getVirtualTable - accessors are virtual by default exactly like methods. But ClassAccessorAccess never checked isVirtual at all: it unconditionally built ThisAccessorOp with a hardcoded FlatSymbolRefAttr naming THIS class's own get/set symbol, completely bypassing the vtable that was already being correctly built. Fix: mirror ClassMethodAccess's own isVirtual/isStorageType check (isStorageType is true for `super.`'s raw-struct thisValue, so super access is naturally excluded from virtual dispatch, same as methods) and route through the vtable via VirtualSymbolRefOp (the same op static virtual method access already uses) feeding into the existing "indirect" (function-value) ThisIndirectAccessorOp, instead of the hardcoded-symbol ThisAccessorOp. Checked before the isDynamicImport branch, matching ClassMethodAccess's ordering - the vtable's own construction already resolves dynamic-import-owned slots, so virtual dispatch through it is correct regardless of module boundaries. One regression found and fixed during verification: getThisRefOfClass only narrows `thisValue` for the isSuperClass case, so an ordinary access whose static type is still a wider union (e.g. `T | null` narrowed only at the type-info level, never cast at the value level - hit by 00iterator_bug.ts's spread-operator-generated `.length` access) reached the new vtable lookup with an un-narrowed union type, which mlirGenPropertyAccessExpression's PropertyRefOp rejects (unlike the old AnyType-typed ThisAccessorOp it replaces). Fixed by explicitly casting to classInfo->classType before the vtable lookup. New same-file regression test 00class_accessor_virtual.ts (compile + jit tiers). Also closed the historical NOTE in import_class_accessor.ts documenting this exact gap as deliberately untested - now asserts virtual dispatch through a base-typed reference works, cross-module, across all 3 tiers (plain, -shared compile, -jit -shared). Full suite: 810/810 green. Co-Authored-By: Claude Sonnet 5 --- tslang/lib/TypeScript/MLIRGenAccessCall.cpp | 94 +++++++++++++++++++ tslang/test/tester/CMakeLists.txt | 2 + .../tester/tests/00class_accessor_virtual.ts | 39 ++++++++ .../tester/tests/import_class_accessor.ts | 17 ++-- 4 files changed, 146 insertions(+), 6 deletions(-) create mode 100644 tslang/test/tester/tests/00class_accessor_virtual.ts diff --git a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp index 2e363b982..a52b037ed 100644 --- a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp +++ b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp @@ -592,6 +592,100 @@ namespace mlirgen { auto effectiveThisValue = getThisRefOfClass(location, classInfo->classType, thisValue, isSuperClass, genContext); + // Accessor vtable dispatch (see accessor-vtable-dispatch-fix memory): overriding + // a get/set pair in a subclass and accessing it through a BASE-TYPED reference + // (an ordinary upcast, not `super`) used to always resolve statically to the + // base's own accessor - accessors were never part of the vtable dispatch + // decision at all, even though their backing get/set functions are ALREADY + // registered as ordinary (virtual-by-default) MethodInfo entries with real + // vtable slots via registerClassMethodMember/getVirtualTable, exactly like + // regular methods. Mirror ClassMethodAccess's own isVirtual/isStorageType check + // (isStorageType is true for `super.`'s raw-struct thisValue, so this + // naturally excludes super access from virtual dispatch, same as methods) and + // route through the vtable via VirtualSymbolRefOp + the existing "indirect" + // (function-value) ThisIndirectAccessorOp, instead of hardcoding this class's + // own get/set symbol names. Checked BEFORE the isDynamicImport branch below, + // same ordering as ClassMethodAccess - the vtable's own construction already + // resolves dynamic-import-owned slots (mlirGenClassVirtualTableDefinition's + // isOwnedByDynamicImport handling), so virtual dispatch through it is correct + // regardless of module boundaries, without needing separate dlsym resolution. + auto isStorageType = isa(thisValue.getType()); + + auto findMethodInfo = [&](FunctionEntry funcEntry) -> const MethodInfo * { + if (!funcEntry) + { + return nullptr; + } + + auto it = llvm::find_if(classInfo->methods, + [&](auto &m) { return m.funcName == funcEntry.name; }); + return it != classInfo->methods.end() ? &*it : nullptr; + }; + + auto getMethodInfo = findMethodInfo(getFunc); + auto setMethodInfo = findMethodInfo(setFunc); + + auto isVirtualAccessor = !isStorageType && + ((getMethodInfo && (getMethodInfo->isAbstract || getMethodInfo->isVirtual)) || + (setMethodInfo && (setMethodInfo->isAbstract || setMethodInfo->isVirtual))); + + if (isVirtualAccessor) + { + // effectiveThisValue can still carry a wider static type here (e.g. a + // union like `T | null` that the caller narrowed at the type-info level to + // find classInfo, but never actually cast the VALUE to) - getThisRefOfClass + // only narrows for the isSuperClass branch, a no-op otherwise. + // mlirGenPropertyAccessExpression's vtable lookup needs a genuine class + // reference, unlike the plain-AnyType ThisAccessorOp this branch replaces, + // so cast explicitly first (same narrowing getThisRefOfClass already does + // for super). + CAST_A(vtableThisValue, location, classInfo->classType, effectiveThisValue, genContext); + + auto vtableAccess = + mlirGenPropertyAccessExpression(location, vtableThisValue, VTABLE_NAME, genContext); + + if (!vtableAccess) + { + emitError(location, "") << "class '" << classInfo->fullName << "' missing 'virtual table'"; + } + + EXIT_IF_FAILED_OR_NO_VALUE(vtableAccess) + + auto resolveVirtualAccessorFunc = [&](FunctionEntry funcEntry, const MethodInfo *methodInfo) -> mlir::Value { + if (!funcEntry) + { + return mlir::Value(); + } + + assert(genContext.allowPartialResolve || methodInfo->virtualIndex >= 0); + + return builder.create( + location, funcEntry.funcType, vtableAccess, + builder.getI32IntegerAttr(methodInfo->virtualIndex), + mlir::FlatSymbolRefAttr::get(builder.getContext(), funcEntry.name)); + }; + + auto getterValue = resolveVirtualAccessorFunc(getFunc, getMethodInfo); + auto setterValue = resolveVirtualAccessorFunc(setFunc, setMethodInfo); + + auto opaqueThisType = mlir_ts::OpaqueType::get(builder.getContext()); + if (!getterValue) + { + getterValue = builder.create( + location, mlir_ts::FunctionType::get(builder.getContext(), {opaqueThisType}, {accessorResultType}, false)); + } + + if (!setterValue) + { + setterValue = builder.create( + location, mlir_ts::FunctionType::get(builder.getContext(), {opaqueThisType, accessorResultType}, {}, false)); + } + + auto thisIndirectAccessorOp = builder.create( + location, accessorResultType, vtableThisValue, getterValue, setterValue, mlir::Value()); + return thisIndirectAccessorOp.getResult(0); + } + if (classInfo->isDynamicImport) { // A plain FlatSymbolRefAttr (the path below) lowers to a call the static linker diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 0475c8849..3155ba950 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -236,6 +236,7 @@ add_test(NAME test-compile-00-class-discover-types COMMAND test-runner "${PROJEC add_test(NAME test-compile-00-class-accessor COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor.ts") add_test(NAME test-compile-00-class-accessor-2 COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor2.ts") add_test(NAME test-compile-00-class-accessor-super COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor_super.ts") +add_test(NAME test-compile-00-class-accessor-virtual COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor_virtual.ts") add_test(NAME test-compile-00-class-super COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_super.ts") add_test(NAME test-compile-00-class-super-static COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_super_static.ts") add_test(NAME test-compile-00-class-default-constructor COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_def_constr.ts") @@ -599,6 +600,7 @@ add_test(NAME test-jit-00-class-discover-types COMMAND test-runner -jit "${PROJE add_test(NAME test-jit-00-class-accessor COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor.ts") add_test(NAME test-jit-00-class-accessor-2 COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor2.ts") add_test(NAME test-jit-00-class-accessor-super COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor_super.ts") +add_test(NAME test-jit-00-class-accessor-virtual COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_accessor_virtual.ts") add_test(NAME test-jit-00-class-super COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_super.ts") add_test(NAME test-jit-00-class-super-static COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_super_static.ts") add_test(NAME test-jit-00-class-default-constructor COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00class_def_constr.ts") diff --git a/tslang/test/tester/tests/00class_accessor_virtual.ts b/tslang/test/tester/tests/00class_accessor_virtual.ts new file mode 100644 index 000000000..c7f65e77f --- /dev/null +++ b/tslang/test/tester/tests/00class_accessor_virtual.ts @@ -0,0 +1,39 @@ +// Regression test for the accessor-vtable-dispatch-fix: overriding a +// get/set accessor pair in a subclass and accessing it through a +// BASE-TYPED reference (an ordinary upcast, not `super`) used to always +// resolve statically to the base's own accessor - accessors were never +// part of the vtable dispatch decision at all, unlike ordinary methods. +class Base { + protected _val: number = 10; + + get val(): number { + return this._val; + } + + set val(v: number) { + this._val = v; + } +} + +class Derived extends Base { + get val(): number { + return this._val * 2; + } + + set val(v: number) { + this._val = v + 1; + } +} + +function main() { + const d = new Derived(); + const asBase: Base = d; + + asBase.val = 5; + assert(asBase.val == 12); + + // and through the derived-typed reference directly, for good measure + assert(d.val == 12); + + print("done."); +} diff --git a/tslang/test/tester/tests/import_class_accessor.ts b/tslang/test/tester/tests/import_class_accessor.ts index de33365da..dced8af1b 100644 --- a/tslang/test/tester/tests/import_class_accessor.ts +++ b/tslang/test/tester/tests/import_class_accessor.ts @@ -20,12 +20,17 @@ function main() { assert(t.celsius == 100); assert(t.fahrenheit == 212); - // NOTE: accessing an overridden accessor through a base-typed reference - // (`const asBase: M.Temperature = t; asBase.celsius`) is a known, - // pre-existing, non-cross-module-specific gap: get/set accessors are not - // part of the vtable, so such access resolves statically to the base's - // own accessor rather than dispatching virtually. Not exercised here - - // this test only covers the super-accessor-call crash fix. + // Accessing an overridden accessor through a base-typed reference used to + // be a known, pre-existing, non-cross-module-specific gap: get/set + // accessors were not part of the vtable, so such access resolved + // statically to the base's own accessor rather than dispatching + // virtually (see accessor-vtable-dispatch-fix memory). Fixed - this now + // dispatches to ClampedTemperature's override even through the + // M.Temperature-typed reference, cross-module. + const asBase: M.Temperature = t; + asBase.celsius = -5; + assert(asBase.celsius == 0); + assert(asBase.fahrenheit == 32); print("done."); }