Fix nested break/continue scoping, stack-new sizing, and 3 other lowering bugs#232
Merged
Merged
Conversation
- Implemented visitBreakContinueInScope to correctly handle unlabeled break/continue in nested loops. - Replaced assert with notifyMatchFailure in SymbolCallInternalOpLowering for unresolved callee symbols. - Adjusted stack allocation in NewOpLowering to use the correct storage type. - Added test for triple nested unlabeled break/continue behavior.
This was referenced Jul 14, 2026
ASDAlexander77
added a commit
that referenced
this pull request
Jul 15, 2026
A follow-up static audit of LowerToAffineLoops.cpp, LowerToLLVM.cpp, and the TypeScriptExceptionPass files (beyond the 10 findings already closed in PRs #230/#232/#233/#234/#235) found and fixes 7 more confirmed bugs: - SwitchStateOpLowering: generator yield/resume state labels were collected into a SmallPtrSet<Operation*,16>, whose iteration order is only insertion order below 16 entries; past that it silently falls back to pointer-hash order, scrambling the positional switch dispatch used by generator resume. Replaced with an order-preserving SmallVector. - StringConcatOpLowering: the stack-allocation path sized its Alloca using a pointer type instead of i8, over-allocating by sizeof(ptr) for every multi-argument string concatenation (e.g. console.log with 2+ args). - ArraySpliceOpLowering: mixed genuine MLIR index-dialect ops with already-LLVM-converted operands, a type mismatch that made every Array.prototype.splice() call fail to lower correctly (the only .splice() call in the test suite was commented out because of this). Reworked to stay consistently in the LLVM-converted domain, matching every sibling array-mutation lowering. - GlobalOpLowering: the side-effect enumeration deciding whether a global initializer can be inlined as a constant vs. must run as a real constructor was missing several ops with real side effects (array push/unshift/splice/pop/shift, delete, set-length, string concat/ char-to-string). - LandingPadFixPass: MadeChange was set unconditionally for every landingpad visited, even when nothing was rewritten, causing needless analysis invalidation on every EH-containing function on every compile. - Win32ExceptionPass: the PHI-user removal loop erased only the first PHI user found then unconditionally erased the underlying value, violating LLVM's no-remaining-uses invariant when more than one PHI referenced the same to-be-removed value (own TODO comment flagged this as incomplete). - Win32ExceptionPass: getThrowFn's fallback path created a stray _CxxThrowException Function without attaching it to the module. Added 00array_splice.ts (JIT + AOT) covering shrink/grow/equal-size splice cases, since this path had zero prior test coverage. Full ctest suite green: 692/692. 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
Follow-up to #230's static review of
LowerToAffineLoops.cpp/LowerToLLVM.cpp, addressing 5 of the 7 remaining PLAUSIBLE findings tracked indocs/LowerToAffineLoops-LowerToLLVM-review.md.WhileOp/DoWhileOp/ForOp/LabelOplowering claimed unlabeledbreak/continuefrom nested loops via an unbounded region walk, relying on undocumented MLIR dialect-conversion worklist ordering to land on the correct (innermost) target. Replaced with a sharedvisitBreakContinueInScopewalk that stops treating unlabeled break/continue as claimable once it crosses a nested loop/label (or, forbreakonly,switch) boundary, while still finding labeled jumps that legitimately target an outer scope.NewOpLowering's stack-allocation path sized the alloca off the converted pointer type of a class instead of its storage/struct type, allocating onlysizeof(ptr)bytes on the stack regardless of the class's actual field layout — a stack under-allocation for any stack-new'd class with fields.SymbolCallInternalOpLowering,LabelOpLowering,SwitchOpLoweringeach had a debug-onlyassertguarding a real failure condition (unresolved callee symbol; malformed label/switch region terminator), silently producing malformed IR in release builds. Converted to proper pattern failures (notifyMatchFailure).TryOp(e.g. from ausingdeclaration with no explicit catch/finally) erased its cleanup block's terminator with nothing replacing it on the Linux/Itanium path. Added the missing landing-pad setup, mirroring the existing Windows and finally-with-cleanup precedent.Item 8 (Windows landing-pad cleanup filter reusing the typed-catch filter value) is deliberately left alone — the code already self-flags as incomplete (
// BUG: in LLVM landing pad is not fully implemented) and a wrong fix risks a worse regression than the status quo. Item 5 (finally-region cloning losing break/continue/try wiring) also remains open, tracked in the review doc.Test plan
cmake --build __build/tslang/windows-msbuild-2026-debug --config Debug -j8— clean build, no new warningsctest --test-dir __build/tslang/windows-msbuild-2026-debug -C Debug -j8— 688/688 passed (JIT + AOT)test_triple_nested_unlabeledin00break_continue.ts(3 levels of nestedfor, innermost unlabeled break/continue) — JIT-executed and checked by value00disposable.ts/01disposable.ts/02disposable.ts(cleanup-only try viausing) — JIT + AOT green, exercising the item 9 fix