-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [switch-exhaustiveness-check] add an option to warn against a default
case on an already exhaustive switch
#7539
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
Conversation
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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Should we exempt the common case where the default case is a single throw statement? |
Why would we exempt that? That's super dangerous / bad code for exactly the reasons that I put into the documentation. |
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
default
case on an already exhaustive switch
Can you expand more on why this is a requirement? Why do we care if a 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:
|
@JoshuaKGoldberg @bradzacher Pinging, since a month has passed with no reply. |
This should be behind an option because not all codebases will want this behaviour. As an example - imagine that you have an endpoint that returns a value of wither If the user does not refresh their webpage - then they will be on "v1" of your JS code talking to "v2" of your backend. 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 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. 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!!! Another use cases is that not every switched value will be a union/enum value - sometimes they might just be However there's no way to make the rule just apply specifically to So if this change isn't behind an option - that sort of user is now in the aforementioned unresolvable state! They want to use There are many usecases to consider! There's no harm in adding it behind an option. |
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 |
To partially answer my question in the previous post, I am now recalling that previous to the |
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There is also an option to check the exhaustiveness of switches on non-union types by requiring a default clause. | ||
## Options | ||
|
||
### `"allowDefaultCaseForExhaustiveSwitch"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #8154 as a followup 👍
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
…arn against a `default` case on an already exhaustive `switch` (typescript-eslint#7539) * feat: switch-exhaustiveness-check checks for dangerous default case * fix: spelling * fix: comment * fix: docs * feat: allowDefaultCase option * fix: tests * fix: lint * fix: prettier * refactor: finish merge * fix: format * fix: lint * chore: update docs * chore: update docs * chore: format * fix: test * fix: tests * fix: tests * fix: tests * fix: test * fix: test * fix: tests * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * fix: double options in docs * Update packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * feat: simplify code flow * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * fix: grammar * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * fix: wording on option * Update packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * docs: add playground link * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * chore: add punctuation * Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * chore: remove comment * refactor: rename option * fix: prettier * fix: lint * fix: tests * refactor: better metadata * fix: tests * refactor: rename interface * refactor: make interface readonly --------- Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
PR Checklist
Overview
I added a second check to the
switch-exhaustiveness-check
rule. The docs have been updated to say: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:I refactored the file such that the logic flow is now:
switchIsNotExhaustive
(fed with the metadata)dangerousDefaultCase
(fed with the metadata)