-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Fix unreachable code detection persisting after incremental edits #62783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
…nder Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where unreachable code errors persist after removing the statement that caused unreachability in incremental compilation scenarios. When a throw statement is added and then deleted, subsequent code remains incorrectly flagged as unreachable despite being reachable again.
- Clears stale
NodeFlags.Unreachableflags during re-binding in incremental scenarios - Adds a fourslash test demonstrating the add/remove/verify workflow
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/cases/fourslash/unreachableCodeAfterEdit.ts | New test that verifies unreachable code errors clear after removing throw statement |
| src/compiler/binder.ts | Clears NodeFlags.Unreachable before reachability analysis to prevent stale state |
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
@copilot also do this for labels |
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Applied the same fix for labels in 6bb33c1. The Unreachable flag is now cleared before being set in bindLabeledStatement, and also cleared when a label becomes referenced. |
|
@copilot There wont be an existing test. Undo your label fix, then make a new fourslash test which uses a label, then makes it unused and have an error, then restores the use and asserts no errors. You need to disallow unused labels. Make that failing test, commit it, then fix the bug. If it turns out it already works fine, commit the passing test anyway, then undo the main fix in the PR to make sure both tests fail |
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Created the label test in ff3e6fd and investigated why it passes without needing a fix. Debug output revealed that label identifier nodes always have In contrast, statement nodes like class declarations ARE reused, which is why they need the flag cleared in Test committed in ff3e6fd, fix restored in f3981b2. |
|
The more reasonable answer for why this doesn't affect labels is that incremental parsing transfers statements, but if you have: myLabel: while (true) {
if (Math.random() > 0.5) {
/*marker*/break myLabel;
}
}Even at the top level, modifying any part of this replaces the whole statement. And since label uses must necessarily be inside the labeled statement (no goto), any other situation is impossible. But, for the |
Fixes incorrect unreachable code detection that persists after editing code in incremental compilation scenarios.
Problem
When code is edited to remove a statement that made subsequent code unreachable (e.g., a
throwstatement), the unreachable code error persists incorrectly. This reproduces the issue reported in microsoft/vscode#278270.Root Cause
In incremental scenarios where ASTs are reused, the binder was not clearing the
NodeFlags.Unreachableflag from previous binding passes. When re-binding a file after edits, statement nodes retained the Unreachable flag even though the code that made them unreachable had been removed.Solution
Added code in
src/compiler/binder.tsto clear theUnreachableflag in thebindChildrenfunction before determining if the node is currently unreachable. This ensures that the flag is reset for each binding pass, preventing stale flags from persisting.Why Labels Don't Need Special Handling
Through debugging, I discovered that label identifier nodes are recreated fresh by the parser in incremental scenarios (always have flags=0 before binding), whereas statement nodes like classes are reused with their existing flags. This is why the unused label test passes without needing additional handling - label nodes don't exhibit the same bug.
Changes
NodeFlags.UnreachableinbindChildrenbefore reachability checkTesting
Backlogmilestonemainbranchhereby runtests-parallellocally (99,000 tests passing)Test Results
✅ Unreachable code test passes
✅ Unused label test passes
✅ Existing unreachable code test passes
✅ Label-related tests pass (60 tests)
✅ Lint passes
✅ Format passes
Fixes #62782
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.