-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Make catch clause checking consistent with variable declarations #52240
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
165b112
Add test cases
jakebailey f877b23
Treat catch clauses the same as other variable declarations
jakebailey 6a21dd6
Also test non-strict mode
jakebailey c7a4c13
Return early
jakebailey ac616e2
Merge branch 'main' into fix-47383
jakebailey 6aba586
Merge branch 'main' into fix-47383
jakebailey 838da0d
Remove comment
jakebailey b4ae1c2
Fix error in tsserver with semantic tokens enabled
jakebailey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
60 changes: 60 additions & 0 deletions
60
...eference/destructureCatchClause(strict=false,useunknownincatchvariables=false).errors.txt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| tests/cases/compiler/destructureCatchClause.ts(26,17): error TS2339: Property 'x' does not exist on type '{}'. | ||
| tests/cases/compiler/destructureCatchClause.ts(27,15): error TS2461: Type 'unknown' is not an array type. | ||
| tests/cases/compiler/destructureCatchClause.ts(29,17): error TS2339: Property 'a' does not exist on type '{}'. | ||
| tests/cases/compiler/destructureCatchClause.ts(30,17): error TS2339: Property 'a' does not exist on type '{}'. | ||
| tests/cases/compiler/destructureCatchClause.ts(32,15): error TS2461: Type 'unknown' is not an array type. | ||
| tests/cases/compiler/destructureCatchClause.ts(33,15): error TS2461: Type 'unknown' is not an array type. | ||
| tests/cases/compiler/destructureCatchClause.ts(35,17): error TS2339: Property 'a' does not exist on type '{}'. | ||
|
|
||
|
|
||
| ==== tests/cases/compiler/destructureCatchClause.ts (7 errors) ==== | ||
| // These are okay with useUnknownInCatchVariables=false, but not okay with useUnknownInCatchVariables=true. | ||
| try {} catch ({ x }) { x } | ||
| try {} catch ([ x ]) { x } | ||
|
|
||
| try {} catch ({ a: { x } }) { x } | ||
| try {} catch ({ a: [ x ] }) { x } | ||
|
|
||
| try {} catch ([{ x }]) { x } | ||
| try {} catch ([[ x ]]) { x } | ||
|
|
||
| try {} catch ({ a: { b: { c: { x }} }}) { x } | ||
|
|
||
|
|
||
| try {} catch ({ x }: any) { x } | ||
| try {} catch ([ x ]: any) { x } | ||
|
|
||
| try {} catch ({ a: { x } }: any) { x } | ||
| try {} catch ({ a: [ x ] }: any) { x } | ||
|
|
||
| try {} catch ([{ x }]: any) { x } | ||
| try {} catch ([[ x ]]: any) { x } | ||
|
|
||
| try {} catch ({ a: { b: { c: { x }} }}: any) { x } | ||
|
|
||
|
|
||
| try {} catch ({ x }: unknown) { x } | ||
| ~ | ||
| !!! error TS2339: Property 'x' does not exist on type '{}'. | ||
| try {} catch ([ x ]: unknown) { x } | ||
| ~~~~~ | ||
| !!! error TS2461: Type 'unknown' is not an array type. | ||
|
|
||
| try {} catch ({ a: { x } }: unknown) { x } | ||
| ~ | ||
| !!! error TS2339: Property 'a' does not exist on type '{}'. | ||
| try {} catch ({ a: [ x ] }: unknown) { x } | ||
| ~ | ||
| !!! error TS2339: Property 'a' does not exist on type '{}'. | ||
|
|
||
| try {} catch ([{ x }]: unknown) { x } | ||
| ~~~~~~~ | ||
| !!! error TS2461: Type 'unknown' is not an array type. | ||
| try {} catch ([[ x ]]: unknown) { x } | ||
| ~~~~~~~ | ||
| !!! error TS2461: Type 'unknown' is not an array type. | ||
|
|
||
| try {} catch ({ a: { b: { c: { x }} }}: unknown) { x } | ||
| ~ | ||
| !!! error TS2339: Property 'a' does not exist on type '{}'. | ||
|
|
145 changes: 145 additions & 0 deletions
145
...elines/reference/destructureCatchClause(strict=false,useunknownincatchvariables=false).js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| //// [destructureCatchClause.ts] | ||
| // These are okay with useUnknownInCatchVariables=false, but not okay with useUnknownInCatchVariables=true. | ||
| try {} catch ({ x }) { x } | ||
| try {} catch ([ x ]) { x } | ||
|
|
||
| try {} catch ({ a: { x } }) { x } | ||
| try {} catch ({ a: [ x ] }) { x } | ||
|
|
||
| try {} catch ([{ x }]) { x } | ||
| try {} catch ([[ x ]]) { x } | ||
|
|
||
| try {} catch ({ a: { b: { c: { x }} }}) { x } | ||
|
|
||
|
|
||
| try {} catch ({ x }: any) { x } | ||
| try {} catch ([ x ]: any) { x } | ||
|
|
||
| try {} catch ({ a: { x } }: any) { x } | ||
| try {} catch ({ a: [ x ] }: any) { x } | ||
|
|
||
| try {} catch ([{ x }]: any) { x } | ||
| try {} catch ([[ x ]]: any) { x } | ||
|
|
||
| try {} catch ({ a: { b: { c: { x }} }}: any) { x } | ||
|
|
||
|
|
||
| try {} catch ({ x }: unknown) { x } | ||
| try {} catch ([ x ]: unknown) { x } | ||
|
|
||
| try {} catch ({ a: { x } }: unknown) { x } | ||
| try {} catch ({ a: [ x ] }: unknown) { x } | ||
|
|
||
| try {} catch ([{ x }]: unknown) { x } | ||
| try {} catch ([[ x ]]: unknown) { x } | ||
|
|
||
| try {} catch ({ a: { b: { c: { x }} }}: unknown) { x } | ||
|
|
||
|
|
||
| //// [destructureCatchClause.js] | ||
| // These are okay with useUnknownInCatchVariables=false, but not okay with useUnknownInCatchVariables=true. | ||
| try { } | ||
| catch (_a) { | ||
| var x = _a.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_b) { | ||
| var x = _b[0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_c) { | ||
| var x = _c.a.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_d) { | ||
| var x = _d.a[0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_e) { | ||
| var x = _e[0].x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_f) { | ||
| var x = _f[0][0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_g) { | ||
| var x = _g.a.b.c.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_h) { | ||
| var x = _h.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_j) { | ||
| var x = _j[0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_k) { | ||
| var x = _k.a.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_l) { | ||
| var x = _l.a[0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_m) { | ||
| var x = _m[0].x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_o) { | ||
| var x = _o[0][0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_p) { | ||
| var x = _p.a.b.c.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_q) { | ||
| var x = _q.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_r) { | ||
| var x = _r[0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_s) { | ||
| var x = _s.a.x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_t) { | ||
| var x = _t.a[0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_u) { | ||
| var x = _u[0].x; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_v) { | ||
| var x = _v[0][0]; | ||
| x; | ||
| } | ||
| try { } | ||
| catch (_w) { | ||
| var x = _w.a.b.c.x; | ||
| x; | ||
| } |
Oops, something went wrong.
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.
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.
why is this needed? How much does it increase the tree-walking that control flow analysis does?
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.
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
anyorunknown,assumeInitializedturns intotrue, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
faster is good 👍🏼
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.
I think it could be faster, even;
assumeInitializedis a huge expression and I think a bit of work could be skipped if some of the more simple conditions were true.