Fix cross-module super-accessor crash and add class coverage#278
Merged
Merged
Conversation
super.<accessor> (get or set) crashed MLIR verification because ClassAccessorAccess never repaired the `this` operand via getThisRefOfClass the way ClassMethodAccess does, leaving a raw by-value ClassStorageType struct where a pointer was expected. Not cross-module-specific - fixed by threading isSuperClass through ClassAccessorAccess, with a same-file regression test added alongside the cross-module one that surfaced it. Also adds cross-module (export/import) test coverage for abstract classes and static members, both passing cleanly with no changes needed.
2 tasks
ASDAlexander77
added a commit
that referenced
this pull request
Jul 22, 2026
…ss) (#282) super[i] hit llvm_unreachable: the ElementAccessExpression codegen's super-specific branch only handled a constant string index, with no fallback to route a real (non-constant) index into ClassIndexAccess the way ordinary t[i] access already does. Fixing that exposed the second, originally-suspected bug: ClassIndexAccess never took isSuperClass, so `super[i]`'s thisValue was never repaired via getThisRefOfClass the way ClassMethodAccess/ClassAccessorAccess already do (same class of bug PR #278 fixed for super.<accessor>). Adds a same-file regression test (00class_indexer_super.ts) covering an overridden indexer calling back into the base via super[i]/super[i] = v. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
ASDAlexander77
added a commit
that referenced
this pull request
Jul 23, 2026
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.<accessor>`'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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
super.<accessor>(get or set) triggered an MLIR verification error ('llvm.call' op operand type mismatch for operand 0) becauseClassAccessorAccessnever repaired thethisoperand viagetThisRefOfClass, unlikeClassMethodAccess/ClassGenericMethodAccess. Fixed by threadingisSuperClassthroughClassAccessorAccessand callinggetThisRefOfClassfor the non-static branch. Not cross-module-specific - reproduces in a single file too.export/import) test coverage for class areas that had none before: abstract classes, static members, and accessors (all 3 link tiers: plain compile,-sharedDLL,-jit -shared). Abstract classes and static members pass cleanly.00class_accessor_super.ts) for the crash fix, since the bug wasn't cross-module-specific.export_class_generic.ts/import_class_generic.ts) and the-sharedtiers of the accessor test are added but currently commented out inCMakeLists.txt- they found two additional, separate, pre-existing bugs in the cross-module declaration-reconstruction path (generic specializations wrongly getdllexportlinkage with a comma that breaks the linker's/EXPORT:directive; accessors aren't resolvable at all through the-shareddynamic-import path). These are documented in code comments and left disabled pending a dedicated fix.Test plan
ctest -C Debug -R accessor- all 7 accessor tests pass (including new same-file and cross-module tests)ctest -C Debug -R "class-abstract|class-static"- all 20 tests pass across all 3 tiersctest -C Debug -j8- 776/776 passed, no regressions