feat(eslint-plugin): [switch-exhaustiveness-check] add an option to warn against a default case on an already exhaustive switch

[Zamiell]

PR Checklist

• Addresses an existing open issue: fixes [switch-exhaustiveness-check] ban the default case to enforce all cases are handled and prevent accidental mishandling of new cases #3616
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

I added a second check to the switch-exhaustiveness-check rule. The docs have been updated to say:

Additionally, this also reports when a switch statement has a case for everything in a union and also contains a default case. default cases in this situation are obviously superfluous, as they would contain dead code. But beyond being superfluous, these kind of default cases are actively harmful: if a new value is added to the switch statement union, the default statement would prevent this rule from alerting you that you need to handle the new case.

Currently, the rule has no options. Rather than adding a new option for this behavior, I decided that it should not be configurable, because I suspect that everyone will want to have this feature on by default and never turn it off.

More Details

Currently, the switch-exhaustiveness-check rule works like this:

1. if the thing switching on is not a union, early return
2. if the switch statement has a default case, early return
3. if missing branches > 0, trigger the rule

I refactored the file such that the logic flow is now:

1. if the thing switching on is not a union, early return
2. gather metadata about the switch statement (which mainly includes the missing branches)
3. check switchIsNotExhaustive (fed with the metadata)
4. check dangerousDefaultCase (fed with the metadata)

[typescript-eslint]

Thanks for the PR, @Zamiell!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	fbc3f3e
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6577c1da1c4e5b0008d20a2a
😎 Deploy Preview	https://deploy-preview-7539--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 96 (🔴 down 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 5709fe3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

🟥 Failed Commands
nx test eslint-plugin --coverage=false

✅ Successfully ran 18 targets

• nx test eslint-plugin-tslint --coverage=false
• nx run types:build
• nx test rule-schema-to-typescript-types --coverage=false
• nx test visitor-keys --coverage=false
• nx test type-utils --coverage=false
• nx run repo-tools:postinstall-script
• nx test utils --coverage=false
• nx test repo-tools --coverage=false
• nx run-many --target=typecheck --parallel
• nx test typescript-estree --coverage=false
• nx test ast-spec --coverage=false
• nx test parser --coverage=false
• nx test scope-manager --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx run-many --target=lint --parallel
• nx run integration-tests:test
• nx run eslint-plugin:generate:configs
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint

---

Sent with 💌 from NxCloud.

[Josh-Cena]

Should we exempt the common case where the default case is a single throw statement?

[Zamiell]

Why would we exempt that? That's super dangerous / bad code for exactly the reasons that I put into the documentation.

[Josh-Cena]

The reason you gave is that this is unreachable code and may obfuscate future extensions. But here the point is exactly to add an extra runtime assertion against unhandled extensions.

[Zamiell]

The whole point of using TypeScript is that developers can uncover bugs in the code at compile-time instead of end-users discovering bugs in the code at run-time. Finding the switch statement bug at run-time seems completely unacceptable.

[bradzacher]

