Skip to content

Enhancement: [switch-exhaustiveness-check] considerDefaultExhaustiveForUnions only with comment #10251

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.

Sign up for GitHub

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

Jump to bottom
Closed
4 tasks done
FloEdelmann opened this issue Oct 31, 2024 · 8 comments
Closed
4 tasks done

Enhancement: [switch-exhaustiveness-check] considerDefaultExhaustiveForUnions only with comment #10251

FloEdelmann opened this issue Oct 31, 2024 · 8 comments
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@FloEdelmann
Copy link
Contributor

FloEdelmann commented Oct 31, 2024 •
edited
Loading

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/switch-exhaustiveness-check/

Description

I like the new default behavior (considerDefaultExhaustiveForUnions: false), so that I have to explicitly list every union case in a switch statement. However, sometimes there are really big unions where I'd like to rely on the default case implicitly covering all other cases.

I don't want to disable the switch-exhaustiveness-check rule with an ESLint comment, because it will likely be missed and kept when changing the default case, given that it has to be above the switch (…) { line and not above the default: line and is thus out of sight.

Example 
declare const letter: 'A' | 'B' | 'C' | 'D' | 'E' | 'F' | 'G' | 'H' | 'I' | 'J' | 'K' | 'L' | 'M' | 'N' | 'O' | 'P' | 'Q' | 'R' | 'S' | 'T' | 'U' | 'V' | 'W' | 'X' | 'Y' | 'Z';

// eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check -- default case handles letters Q to Z
switch (letter) {
  case 'A': {
    console.log('A');
    break;
  }
  case 'B': {
    console.log('B');
    break;
  }
  case 'C': {
    console.log('C');
    break;
  }
  case 'D': {
    console.log('D');
    break;
  }
  case 'E': {
    console.log('E');
    break;
  }
  case 'F': {
    console.log('F');
    break;
  }
  case 'G': {
    console.log('G');
    break;
  }
  case 'H': {
    console.log('H');
    break;
  }
  case 'I': {
    console.log('I');
    break;
  }
  case 'J': {
    console.log('J');
    break;
  }
  case 'K': {
    console.log('K');
    break;
  }
  case 'L': {
    console.log('L');
    break;
  }
  case 'M': {
    console.log('M');
    break;
  }
  case 'N': {
    console.log('N');
    break;
  }
  case 'O': {
    console.log('O');
    break;
  }
  case 'P': {
    console.log('P');
    break;
  }
  default: {
    console.log('Other');
  }
}

Instead, I propose adding a new option value for considerDefaultExhaustiveForUnions (and maybe even making it the default instead of false): 'onlyWithComment'
With this option, a default case on its own would still not be considered exhaustive, but adding a comment right before the default case would silence the rule.

Example 
declare const letter: 'A' | 'B' | 'C' | 'D' | 'E' | 'F' | 'G' | 'H' | 'I' | 'J' | 'K' | 'L' | 'M' | 'N' | 'O' | 'P' | 'Q' | 'R' | 'S' | 'T' | 'U' | 'V' | 'W' | 'X' | 'Y' | 'Z';

switch (letter) {
  case 'A': {
    console.log('A');
    break;
  }
  case 'B': {
    console.log('B');
    break;
  }
  case 'C': {
    console.log('C');
    break;
  }
  case 'D': {
    console.log('D');
    break;
  }
  case 'E': {
    console.log('E');
    break;
  }
  case 'F': {
    console.log('F');
    break;
  }
  case 'G': {
    console.log('G');
    break;
  }
  case 'H': {
    console.log('H');
    break;
  }
  case 'I': {
    console.log('I');
    break;
  }
  case 'J': {
    console.log('J');
    break;
  }
  case 'K': {
    console.log('K');
    break;
  }
  case 'L': {
    console.log('L');
    break;
  }
  case 'M': {
    console.log('M');
    break;
  }
  case 'N': {
    console.log('N');
    break;
  }
  case 'O': {
    console.log('O');
    break;
  }
  case 'P': {
    console.log('P');
    break;
  }
  // default handles letters Q to Z
  default: {
    console.log('Other');
  }
}

Fail

declare const literal: 'a' | 'b';

switch (literal) {
  case 'a':
    break;
  default:
    break;
}

Pass

declare const literal: 'a' | 'b';

