Skip to content
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

Detect usage of block-scoped variables from earlier case blocks #47160

Closed
wants to merge 5 commits into from

Conversation

RyanCavanaugh
Copy link
Member

Fixes #19503

Note that this has the possibility to incur a false positive:

let sHasBeenSet = false;
switch (n) {
    case 0:
        let s = "ok";
        sHasBeenSet = true;
    case 1:
        if (sHasBeenSet) {
            s; // ok by construction
        }
        break;
}

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Looks good, just thinking about ways we can give some more context to the error when possible.

const usageCaseBlock = getAncestor(node, SyntaxKind.CaseClause);
if (usageCaseBlock) {
if (usageCaseBlock.pos > symbol.valueDeclaration.pos) {
error(node, Diagnostics.Variable_0_is_declared_in_a_prior_case_block, symbolToString(symbol));
Copy link
Member

Choose a reason for hiding this comment

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

Add a "'0' is declared here" related span to the valueDeclaration.

@@ -3361,6 +3361,11 @@
"category": "Error",
"code": 2836
},
"Variable '{0}' is declared in a prior case block.": {
Copy link
Member

Choose a reason for hiding this comment

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

case needs to be quoted

Suggested change
"Variable '{0}' is declared in a prior case block.": {
"Variable '{0}' is declared in a prior 'case' block.": {

It's tough to give some more context, but I think it could be helpful. Maybe something like

Suggested change
"Variable '{0}' is declared in a prior case block.": {
"Variable '{0}' is declared in a prior 'case' block and cannot be referenced if that declaration does not execute.": {

@RyanCavanaugh
Copy link
Member Author

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 15, 2021

Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at e223f1b. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@RyanCavanaugh
Copy link
Member Author

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 15, 2021

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at e223f1b. You can monitor the build here.

@fatcerberus
Copy link

FWIW, despite technically being a false positive, I find the sample code in the OP to be... somewhat suspicious, to put it lightly. Fallthrough is a thing, but cases still look like blocks regardless. If I found that in a code review, I'd immediately suggest moving the declaration of s outside of the switch.

@sandersn sandersn added this to Not started in PR Backlog via automation Jan 31, 2022
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Jan 31, 2022
@sandersn
Copy link
Member

@RyanCavanaugh do you want to come back to this? It's been sitting for a long time.

@sandersn sandersn moved this from Waiting on reviewers to Waiting on author in PR Backlog Mar 27, 2023
@RyanCavanaugh
Copy link
Member Author

I'm not super keen on this one to be honest.

PR Backlog automation moved this from Waiting on author to Done Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Use before declaration errors when using let statements within a switch case
5 participants