Not a full review - just a quick comment.
I think we will have to put this behind an option.
I know a lot of people also turn on the default-case rule (even though they shouldn't really use both rules together).

And if we ship this as is then those users would now be unable to satisfy the linter (as this rule would force them to remove the default, then default-case would force them to re-add it).

There's a whole thought experiment here about how one could indicate to the rule whether or not a switch is meant to be exhaustive or not.. maybe that's a future extension for this rule. (eg a comment above the switch)
But for now we'll need to gate this behaviour

[Zamiell]

I know a lot of people also turn on the default-case rule (even though they shouldn't really use both rules together).
And if we ship this as is then those users would now be unable to satisfy the linter (as this rule would force them to remove the default, then default-case would force them to re-add it).

Can you expand more on why this is a requirement? Why do we care if a typescript-eslint rule conflicts with a core ESLint rule, especially when that core ESLint rule goes against TypeScript best-practices? Is there some design principle of typescript-eslint such that all of its rules should not conflict with base ESLint rules?

Specifically, in this case I don't think the functionality should be behind an option because it prevents an actively harmful piece of code. I'll role-play the hypothetical you describe, and you can tell me where it diverges from your expectations:

• In another routine day at the office, Alice updates the dependencies in her TypeScript project. However, she then notices some new errors from ESLint.
• Alice realizes that the new errors are because of an update to the switch-exhaustiveness-check rule.
• The lint rule advises her to delete the superfluous default case in a switch statement, so she does.
• However, frustratingly, doing that causes another ESLint rule to be triggered, the default-case rule.
• At this point, there is no quick fix, and Alice knows that she has to do a little bit more research to find out what is really going on:
  • Alice knows that the switch-exhaustiveness-check rule comes from the typescript-eslint team and that the default case rule comes from the core ESLint team. The former represents best-practices in the TypeScript ecosystem, and the latter is more generalized.
  • After reading the documentation for the switch-exhaustiveness-check rule, she understands more in depth what is going on - that the default case that the rule was advising her to delete was in actuality actively harmful, and that the lint rule suggestion to delete it was amazingly good.
  • Furthermore, reading the documentation gives her some insight into why default-case is a terrible rule to turn on in TypeScript codebases.
• Alice now has the knowledge to safely turn off the default-case rule for the project (and all other TypeScript projects she manages).
• Alice is thankful for the helpful nod in the right direction from the typescript-eslint team, as she would have never realized that the default-case lint rule was actively harmful otherwise.

[Zamiell]

@JoshuaKGoldberg @bradzacher Pinging, since a month has passed with no reply.

[bradzacher]

This should be behind an option because not all codebases will want this behaviour.
As an example - at Meta we had a concept called "the safety window".
This window was the time during which a user may be on old JS code talking to a BE that is on newer code.
This concept existed because it was the time during which your code could break if it wasn't written in a way to be forwards-compatible.

As an example - imagine that you have an endpoint that returns a value of wither 0 | 1 - we'll call this "v1".
On Monday morning the user loads your web app and starts using it.
On Monday at lunch time you ship a change which causes the endpoint to to instead output 0 | 1 | 2. We'll call this "v2".

If the user does not refresh their webpage - then they will be on "v1" of your JS code talking to "v2" of your backend.
If your code is written like this:

async function foo(): string {
  const value = await getValueFromBackend();
  switch (value) {
    case 0:
      return doSomething();

    case 1:
      return doSomeOtherThing();
  }
}

Then it means that your code will now behave in unexpected ways - most likely it will crash in some way, but it might also silently work correctly but do something fucky.

Either way - this is bad! So instead at Meta every switch statement that deals with API data is enforced to be written with a default - EVEN IF IT IS EXHAUSTIVE. This style means that you must explicitly handle the forwards-compatibility case - even if that handling means causing an explicit crash.

async function foo(): string {
  const value = await getValueFromBackend();
  switch (value) {
    case 0:
      return doSomething();

    case 1:
      return doSomeOtherThing();

    default:
      throw new Error("You're on outdated code - please refresh to update!");
  }
}

An explicit crash is better than an implicit one because it means you know for certain where and how your code will terminate - rather than potentially things continuing with and operating on corrupted data.
I.e. imagine if you were editing a google doc, added a new line and the page crashed. Then when you refreshed you found your entire doc was corrupted and lost forever because you'd been working with v1 JS code against a v2 backend. As a user you would be LIVID. With an explicit error the app could have crashed out before it saved corrupted data and by refreshing your data would have been uncorrupted and recovered (maybe with some small data-loss).

By creating this functionality without an option - a company that follows the above code style would no longer be able to use the rule at all - it would have to be turned off for their entire codebase!

This is really bad!!!
Fun fact this is why flow enums have special syntax to define "an enum with unknown members" - so that the API type generators can generate all API enums with this to allow lint rules to enforce switches are both exhaustive AND include a default case.

---

Another use cases is that not every switched value will be a union/enum value - sometimes they might just be string or number! For these cases a rule like default-case can be helpful to enforce that people are explicitly encoding their assumptions into their code.

However there's no way to make the rule just apply specifically to switches that deal with non-union-type values - so it's on for everything.

So if this change isn't behind an option - that sort of user is now in the aforementioned unresolvable state! They want to use default-case for their non-union-type switches, but they cannot use it because this rule conflicts with it.

---

There are many usecases to consider! There's no harm in adding it behind an option.

[Zamiell]

Thanks for the explanation Brad. I will work on putting it behind an option today.

I am curious though: at Meta, after updating a V1 union to a V2 union, is there a way use tooling somehow to find all the switch statements in a codebase that become out of date and throw an error in CI? (Normally, using the switch-exhastiveness-check rule helps with this, but with all of the mandatory default cases, the rule becomes useless.)

[Zamiell]

To partially answer my question in the previous post, I am now recalling that previous to the switch-exhaustiveness-check rule existing, I used an assertUnreachable helper function as described on StackOverflow. So should the docs be updated to recommend this solution specifically for API switch statements?

[Zamiell]

I addressed all of the changes now that Joshua requested. Additionally, CI seems to be failing, but it's just failing for the same reason that its failing on main. If you fix CI on main, then I can click the "Update branch" button, and I think this PR would pass.

[JoshuaKGoldberg]

This is looking great, thanks so much for all your hard work on it! ❤️‍🔥

Marking as approved from my end. Only a couple of comments, neither are particularly important to me. Just there if you find them useful.

I'll wait a bit for a review from @bradzacher since he also was looking at this. But if Brad doesn't have time, no worries, we can merge.

[JoshuaKGoldberg]

[Docs] What do you think about providing a correct / incorrect tab here? I believe most options these days do, and they're useful for users.

I separately posted #8014 (comment) that we might eventually lint/test to enforce this practice. So no worries if you don't have the time or a good example isn't quick to write. Non-blocking IMO 🙂

[JoshuaKGoldberg]

Filed #8154 as a followup 👍