Fix optional interface method bugs (vtable crash + undefined comparison) and extend test coverage - #272
Merged
Merged
Conversation
Credits Claude Code as a development tool, matching the existing "Powered by LLVM" badge style. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a cross-module test pair for `interface Derived extends Base` where Base declares an optional field, cast from imported exported objects both with and without the optional field provided. Confirms the fix from #271 (same-module only) also covers the cross-module path, which has historically been more fragile due to vtable cloning and GC-heap boxing - no additional bug found; full interface test suite (69 tests) passes 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ited optional method
An object literal omitting an optional METHOD inherited via `extends`
(e.g. `interface Base { opt?(n: number): number }`, `interface Derived
extends Base`) crashed the compiler outright with
llvm_unreachable("not implemented yet") in
mlirGenObjectVirtualTableDefinitionForInterface. The equivalent missing
FIELD case already had a placeholder pattern (a -1 sentinel cast to the
field's ref type, inserted into the vtable slot), but the missing-method
case had never been implemented at all - this was simply unsupported,
not merely broken for extends specifically.
Fix: mirror the missing-field's -1-sentinel placeholder for a missing
method's vtable slot, cast to the method's function-pointer-ref type
instead of a field type.
New test 00interface_optional_method_extends.ts locks this in
(compile+jit, registered in CMakeLists.txt). Note: comparing an optional
interface method against `undefined` (`x.optMethod == undefined`) is a
separate, deeper, pre-existing bug (hardcoded to a compile-time-constant
result in UndefLogicHelper.h, unrelated to extends) - deliberately left
unfixed and documented in the test's comments after an attempted fix
caused an unresolved control-flow crash; deferred for a future session.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g wrong constant Comparing an optional interface method against `undefined` (`obj.optMethod == undefined`) never worked for any interface, extends or not: UndefLogicHelper.h's processUndefVale only special-cased InterfaceType/ClassType against undefined, so a BoundFunctionType fell through to a hardcoded compile-time-constant false/true regardless of whether the method was actually present. Fix required two coordinated changes: - InterfaceSymbolRefOpLowering (LowerToLLVM.cpp) now branchlessly selects a null `this` pointer (LLVM::SelectOp) when an optional method's vtable slot holds the "missing member" -1 sentinel - a real bound method's `this` is never null, giving the comparison something concrete to check. - UndefLogicHelper.h's new BoundFunctionType branch checks that null-or-not via a directly-emitted LLVM::ICmpOp, not the shared LogicOp<StdIOpTy, V1, v1, ...> helper: v1 is a template parameter baked in from the outer comparison operator that triggered the whole call (e.g. arith::CmpIPredicate::ne for a source-level `!=`), so passing a different SyntaxKind at the call site was silently ignored and inverted the result. An earlier attempt using CodeLogicHelper's conditionalExpressionLowering (block-splitting control flow) caused an unresolved crash; WinDbg dump analysis showed execution running off the end of a JIT-compiled block. Replacing it with the branchless select (already used successfully elsewhere in this file) resolved it cleanly. Restores the `== undefined`/`!= undefined` assertions in 00interface_optional_method_extends.ts now that they pass correctly. Full test suite: 758/758 passing. 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
Four commits, landed together on this branch:
interface Derived extends BasewhereBasedeclares an optional field, confirming the fix from Fix casting object literal that omits an extends-inherited optional field #271 also covers the cross-module path (no additional bug found).extendscrashed the compiler outright (llvm_unreachable) building the vtable placeholder; only missing fields had that pattern implemented. Fixed by mirroring the missing-field-1-sentinel placeholder for methods.undefinedcomparison always returning wrong constant — comparing any optional interface method againstundefined(obj.optMethod == undefined) was hardcoded to a compile-time-constant result, regardless of whether the method was actually present. Fixed with two coordinated changes: a branchlessLLVM::SelectOpinInterfaceSymbolRefOpLoweringthat nulls outthiswhen a method's vtable slot holds the "missing" sentinel, and a directly-emittedLLVM::ICmpOpinUndefLogicHelper.h(the sharedLogicOp<...>helper's comparison predicate turned out to be a template parameter baked in from the outer calling operator, silently ignoring theSyntaxKindpassed at the call site and inverting the result).Test plan
00interface_optional_extends.ts,export/import_object_literal_structural_typed_extends_interface_optional.ts(cross-module),00interface_optional_method_extends.ts— all compile+jit, registered intest/tester/CMakeLists.txtctest -C Debug): 758/758 passing, 100%ctest -R "interface", 71 tests): 100% passing🤖 Generated with Claude Code