From 012d72582226f23bce5143984463639064fc202e Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Thu, 23 Jul 2026 15:48:03 +0100 Subject: [PATCH] Fix cross-module abstract-class vtable slot shift (printer dropped `abstract`) DeclarationPrinter never printed the `abstract` modifier (class- or method-level) when embedding a class declaration into a compiled binary's __decls. The reimporting module re-parsed the declaration as a CONCRETE class, so mlirGenClassNew - which skips `.new` synthesis for abstract classes - generated a `.new` vtable slot the exporting module never had. Every base-class slot after `.instanceOf` shifted by one, and the exporting module's compiled method bodies (with slot indexes baked in) dispatched `this.()` into the wrong slot: a pointer-returning `.new` called through a double-returning signature, "returning" stale XMM0. That stale-register garbage is why this hid so well: a directly preceding `assert(s.area() == 9)` left 9.0 in XMM0, so direct and base-class-cast calls passed by luck while an interface-path call got 0 - which is why the disabled test's comment misattributed this to an interface `this`-binding gap, and why import_class_abstract (2-level chain) appeared to pass. Fix: print `abstract ` before `class` (layout-relevant: controls `.new` synthesis) and before abstract method signatures (semantic: the importer must not treat the exporter's abstract methods as linkable symbols). Also: re-enable both disabled *-class-implements-interface-abstract tests, add a deterministic single-level regression pair (*_class_abstract_virtual_dispatch.ts - describe() twice before any area() call so register luck can't mask a regression), and add an opt-in TSLANG_TEST_KEEP_TEMP env var to test-runner so failing tests' artifacts survive for inspection. 816/816 tests green. Co-Authored-By: Claude Fable 5 --- tslang/lib/TypeScript/DeclarationPrinter.cpp | 16 +++++++++ tslang/test/tester/CMakeLists.txt | 35 ++++++++++--------- tslang/test/tester/test-runner.cpp | 2 +- .../export_class_abstract_virtual_dispatch.ts | 24 +++++++++++++ .../import_class_abstract_virtual_dispatch.ts | 29 +++++++++++++++ 5 files changed, 89 insertions(+), 17 deletions(-) create mode 100644 tslang/test/tester/tests/export_class_abstract_virtual_dispatch.ts create mode 100644 tslang/test/tester/tests/import_class_abstract_virtual_dispatch.ts diff --git a/tslang/lib/TypeScript/DeclarationPrinter.cpp b/tslang/lib/TypeScript/DeclarationPrinter.cpp index e7571125f..813a2bab5 100644 --- a/tslang/lib/TypeScript/DeclarationPrinter.cpp +++ b/tslang/lib/TypeScript/DeclarationPrinter.cpp @@ -401,6 +401,17 @@ namespace typescript printNamespaceBegin(classType->elementNamespace); printBeforeDeclaration(); + // `abstract` is layout-relevant, not just semantic: mlirGenClassNew skips + // synthesizing the `.new` method for abstract classes, so dropping the + // modifier makes the reimporting module insert a `.new` vtable slot the + // exporting module never had, shifting every subsequent virtual index and + // corrupting cross-module virtual dispatch (base-module code calling + // this.method() lands on the wrong slot). + if (classType->isAbstract) + { + os << "abstract "; + } + os << "class " << classType->name; if (classType->baseClasses.size() > 0) @@ -554,6 +565,11 @@ namespace typescript os << "private "; } + if (method.isAbstract) + { + os << "abstract "; + } + printMethod( method.isStatic, method.name, diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 29db733d9..7e66e9cd0 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -946,20 +946,22 @@ add_test(NAME test-compile-shared-export-import-class-static COMMAND test-runner add_test(NAME test-compile-shared-export-import-class-implements-interface-multilevel COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_multilevel.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_multilevel.ts") add_test(NAME test-compile-shared-export-import-class-implements-interface-optional COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_optional.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_optional.ts") add_test(NAME test-compile-shared-export-import-class-structural-interface COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_structural_interface.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_structural_interface.ts") -# DISABLED: a separate, NOT fixed bug found 2026-07-22 while verifying the fix above. Unlike the -# multilevel/optional/structural variants (all pass now), this one calls describe() - a CONCRETE -# method inherited from the abstract dynamic-import base M.Shape, whose body internally calls the -# still-virtual this.area() - through an interface reference -# (`const asDescribable: M.Describable = s; asDescribable.describe()`). Confirmed via the real -# harness (instrumented with print instead of assert, see the session transcript): the DIRECT call -# `s.describe()` and the base-class-cast call `asShape.describe()` both return the correct -# "red area=9", but the SAME describe() reached through the interface returns "red area=0" - -# this.area() silently resolves to 0 instead of dispatching to Square's override, specifically -# when invoked via the interface's function-pointer slot. Root cause not yet isolated (likely a -# `this`-identity/binding gap specific to calling an inherited concrete method through an -# interface vtable slot, distinct from the link-time issue fixed above) - left for a dedicated -# follow-up. -# add_test(NAME test-compile-shared-export-import-class-implements-interface-abstract COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_abstract.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_abstract.ts") +# FIXED 2026-07-23 (was disabled): the "this.area()-returns-0-via-interface" symptom had nothing +# to do with interfaces or `this` binding. DeclarationPrinter dropped the `abstract` modifier when +# embedding the class declaration into the compiled binary (__decls), so the reimporting module +# treated M.Shape as concrete and synthesized a `.new` method for it - a vtable slot the exporting +# module never created (mlirGenClassNew skips `.new` for abstract classes). Every base-class slot +# after `.instanceOf` was thereby shifted by one in the importer's rebuilt vtable, and describe()'s +# compiled-in slot-1 read for area() landed on `.new` instead - a pointer-returning function called +# through a double-returning signature, so the "result" was whatever garbage was left in XMM0. +# That garbage is why the symptom looked interface-specific: a preceding `assert(s.area() == 9)` +# left 9.0 in XMM0, making the direct and base-class-cast calls "pass" by pure luck, while the +# interface path (more string machinery in between) got 0. See the abstract-virtual-dispatch tests +# below for the deterministic minimal regression pair. +add_test(NAME test-compile-shared-export-import-class-implements-interface-abstract COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_abstract.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_abstract.ts") +# minimal deterministic regression pair for the vtable-slot-shift bug above (single-level abstract, +# describe() called twice before any area() call so XMM0 luck can't hide a regression) +add_test(NAME test-compile-shared-export-import-class-abstract-virtual-dispatch COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_abstract_virtual_dispatch.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_abstract_virtual_dispatch.ts") # FIXED: see the matching test-compile-export-import-class-generic comment above (2026-07-22). add_test(NAME test-compile-shared-export-import-class-generic COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_generic.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_generic.ts") add_test(NAME test-compile-shared-export-import-function-generic COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_function_generic.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_function_generic.ts") @@ -1007,8 +1009,9 @@ add_test(NAME test-jit-shared-export-import-class-static COMMAND test-runner -ji add_test(NAME test-jit-shared-export-import-class-implements-interface-multilevel COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_multilevel.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_multilevel.ts") add_test(NAME test-jit-shared-export-import-class-implements-interface-optional COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_optional.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_optional.ts") add_test(NAME test-jit-shared-export-import-class-structural-interface COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_structural_interface.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_structural_interface.ts") -# DISABLED: see the matching test-compile-shared-export-import-class-implements-interface-abstract comment above (known issue, 2026-07-22, NOT fixed) - the JIT tier of the abstract variant hits the same this.area()-returns-0-via-interface bug. -# add_test(NAME test-jit-shared-export-import-class-implements-interface-abstract COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_abstract.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_abstract.ts") +# FIXED 2026-07-23 (was disabled): see the matching test-compile-shared-export-import-class-implements-interface-abstract comment above (DeclarationPrinter dropped `abstract`, shifting the reimported vtable's slots). +add_test(NAME test-jit-shared-export-import-class-implements-interface-abstract COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_implements_interface_abstract.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_implements_interface_abstract.ts") +add_test(NAME test-jit-shared-export-import-class-abstract-virtual-dispatch COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_abstract_virtual_dispatch.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_abstract_virtual_dispatch.ts") # FIXED: see the matching test-compile-export-import-class-generic comment above (2026-07-22). add_test(NAME test-jit-shared-export-import-class-generic COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_generic.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_generic.ts") add_test(NAME test-jit-shared-export-import-function-generic COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_function_generic.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_function_generic.ts") diff --git a/tslang/test/tester/test-runner.cpp b/tslang/test/tester/test-runner.cpp index a8b803793..9834d1faa 100644 --- a/tslang/test/tester/test-runner.cpp +++ b/tslang/test/tester/test-runner.cpp @@ -270,7 +270,7 @@ std::string checkOutputAndCleanup(std::string tempOutputFileNameNoExt) auto output = readOutput(txtFile); auto errors = readOutput(errFile); - deleteFiles(tempOutputFileNameNoExt); + if (!getenv("TSLANG_TEST_KEEP_TEMP")) deleteFiles(tempOutputFileNameNoExt); if (output.find("done.") != std::string::npos) { diff --git a/tslang/test/tester/tests/export_class_abstract_virtual_dispatch.ts b/tslang/test/tester/tests/export_class_abstract_virtual_dispatch.ts new file mode 100644 index 000000000..deb4942e0 --- /dev/null +++ b/tslang/test/tester/tests/export_class_abstract_virtual_dispatch.ts @@ -0,0 +1,24 @@ +namespace M { + + // Single-level abstract class whose CONCRETE method dispatches the + // still-abstract area() through `this` - the minimal shape of the + // cross-module vtable-slot-mismatch bug: DeclarationPrinter used to drop + // the `abstract` modifier when embedding this declaration into the + // compiled binary (__decls), so the reimporting module treated Shape as + // concrete and synthesized a `.new` vtable slot the exporting module + // never had (mlirGenClassNew skips it for abstract classes). That + // shifted every subsequent slot by one, and describe()'s baked-in + // slot-1 read for area() landed on `.new` instead - returning whatever + // garbage happened to sit in XMM0 (a pointer-returning function called + // through a double-returning signature). + + export abstract class Shape { + color: string = "red"; + + abstract area(): number; + + describe(): string { + return `${this.color} area=${this.area()}`; + } + } +} diff --git a/tslang/test/tester/tests/import_class_abstract_virtual_dispatch.ts b/tslang/test/tester/tests/import_class_abstract_virtual_dispatch.ts new file mode 100644 index 000000000..62a1ec0cf --- /dev/null +++ b/tslang/test/tester/tests/import_class_abstract_virtual_dispatch.ts @@ -0,0 +1,29 @@ +import './export_class_abstract_virtual_dispatch' + +class Square extends M.Shape { + side: number = 3; + + area(): number { + return this.side * this.side; + } +} + +function main() { + const s = new Square(); + + // deliberately call describe() FIRST (no preceding area() call) and + // TWICE: with the vtable slots misaligned, describe() called `.new` + // through a double-returning signature, so the result was whatever was + // left in XMM0 - a preceding `assert(s.area() == 9)` could leave 9.0 + // there and make a single assert pass by pure luck (this bug hid behind + // exactly that luck in import_class_abstract.ts for a while). + assert(s.describe() == "red area=9"); + assert(s.describe() == "red area=9"); + assert(s.area() == 9); + + const asShape: M.Shape = s; + assert(asShape.describe() == "red area=9"); + assert(asShape.area() == 9); + + print("done."); +}