-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Move unreachable checks to checker #62751
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
|
@typescript-bot test it |
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 moves unreachability checks from the binder to the checker phase of compilation. Previously, unreachable code detection happened during binding and generated diagnostics immediately. Now, the binder marks nodes as unreachable using a flag, and the checker generates diagnostics when visiting those nodes. This allows for more flexible diagnostic reporting (errors vs suggestions) and better handling of consecutive unreachable statements.
Key Changes:
- Added
NodeFlags.Unreachableflag to mark unreachable nodes during binding - Moved diagnostic generation for unreachable code from binder to checker
- Improved handling of consecutive unreachable statements by reporting them as a single diagnostic range
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/compiler/types.ts | Added Unreachable flag to NodeFlags enum |
| src/compiler/binder.ts | Removed unreachability diagnostic logic; now only marks nodes as unreachable via flags |
| src/compiler/checker.ts | Added logic to detect and report unreachable code diagnostics based on flags set by binder |
| tests/cases/fourslash/*.ts | Updated tests to expect suggestion diagnostics instead of no diagnostics when unreachable code is allowed |
| tests/cases/compiler/reachabilityChecks*.ts | Added new test cases for unreachable code detection in various scenarios |
| tests/baselines/reference/*.txt | Updated baseline files to reflect new diagnostic reporting behavior |
Comments suppressed due to low confidence (1)
src/compiler/checker.ts:1
- [nitpick] The inline boolean comments for
skipTriviaparameters are unclear. Consider adding a comment explaining why we stop at comments but not at line breaks, or use named parameters if possible.
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
@typescript-bot perf test this faster |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot cherry-pick this to tsgo-port |
|
Hey, @jakebailey! I've created #62760 for you. This involved updating baselines; please check the diff. |
…62760) Co-authored-by: Jake Bailey <[email protected]>
This ports microsoft/typescript-go#2067 to this repo. See that PR for a longer write-up.
This is technically a breaking change, as these diagnostics are no longer automatically provided on bound files. But, you'd be silly to rely solely on the binder diagnostics for this info when the checker does even more unreachable checks using type info. So, I doubt anyone's doing that.
Not in this PR is changing the option declaration to make these not marked as source file affecting; have not looked into that (tsgo does not use that info at all).
Closes #55588
Fixes #55562