Skip to content

Commit bc91ff4

Browse files
Fix cross-module abstract-class vtable slot shift (printer dropped abstract) (#290)
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.<abstractMethod>()` 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 <noreply@anthropic.com>
1 parent 7183690 commit bc91ff4

5 files changed

Lines changed: 89 additions & 17 deletions

File tree

tslang/lib/TypeScript/DeclarationPrinter.cpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,17 @@ namespace typescript
401401
printNamespaceBegin(classType->elementNamespace);
402402

403403
printBeforeDeclaration();
404+
// `abstract` is layout-relevant, not just semantic: mlirGenClassNew skips
405+
// synthesizing the `.new` method for abstract classes, so dropping the
406+
// modifier makes the reimporting module insert a `.new` vtable slot the
407+
// exporting module never had, shifting every subsequent virtual index and
408+
// corrupting cross-module virtual dispatch (base-module code calling
409+
// this.method() lands on the wrong slot).
410+
if (classType->isAbstract)
411+
{
412+
os << "abstract ";
413+
}
414+
404415
os << "class " << classType->name;
405416

406417
if (classType->baseClasses.size() > 0)
@@ -554,6 +565,11 @@ namespace typescript
554565
os << "private ";
555566
}
556567

568+
if (method.isAbstract)
569+
{
570+
os << "abstract ";
571+
}
572+
557573
printMethod(
558574
method.isStatic,
559575
method.name,

tslang/test/tester/CMakeLists.txt

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -946,20 +946,22 @@ add_test(NAME test-compile-shared-export-import-class-static COMMAND test-runner
946946
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")
947947
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")
948948
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")
949-
# DISABLED: a separate, NOT fixed bug found 2026-07-22 while verifying the fix above. Unlike the
950-
# multilevel/optional/structural variants (all pass now), this one calls describe() - a CONCRETE
951-
# method inherited from the abstract dynamic-import base M.Shape, whose body internally calls the
952-
# still-virtual this.area() - through an interface reference
953-
# (`const asDescribable: M.Describable = s; asDescribable.describe()`). Confirmed via the real
954-
# harness (instrumented with print instead of assert, see the session transcript): the DIRECT call
955-
# `s.describe()` and the base-class-cast call `asShape.describe()` both return the correct
956-
# "red area=9", but the SAME describe() reached through the interface returns "red area=0" -
957-
# this.area() silently resolves to 0 instead of dispatching to Square's override, specifically
958-
# when invoked via the interface's function-pointer slot. Root cause not yet isolated (likely a
959-
# `this`-identity/binding gap specific to calling an inherited concrete method through an
960-
# interface vtable slot, distinct from the link-time issue fixed above) - left for a dedicated
961-
# follow-up.
962-
# 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")
949+
# FIXED 2026-07-23 (was disabled): the "this.area()-returns-0-via-interface" symptom had nothing
950+
# to do with interfaces or `this` binding. DeclarationPrinter dropped the `abstract` modifier when
951+
# embedding the class declaration into the compiled binary (__decls), so the reimporting module
952+
# treated M.Shape as concrete and synthesized a `.new` method for it - a vtable slot the exporting
953+
# module never created (mlirGenClassNew skips `.new` for abstract classes). Every base-class slot
954+
# after `.instanceOf` was thereby shifted by one in the importer's rebuilt vtable, and describe()'s
955+
# compiled-in slot-1 read for area() landed on `.new` instead - a pointer-returning function called
956+
# through a double-returning signature, so the "result" was whatever garbage was left in XMM0.
957+
# That garbage is why the symptom looked interface-specific: a preceding `assert(s.area() == 9)`
958+
# left 9.0 in XMM0, making the direct and base-class-cast calls "pass" by pure luck, while the
959+
# interface path (more string machinery in between) got 0. See the abstract-virtual-dispatch tests
960+
# below for the deterministic minimal regression pair.
961+
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")
962+
# minimal deterministic regression pair for the vtable-slot-shift bug above (single-level abstract,
963+
# describe() called twice before any area() call so XMM0 luck can't hide a regression)
964+
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")
963965
# FIXED: see the matching test-compile-export-import-class-generic comment above (2026-07-22).
964966
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")
965967
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
10071009
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")
10081010
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")
10091011
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")
1010-
# 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.
1011-
# 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")
1012+
# 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).
1013+
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")
1014+
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")
10121015
# FIXED: see the matching test-compile-export-import-class-generic comment above (2026-07-22).
10131016
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")
10141017
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")

tslang/test/tester/test-runner.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ std::string checkOutputAndCleanup(std::string tempOutputFileNameNoExt)
270270
auto output = readOutput(txtFile);
271271
auto errors = readOutput(errFile);
272272

273-
deleteFiles(tempOutputFileNameNoExt);
273+
if (!getenv("TSLANG_TEST_KEEP_TEMP")) deleteFiles(tempOutputFileNameNoExt);
274274

275275
if (output.find("done.") != std::string::npos)
276276
{
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
namespace M {
2+
3+
// Single-level abstract class whose CONCRETE method dispatches the
4+
// still-abstract area() through `this` - the minimal shape of the
5+
// cross-module vtable-slot-mismatch bug: DeclarationPrinter used to drop
6+
// the `abstract` modifier when embedding this declaration into the
7+
// compiled binary (__decls), so the reimporting module treated Shape as
8+
// concrete and synthesized a `.new` vtable slot the exporting module
9+
// never had (mlirGenClassNew skips it for abstract classes). That
10+
// shifted every subsequent slot by one, and describe()'s baked-in
11+
// slot-1 read for area() landed on `.new` instead - returning whatever
12+
// garbage happened to sit in XMM0 (a pointer-returning function called
13+
// through a double-returning signature).
14+
15+
export abstract class Shape {
16+
color: string = "red";
17+
18+
abstract area(): number;
19+
20+
describe(): string {
21+
return `${this.color} area=${this.area()}`;
22+
}
23+
}
24+
}
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import './export_class_abstract_virtual_dispatch'
2+
3+
class Square extends M.Shape {
4+
side: number = 3;
5+
6+
area(): number {
7+
return this.side * this.side;
8+
}
9+
}
10+
11+
function main() {
12+
const s = new Square();
13+
14+
// deliberately call describe() FIRST (no preceding area() call) and
15+
// TWICE: with the vtable slots misaligned, describe() called `.new`
16+
// through a double-returning signature, so the result was whatever was
17+
// left in XMM0 - a preceding `assert(s.area() == 9)` could leave 9.0
18+
// there and make a single assert pass by pure luck (this bug hid behind
19+
// exactly that luck in import_class_abstract.ts for a while).
20+
assert(s.describe() == "red area=9");
21+
assert(s.describe() == "red area=9");
22+
assert(s.area() == 9);
23+
24+
const asShape: M.Shape = s;
25+
assert(asShape.describe() == "red area=9");
26+
assert(asShape.area() == 9);
27+
28+
print("done.");
29+
}

0 commit comments

Comments
 (0)