Skip to content

Commit 89eb986

Browse files
Fix cross-module class extends crash and add regression coverage (#274)
ClassMethodAccess's isDynamicImport branches (called for a class whose base lives in another module) fed an unchecked, possibly-null Value into CreateBoundFunctionOp when the target method's global variable couldn't be resolved by name - an intermittent, heap-layout-sensitive access violation (masked whenever a debugger happened to be attached, which made it look like a flaky Heisenbug). Adds a null-check that turns the failure into a clean compile error, plus a theModule.lookupSymbol fallback since compiler- synthesized methods (e.g. .instanceOf) are registered as real FuncOps rather than the dlsym-style global variables regular imported methods use. Adds the project's first regression coverage for cross-module class inheritance: basic 2-level extends, a 3-level chain, and extends+implements combined. The non-shared-lib variants pass and are enabled; the -shared (AOT/JIT) variants hit a separate, deeper pre-existing bug in when .instanceOf gets synthesized for a class compiled across a real DLL boundary - left disabled with a detailed comment rather than patched further, since two targeted fix attempts each introduced a worse regression (an infinite loop in unrelated same-module class tests). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 6c98e8d commit 89eb986

8 files changed

Lines changed: 252 additions & 3 deletions

tslang/lib/TypeScript/MLIRGenImpl.h

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5353,13 +5353,18 @@ class MLIRGenImpl
53535353
{
53545354
// need to resolve global variable
53555355
auto globalFuncVar = resolveFullNameIdentifier(location, funcName, false, genContext);
5356+
if (!globalFuncVar)
5357+
{
5358+
emitError(location, "Class member '") << funcName << "' can't be resolved (dynamic import)";
5359+
return mlir::Value();
5360+
}
53565361

53575362
if (!isThisValueClassRef)
53585363
{
53595364
CAST_A(opaqueThisValue, location, getOpaqueType(), thisValue, genContext);
53605365
auto boundMethodValue = builder.create<mlir_ts::CreateBoundFunctionOp>(
5361-
location, getBoundFunctionType(effectiveFuncType), opaqueThisValue, globalFuncVar);
5362-
return boundMethodValue;
5366+
location, getBoundFunctionType(effectiveFuncType), opaqueThisValue, globalFuncVar);
5367+
return boundMethodValue;
53635368
}
53645369

53655370
return globalFuncVar;
@@ -5442,7 +5447,33 @@ class MLIRGenImpl
54425447
if (classInfo->isDynamicImport)
54435448
{
54445449
// need to resolve global variable
5450+
//
5451+
// Not every method of an isDynamicImport class is actually
5452+
// registered as a dlsym-style global variable - a
5453+
// compiler-synthesized method (e.g. .instanceOf, ForceVirtual,
5454+
// see mlirGenClassInstanceOfMethod) never carries its own
5455+
// @dllimport decorator (that's only ever attached to
5456+
// source-declared methods reprinted under `@dllimport class
5457+
// ... { ... }`), so mlirGenFunctionLikeDeclaration's decorator
5458+
// check never routes it through
5459+
// mlirGenFunctionLikeDeclarationDynamicImport - it gets a real
5460+
// (bodyless-for-a-declaration) FuncOp registered directly
5461+
// instead, just like a same-module method. Try that first.
5462+
if (auto funcOp = theModule.lookupSymbol<mlir_ts::FuncOp>(funcName))
5463+
{
5464+
auto thisSymbOp = builder.create<mlir_ts::ThisSymbolRefOp>(
5465+
location, getBoundFunctionType(effectiveFuncType), effectiveThisValue,
5466+
mlir::FlatSymbolRefAttr::get(builder.getContext(), funcName));
5467+
return thisSymbOp;
5468+
}
5469+
54455470
auto globalFuncVar = resolveFullNameIdentifier(location, funcName, false, genContext);
5471+
if (!globalFuncVar)
5472+
{
5473+
emitError(location, "Class member '") << funcName << "' can't be resolved (dynamic import)";
5474+
return mlir::Value();
5475+
}
5476+
54465477
CAST_A(opaqueThisValue, location, getOpaqueType(), effectiveThisValue, genContext);
54475478
auto boundMethodValue = builder.create<mlir_ts::CreateBoundFunctionOp>(
54485479
location, getBoundFunctionType(effectiveFuncType), opaqueThisValue, globalFuncVar);
@@ -5457,7 +5488,7 @@ class MLIRGenImpl
54575488
return thisSymbOp;
54585489
}
54595490
}
5460-
}
5491+
}
54615492

