fix break and continue inside sync try/catch blocks#1727
Open
pilaoda wants to merge 5 commits into
Open
Conversation
…ly handle returns and re-throws (test): - enable previously skipped re-throw tests - add tests for complex try-catch-finally return and throw scenarios
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the TypeScript-to-Lua try/catch/finally transformation to correctly handle control flow (return/break/continue) across try, catch, and finally blocks. Previously, finally did not behave correctly with respect to control flow (see #1137), and break/continue inside try were only handled in async contexts.
Changes:
- Rewrote
transformTryStatementto wrap both try and catch inpcall, using a unified____hasReturnOrErrorflag so finally can override returns and errors can be re-thrown after finally executes. - Generalized the try-scope break/continue flag handling (previously
asyncTryHas*) to apply to all try/catch blocks, not just async ones; renamedfindAsyncTryScopeBeforeLoop→findTryScopeBeforeLoopandasyncTryHas{Break,Continue}→tryHas{Break,Continue}. - Added extensive unit tests covering returns from try/catch/finally, throws from finally, and break/continue inside try/catch with and without finally; un-skipped the
re-throwtest (#1137).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/transformation/visitors/errors.ts | Rewrote try transform with double-pcall pattern and propagated source nodes to emitted Lua nodes. |
| src/transformation/visitors/break-continue.ts | Removed async-only gating; now uses the renamed findTryScopeBeforeLoop and tryHas{Break,Continue} flags. |
| src/transformation/utils/scope.ts | Renamed scope fields and helper to drop the async prefix since they now apply to all try scopes. |
| test/unit/error.spec.ts | Added many new tests for finally-overriding-return, throw-in-finally, and break/continue inside try/catch; enabled the previously-skipped re-throw test. |
Co-Authored-By: Claude Opus 4.6 <[email protected]>
pilaoda
force-pushed
the
feat/sync-try-break-continue
branch
from
979e9eb to
6a384ec
Compare
… tryCall Co-Authored-By: Claude Opus 4.6 <[email protected]>
pilaoda
force-pushed
the
feat/sync-try-break-continue
branch
from
6a384ec to
3d34a67
Compare
Extract duplicated return-if-has-return-or-error into a module-level helper. Expand the elseBranch ternary into an explicit if/else with comments for each case (non-empty catch, no catch, empty catch). Co-Authored-By: Claude Opus 4.6 <[email protected]>
- implement break and continue support within try-catch-finally blocks - generalize and rename async-specific try scope properties and helpers (test): add tests for break and continue inside try-catch-finally blocks
pilaoda
force-pushed
the
feat/sync-try-break-continue
branch
from
3d34a67 to
5f38c9a
Compare
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.
Depends on #1726
Summary
break/continueinside pcall-wrapped try/catch into flag variables +return, then check flags after pcall to execute the actual control flow.findAsyncTryScopeBeforeLoop/findTryScopeBeforeLoopandasyncTryHasBreak/asyncTryHasContinueinto shared helpers and fields.