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."); }