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

[no-fallthrough] Lint rule needs type information to avoid false positives e.g. nested switch #3455

Open
3 tasks done
OliverJAsh opened this issue May 28, 2021 · 3 comments
Open
3 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new base rule extension New base rule extension required to handle a TS specific case package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@OliverJAsh
Copy link
Contributor

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "no-fallthrough": [2]
  }
}
declare const n: { a: 1; b: 1 | 2 } | { a: 2 };
(() => {
  switch (n.a) {
    case 1: {
      switch (n.b) {
        case 1:
          return 1;
        case 2:
          return 1;
      }
    }
    // ESLint error:
    // Expected a 'break' statement before 'case'.
    case 2: {
      return 2;
    }
  }
})();

Expected Result

No lint error

Actual Result

Lint error (see comment)

Additional Info

I think the TS ESLint plugin needs to provide its own version of no-fallthrough in order to take advantage of type information so we can avoid false positives such as this.

Versions

package version
@typescript-eslint/eslint-plugin latest
@typescript-eslint/parser latest
TypeScript latest
ESLint latest
node latest
@OliverJAsh OliverJAsh added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 28, 2021
@bradzacher bradzacher added enhancement: new base rule extension New base rule extension required to handle a TS specific case and removed triage Waiting for maintainers to take a look labels May 28, 2021
@BobNobrain
Copy link

Another example of code with false positive no-fallthrough error due to lack of information about types:

switch(x) {
    case 0:
        process.exit(0);
        // putting `break` here silents no-fallthrough, but causes unreachable code warning from ts

    default:
        // ...
}

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@wclr
Copy link

wclr commented Feb 5, 2022

Wanted to make new rule proposal but then found this. So core no-fallthrough requires an unneeded break, even if a nested switch case is exhaustive. But exhaustiveness of the case can be detected only with types.

Such code with core `no-fallthrough enabled requires unneeded break:

const fn = (x: "A" | "B", n: "Err" | "Ok"): number => {
  switch (x) {
    case "A":
      switch (n) {
        case "Ok":
          return 1
        
        case "Err":
          return 0  
        
      }
    break; // this break actually not needed with typescript 
    default:
      return -1
  }
}

image

TS config actually has the option noFallthroughCasesInSwitch, which almost checks the same thing, but error reporting is a bit scarce.

@dani-mp
Copy link

dani-mp commented Sep 28, 2023

Had this problem today with a nested switch. The formatter removes the unnecessary breaks. And eslint complains about them being expected.

@bradzacher bradzacher removed the breaking change This change will require a new major version to be released label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new base rule extension New base rule extension required to handle a TS specific case package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

6 participants