switch (literal) {
  case 'a':
    break;
  // all other cases
  default:
    break;
}

Additional Info

No response
@FloEdelmann FloEdelmann added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Oct 31, 2024
@FloEdelmann
Copy link
Contributor Author

FloEdelmann commented Oct 31, 2024 •
edited
Loading

Alternatively, the error could be reported not on the switch node itself, but on the default node, if it exists and considerDefaultExhaustiveForUnions is false. Then the ESLint disable comment would be right at the same location as I suggested.

@auvred
Copy link
Member

auvred commented Nov 1, 2024

Alternatively, the error could be reported not on the switch node itself, but on the default node, if it exists and considerDefaultExhaustiveForUnions is false. Then the ESLint disable comment would be right at the same location as I suggested.

I think this is a better idea because if we have to choose between two comment options:

  1. a non-standard way that exists only in this particular rule
  2. the well-known way of suppressing ESLint errors with eslint-disable.

IMO, option (2) is better, if only because most ESLint users are familiar with it. Besides, I'm sure that even if we adopt option (1), most rule users will still put the eslint-disable directive on the whole switch (...) {...} block because they haven't read about this feature of the rule or will forget about it, since it's a really rare pattern among other rules.

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Nov 1, 2024 •
edited
Loading

I don't want to disable the switch-exhaustiveness-check rule with an ESLint comment, because it will likely be missed and kept when changing the default case, given that it has to be above the switch (…) { line and not above the default: line and is thus out of sight.

Sorry, I'm not quite wrapping my head around this. Could you elaborate slightly?

@FloEdelmann
Copy link
Contributor Author

FloEdelmann commented Nov 4, 2024 •
edited
Loading

When there are many or long cases above the default case, the // eslint-disable-next-line comment above the switch line is far away from the default line. Then assume there is a refactoring and the default case is removed. Since the rule is disabled for the whole switch statement, no lint issue is reported and thus cases are missed.

If the comment had been above the default case, this would be noticed (either both the default case and the disable comment would be removed together, or the default case is removed and then the disable comment is reported as unused). Also, since there is no default case anymore then, the lint error reporting the now non-exhausive switch statement would again be reported in the switch line.

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Nov 8, 2024 •
edited
Loading

Ok, I hear what you're saying, but I'm -1 on moving the report location. I don't think you can really safely do any operation to a switch statement without knowing what the switched value is, so you generally have to be aware of what the head of the switch statement looks like anyway, especially for adding or removing cases, of all things.

I think I could just as well say "what if I did a refactoring and removed the first case, which I thought was unused" and didn't see the eslint-disable case all the way down on the default instead of at the top of the switch where I was working.

My gut says the bug-preventing potential of the report location being on the default (if a default is even present, which it need not be) instead of the of the top of the switch is marginal at best, and is substantially outweighed by the inconsistency and inconvenience of having the report for the same complaint move to a different place when you add or remove a default case.

@bradzacher
Copy link
Member

I'm personally -1 as well.
I also think that it's better reporting on the switch (x) because it is a lot more intuitive to investigate the missing cases by hovering the switched variable.
If it's on the default then you could be a ways away from that.

I think no matter where you report there are going to be trade-offs.

@JoshuaKGoldberg JoshuaKGoldberg added evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important and removed triage Waiting for team members to take a look labels Dec 7, 2024
@JoshuaKGoldberg
Copy link
Member

The switch-exhaustiveness-check rule is getting bigger by the season 🫤. If we keep up this rate of options expansion, it'll have a dozen configurability options by 2026. I'm slowly becoming more -1 on adding additional nuanced options to the rule. Also more -1 on adding more changes for nuanced features like this one.

But hey, maybe this is a big useful thing more folks will be asking about now that considerDefaultExhaustiveForUnions is in? Marking as evaluating community engagement.

@JoshuaKGoldberg
Copy link
Member

It's been >4 months without any new engagement. The team is still not in favor of growing the rule's complexity in this way. Closing out as wontfix for tseslint.

Note that you can always fork and/or rewrite the rule. https://typescript-eslint.io/developers has docs on writing your own rules and plugins.

Cheers! 💖

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Apr 21, 2025
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Apr 29, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@FloEdelmann @JoshuaKGoldberg @bradzacher @kirkwaiblinger @auvred