Skip to content

fix(47383): Destructuring of unknown catch variable is not an error #47442

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

Closed
wants to merge 0 commits into from

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #47383

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Jan 14, 2022
@jakebailey
Copy link
Member

This PR does pass tests, but I'm a bit confused as to why it doesn't work when I run the build via tsserver locally in VS Code; do these sorts of checks need to be somewhere else, maybe? Some missing case in another function?

@jakebailey
Copy link
Member

Hm, apparently checkTryStatement doesn't actually call a check function on the catch clause / its variable declaration. That doesn't seem right.

@jakebailey
Copy link
Member

Yeah, I think checkTryStatement needs checkVariableLikeDeclaration(declaration), but that seems to cause some baseline changes including follow-on errors about implicit any, which are probably not right.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as needing changes (meant to do this with my last comment), as this doesn't seem to work in VS Code (and probably other editors), so we'll need to make sure that check call is added. Not entirely certain how to ensure the tests actually walk that; maybe they already check the opposite?

@a-tarasyuk a-tarasyuk force-pushed the fix/47383 branch 3 times, most recently from 18988f3 to 40964a3 Compare February 1, 2022 10:29
@a-tarasyuk
Copy link
Contributor Author

@jakebailey Thanks for the review and sorry for the delay in response. I've added changes that cover the following cases

// @strict: true
// @useUnknownInCatchVariables: true

try {
    // ...
}
catch ({ name }) {
         ^^^^ Property 'name' does not exist on type 'unknown'
         // { name } has 'unknown' type because @useUnknownInCatchVariables: true
    name; 
}

try {
    // ...
}
catch ({ name } : unknown) {
         ^^^^ Property 'name' does not exist on type 'unknown'
         // { name } has explicit type 'unknown'
    name;
}
// @strict: true
// @useUnknownInCatchVariables: false

try {
    // ...
}
catch ({ name }) {
        // Ok
        // { name } has 'any' type because @useUnknownInCatchVariables: false
    name; 
}

try {
    // ...
}
catch ({ name } : unknown) {
         ^^^^ Property 'name' does not exist on type 'unknown'
         // { name } has explicit type 'unknown'
    name;
}
// @strict: false
// @useUnknownInCatchVariables: true

try {
    // ...
}
catch ({ name }) {
         ^^^^ Property 'name' does not exist on type '{}' 
         // { name } has '{}' type because @useUnknownInCatchVariables: true
         // `unknown` is represented as `{}` with disabled `strict: false` check
    name; 
}

try {
    // ...
}
catch ({ name } : unknown) {
         ^^^^ Property 'name' does not exist on type `{}`
         // { name } has explicit type 'unknown'
    name;
}
// @strict: false
// @useUnknownInCatchVariables: true

try {
    // ...
}
catch ({ name }) {
         // Ok
         // { name } has 'any' type because @useUnknownInCatchVariables: false
    name; 
}

try {
    // ...
}
catch ({ name } : unknown) {
         ^^^^ Property 'name' does not exist on type `{}`
         // { name } has explicit type 'unknown'
    name;
}

Is this the expected behavior? Or do we need to cover other cases?

@jakebailey
Copy link
Member

jakebailey commented Feb 1, 2022

Unless I'm mistaken, all of the examples above should have name be of type any (versus unknown), but that's what the PR has (so maybe your comment is wrong), so that's still good and consistent with other restructuring.

It looks like things are still not quite right in the editor, though. If you use your test case but don't reference the variable in question, it still doesn't show an error. Compare the three cases here, each with the destructured variable left referenced.

const { xyz }: unknown = {};

function foo({ name }: unknown) {
    // ...
}

try {
    // ...
}
catch ({ name }: unknown) {
    // ...
}

image

I'm not sure what you've changed to get further is correct, though; it still feels to me like there's something missing during the checker walk, and these errors are only functioning due to the side effect of evaluating the variable later.

@a-tarasyuk a-tarasyuk marked this pull request as draft February 1, 2022 20:41
@a-tarasyuk a-tarasyuk force-pushed the fix/47383 branch 2 times, most recently from 474027f to 5599944 Compare February 3, 2022 08:34
@a-tarasyuk a-tarasyuk marked this pull request as ready for review February 3, 2022 09:03
@a-tarasyuk a-tarasyuk requested a review from jakebailey February 3, 2022 09:03
@sandersn
Copy link
Member

I'm confused about the state of this PR. Why is it marked draft? If a team member signs off, is it OK to merge? If so, I guess that means it needs another review?

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Apr 1, 2022

@sandersn I'm really sorry for the confusion with the state of the PR. I've closed it to make this issue open to PR acceptance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Destructuring of unknown catch variable is not an error
5 participants