Make catch clause checking consistent with variable declarations #52240
Conversation
This file contains 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
|
@typescript-bot test this |
typescript-bot
added
Author: Team
For Milestone Bug
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at f877b23. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at f877b23. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at f877b23. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at f877b23. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at f877b23. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at f877b23. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at f877b23. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at f877b23. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52240
System
Hosts
Scenarios
TSServerComparison Report - main..52240
System
Hosts
Scenarios
StartupComparison Report - main..52240
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Also, with this change in place, it would be really, really easy to allow arbitrary type annotations on catch clauses. |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsfelixrieseberg/windows95
|
jakebailey
changed the title
jakebailey
commented
Jan 14, 2023
| // These are okay with useUnknownInCatchVariables=false, but not okay with useUnknownInCatchVariables=true. | ||
| try {} catch ({ x }) { x } | ||
| ~ | ||
| !!! error TS2339: Property 'x' does not exist on type '{}'. |
There was a problem hiding this comment.
These kinds of errors happen in strictNullChecks=false because destructuring an object uses the apparent type, and the apparent type of unknown in strictNullChecks=false is {}. See #47494.
I can't say I like these errors (especially given the errors for destructuring an array actually do mention unknown), but I feel like the likelihood of someone enabling useUnknownInCatchVariables but not enabling strictNullChecks is very low, as is having strictNullChecks disabled and explicitly annotating with : unknown.
jakebailey
requested review from
ahejlsberg,
DanielRosenwasser,
RyanCavanaugh and
weswigham
sandersn
approved these changes
Jan 19, 2023
| @@ -27079,6 +27078,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| isFunctionLike(node) && !getImmediatelyInvokedFunctionExpression(node) || | |||
| node.kind === SyntaxKind.ModuleBlock || | |||
| node.kind === SyntaxKind.SourceFile || | |||
| node.kind === SyntaxKind.CatchClause || | |||
There was a problem hiding this comment.
why is this needed? How much does it increase the tree-walking that control flow analysis does?
There was a problem hiding this comment.
Right now, this code is wrong for catch clauses because it doesn't stop at the CatchClause (where the catch clause variable declaration is scoped) and goes to the try/catch's enclosing scope.
Then, it determines that the catch clause variable is not yet defined (because it's checking the wrong scope!), but then because we only allow any or unknown, assumeInitialized turns into true, saving the day.
I noticed this on the PR where I allowed arbitrary annotations because this code path happens to be called by semantic tokens and causes errors that persist in tsserver (yet do not show in tests!); I cherry-picked it even though it's not strictly required because 1) it's definitely wrong, and 2) it's faster to skip all of the work if we know it's a catch clause variable.
There was a problem hiding this comment.
(Half-related, but I swear I read code during working on this PR that looked the same as this code but was correct; I can't remember where anymore...)
There was a problem hiding this comment.
I think it could be faster, even; assumeInitialized is a huge expression and I think a bit of work could be skipped if some of the more simple conditions were true.
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.
Fixes #47383
This changes our handling of catch clauses to work just like other variable declarations (by using the same code!), with the exception that:
anyorunknown. This is the same as before.anyorunknowndepending onuseUnknownInCatchVariables.any, we don't want to see follow-on errors about implicit any within the destructured value. (Or, maybe we do? Probably not?)Now that we check these declarations using the same code as variable declarations, we can correctly and consistently handle destructuring within catch clauses, fixing the linked issue. It also seems to fix issues I've observed in tsserver as the old error was only surfaced as a side effect (so, sometimes errors would appear thanks to semantic tokens and such).
This is a breaking change as you'll get errors on destructure if the type is unknown, but I think it's more consistent.