Run enclosing finally when break/continue jumps out of a try block#234
Merged
Conversation
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
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
Fixes the follow-up tracked as item 5b in
docs/LowerToAffineLoops-LowerToLLVM-review.md(found while testing #233):break/continueinside a try body or catch clause jumped straight to the loop's continuation/increment block, silently skipping the enclosingfinally.Fixed the same way
returnin try/finally already worked, per the repo owner's suggestion:TryOpLoweringcollectsBreakOp/ContinueOpops in the body/catches that already have atsContext->jumpsentry at try-lowering time. That's a clean discriminator for "jumps out of this try" — the target loop/label encloses the try, so its lowering (which populatesjumps) ran first in the pre-order worklist, while jumps targeting a loop inside the try aren't resolved yet.finallyregion is cloned once (mirroring the existing return-cleanup clone), the clone is terminated with a branch to the original target, andjumps[op]is re-pointed at the clone.BreakOpLowering/ContinueOpLoweringneed no structural change — they just branch into the finally copy, which continues to the real target.unwind[op], and the break/continue lowerings now emitEndCatchOpwhen it's set, mirroring return-from-catch.Test plan
ctest --test-dir __build/tslang/windows-msbuild-2026-debug -C Debug -j8— 690/690 passed (JIT + AOT)00try_finally_break_continue.tsextended with: break in try body, continue in try body, two-level nested finally chain (both finallys run), break from a catch clause (catch + finally + EndCatch), one try with both a break and a continue escaping (two distinct targets → two separate finally copies), and a negative case (break of a loop fully inside the try is not rerouted — finally runs exactly once)