54625493
mlir::Value ClassGenericMethodAccess(ClassInfo::TypePtr classInfo,
54635494
mlir::Location location, mlir::Value thisValue, int genericMethodIndex,

tslang/test/tester/CMakeLists.txt

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -847,6 +847,9 @@ add_test(NAME test-compile-include-global-var COMMAND test-runner "${PROJECT_SOU
847847
# imports support only compile mode
848848
add_test(NAME test-compile-import-component COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/component.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/service.ts")
849849
add_test(NAME test-compile-export-import-class-interface COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_interface.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_interface.ts")
850+
add_test(NAME test-compile-export-import-class-extends COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends.ts")
851+
add_test(NAME test-compile-export-import-class-extends-multilevel COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends_multilevel.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends_multilevel.ts")
852+
add_test(NAME test-compile-export-import-class-extends-implements-diamond COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends_implements_diamond.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends_implements_diamond.ts")
850853

851854
# shared libs tests (dlls/compile-time)
852855
add_test(NAME test-compile-time-shared-decl-emit-type COMMAND test-runner -shared -compile-time "${PROJECT_SOURCE_DIR}/test/tester/tests/emit_compiletime_func.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/decl_type.ts")
@@ -862,6 +865,30 @@ add_test(NAME test-compile-shared-decl-emit-class COMMAND test-runner -shared "$
862865
# shared libs tests (exports/imports)
863866
add_test(NAME test-compile-shared-export-import-class-interface COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_interface.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_interface.ts")
864867
add_test(NAME test-compile-shared-export-import-object-literal-with-class-types COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_with_class_types.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_with_class_types.ts")
868+
# KNOWN ISSUE (pre-existing, not a regression from the crash fix below): a
869+
# derived class whose base class lives in another module ("shared lib" /
870+
# cross-module `class extends`) does not reliably work end-to-end through
871+
# test-runner's actual -shared invocation (no explicit --shared-libs for the
872+
# imported DLL; the import statement's own auto-load is all that's used) -
873+
# AOT fails to link (missing dllimport linkage on the compiler-synthesized
874+
# `.instanceOf` method every class gets) and JIT fails to even compile
875+
# (`.instanceOf` sometimes never gets synthesized/registered at all - a
876+
# discovery/partial-resolve pass ordering bug: it depends sensitively on
877+
# exact compile-time sequencing, e.g. a manual repro that adds an extra
878+
# --shared-libs flag "accidentally" avoids it). Two attempts at a targeted
879+
# fix (deferring synthesis until a non-speculative pass) were reverted after
880+
# each caused an INFINITE LOOP in unrelated same-module class tests (a
881+
# same-module class's own discovery retry loop can also always run under
882+
# allowPartialResolve, so deferring "until a real pass" can mean forever) -
883+
# worse than the original clean compile error, so left as a known issue
884+
# rather than risk that regression again. What IS fixed and verified: the
885+
# crash (access violation) that used to occur instead of this clean error -
886+
# see MLIRGenFunctions.cpp's mlirGenFunctionLikeDeclarationDynamicImport
887+
# (registration name/map fix) and MLIRGenImpl.h's ClassMethodAccess
888+
# (FuncOp-or-variable dual lookup with a null-check backstop).
889+
# add_test(NAME test-compile-shared-export-import-class-extends COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends.ts")
890+
# add_test(NAME test-compile-shared-export-import-class-extends-implements-diamond COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends_implements_diamond.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends_implements_diamond.ts")
891+
# add_test(NAME test-compile-shared-export-import-class-extends-multilevel COMMAND test-runner -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends_multilevel.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends_multilevel.ts")
865892
add_test(NAME test-compile-shared-export-import-object-literal-with-interface COMMAND test-runner -shared -gctors-as-method "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_with_interface.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_with_interface.ts")
866893
add_test(NAME test-compile-shared-export-import-object-literal-untyped COMMAND test-runner -shared -gctors-as-method "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_untyped.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_untyped.ts")
867894
add_test(NAME test-compile-shared-export-import-object-literal-untyped-multi-method COMMAND test-runner -shared -gctors-as-method "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_untyped_multi_method.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_untyped_multi_method.ts")
@@ -888,6 +915,10 @@ add_test(NAME test-jit-shared-decl-emit-class COMMAND test-runner -jit -shared "
888915
# shared libs tests (exports/imports)
889916
add_test(NAME test-jit-shared-export-import-class-interface COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_interface.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_interface.ts")
890917
add_test(NAME test-jit-shared-export-import-object-literal-with-class-types COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_with_class_types.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_with_class_types.ts")
918+
# KNOWN ISSUE - see the matching commented-out compile-shared entries above.
919+
# add_test(NAME test-jit-shared-export-import-class-extends COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends.ts")
920+
# add_test(NAME test-jit-shared-export-import-class-extends-multilevel COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends_multilevel.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends_multilevel.ts")
921+
# add_test(NAME test-jit-shared-export-import-class-extends-implements-diamond COMMAND test-runner -jit -shared "${PROJECT_SOURCE_DIR}/test/tester/tests/import_class_extends_implements_diamond.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_class_extends_implements_diamond.ts")
891922
add_test(NAME test-jit-shared-export-import-object-literal-with-interface COMMAND test-runner -jit -shared -gctors-as-method "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_with_interface.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_with_interface.ts")
892923
add_test(NAME test-jit-shared-export-import-object-literal-untyped COMMAND test-runner -jit -shared -gctors-as-method "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_untyped.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_untyped.ts")
893924
add_test(NAME test-jit-shared-export-import-object-literal-untyped-multi-method COMMAND test-runner -jit -shared -gctors-as-method "${PROJECT_SOURCE_DIR}/test/tester/tests/import_object_literal_untyped_multi_method.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/export_object_literal_untyped_multi_method.ts")
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
namespace M {
2+
3+
// Base class exported so another module can `extends` it - covers the
4+
// cross-module analogue of 00class_super.ts's same-module class extends,
5+
// which was never verified across the module boundary (unlike interface
6+
// extends, which got dedicated cross-module coverage in #268-#270).
7+
8+
export class Animal {
9+
name: string;
10+
11+
constructor(name: string) {
12+
this.name = name;
13+
}
14+
15+
speak(): string {
16+
return `${this.name} makes a noise.`;
17+
}
18+
}
19+
}
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
namespace M {
2+
3+
// A class can only `extends` one class, but can `implements` several
4+
// interfaces - this is the class-shaped equivalent of the interface
5+
// diamond coverage (Combined extends Left, Right, #268): a cross-module
6+
// base class plus two cross-module interfaces, both satisfied by one
7+
// most-derived class in the importer. Exercises the same per-object
8+
// vtable-patching machinery from a different angle (class vtable slots
9+
// for inherited methods interleaved with interface vtable slots for
10+
// implemented methods).
11+
12+
export class Base {
13+
base: number = 1;
14+
15+
addBase(n: number): void {
16+
this.base = this.base + n;
17+
}
18+
}
19+
20+
export interface Left {
21+
left: number;
22+
addLeft(n: number): void;
23+
}
24+
25+
export interface Right {
26+
right: number;
27+
addRight(n: number): void;
28+
}
29+
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
namespace M {
2+
3+
// 3-level chain with the first two levels defined (and already linked to
4+
// each other) in the exporting module, so the importer's further-derived
5+
// class must extend a base whose OWN vtable was built across a module
6+
// boundary already - this is the class analogue of the cross-module
7+
// interface multilevel coverage (extends_interface_multilevel.ts), which
8+
// found real vtable-offset bugs when a 2nd/3rd extends target's methods
9+
// were mis-patched.
10+
11+
export class A {
12+
a: number = 1;
13+
14+
addA(n: number): void {
15+
this.a = this.a + n;
16+
}
17+
18+
describe(): string {
19+
return `A:${this.a}`;
20+
}
21+
}
22+
23+
export class B extends A {
24+
b: number = 2;
25+
26+
addB(n: number): void {
27+
this.b = this.b + n;
28+
}
29+
30+
describe(): string {
31+
return `B:${this.b}/${super.describe()}`;
32+
}
33+
}
34+
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import './export_class_extends'
2+
3+
class Dog extends M.Animal {
4+
constructor(name: string) {
5+
super(name);
6+
}
7+
8+
speak(): string {
9+
return `${this.name} barks.`;
10+
}
11+
}
12+
13+
function main() {
14+
const a = new M.Animal("Generic");
15+
assert(a.speak() == "Generic makes a noise.");
16+
17+
const d = new Dog("Mitzie");
18+
assert(d.name == "Mitzie");
19+
assert(d.speak() == "Mitzie barks.");
20+
21+
// virtual dispatch through the base-class-typed reference must still
22+
// resolve to the cross-module-derived override
23+
const asBase: M.Animal = d;
24+
assert(asBase.speak() == "Mitzie barks.");
25+
26+
print("done.");
27+
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import './export_class_extends_implements_diamond'
2+
3+
class Combined extends M.Base implements M.Left, M.Right {
4+
left: number = 2;
5+
right: number = 3;
6+
7+
addLeft(n: number): void {
8+
this.left = this.left + n;
9+
}
10+
11+
addRight(n: number): void {
12+
this.right = this.right + n;
13+
}
14+
}
15+
16+
function main() {
17+
const c = new Combined();
18+
19+
c.addBase(10);
20+
c.addLeft(20);
21+
c.addRight(30);
22+
23+
assert(c.base == 11);
24+
assert(c.left == 22);
25+
assert(c.right == 33);
26+
27+
// cast to each cross-module ancestor/interface type and confirm the
28+
// field/method resolves through the correct (non-corrupted) slot
29+
const asBase: M.Base = c;
30+
assert(asBase.base == 11);
31+
32+
const asLeft: M.Left = c;
33+
asLeft.addLeft(100);
34+
assert(c.left == 122);
35+
36+
const asRight: M.Right = c;
37+
asRight.addRight(100);
38+
assert(c.right == 133);
39+
40+
print("done.");
41+
}
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import './export_class_extends_multilevel'
2+
3+
class C extends M.B {
4+
c: number = 3;
5+
6+
addC(n: number): void {
7+
this.c = this.c + n;
8+
}
9+
10+
describe(): string {
11+
return `C:${this.c}/${super.describe()}`;
12+
}
13+
}
14+
15+
function main() {
16+
const c = new C();
17+
18+
c.addA(10);
19+
c.addB(20);
20+
c.addC(30);
21+
22+
assert(c.a == 11);
23+
assert(c.b == 22);
24+
assert(c.c == 33);
25+
26+
assert(c.describe() == "C:33/B:22/A:11");
27+
28+
// virtual dispatch through each ancestor-typed reference must still
29+
// reach the most-derived override
30+
const asB: M.B = c;
31+
assert(asB.describe() == "C:33/B:22/A:11");
32+
33+
const asA: M.A = c;
34+
assert(asA.describe() == "C:33/B:22/A:11");
35+
36+
print("done.");
37+
}

0 commit comments

Comments
 (0)