Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(eslint-plugin): [switch-exhaustiveness-check] add an option to warn against adefault case on an already exhaustiveswitch#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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
JoshuaKGoldberg merged 50 commits intotypescript-eslint:mainfromIsaacScript:switch
Dec 29, 2023

Conversation

Zamiell
Copy link
Contributor

@ZamiellZamiell commentedAug 26, 2023
edited
Loading

PR Checklist

Overview

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

Additionally, this also reports when aswitch statement has a case for everything in a union andalso contains adefault case.default cases in this situation are obviously superfluous, as they would contain dead code. But beyond being superfluous, these kind ofdefault cases are actively harmful: if a new value is added to the switch statement union, thedefault 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, theswitch-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. checkswitchIsNotExhaustive (fed with the metadata)
  4. checkdangerousDefaultCase (fed with the metadata)

@typescript-eslint
Copy link
Contributor

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 onhttps://opencollective.com/typescript-eslint.

@netlify
Copy link

netlifybot commentedAug 26, 2023
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitfbc3f3e
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/6577c1da1c4e5b0008d20a2a
😎 Deploy Previewhttps://deploy-preview-7539--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
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 yourNetlify site configuration.

@nx-cloud
Copy link

nx-cloudbot commentedAug 26, 2023
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit5709fe3. 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

Sent with 💌 fromNxCloud.

@Josh-Cena
Copy link
Member

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

@Zamiell
Copy link
ContributorAuthor

Zamiell commentedSep 4, 2023
edited
Loading

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

@Josh-Cena
Copy link
Member

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
Copy link
ContributorAuthor

Zamiell commentedSep 4, 2023
edited
Loading

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.

Copy link
Member

@bradzacherbradzacher left a comment
edited
Loading

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 thedefault-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 thedefault, thendefault-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

@bradzacherbradzacher added the awaiting responseIssues waiting for a reply from the OP or another party labelSep 8, 2023
@bradzacherbradzacher changed the titlefeat: switch-exhaustiveness-check checks for dangerous default casefeat(eslint-plugin): [switch-exhaustiveness-check] add an option to warn against adefault case on an already exhaustiveswitchSep 8, 2023
@bradzacherbradzacher added the enhancement: plugin rule optionNew rule option for an existing eslint-plugin rule labelSep 8, 2023
@Zamiell
Copy link
ContributorAuthor

Zamiell commentedSep 8, 2023
edited
Loading

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 atypescript-eslint rule conflicts with a core ESLint rule, especially when that core ESLint rule goes against TypeScript best-practices? Is there some design principle oftypescript-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 theswitch-exhaustiveness-check rule.
  • The lint rule advises her to delete the superfluousdefault case in a switch statement, so she does.
  • However, frustratingly, doing that causes another ESLint rule to be triggered, thedefault-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 theswitch-exhaustiveness-check rule comes from thetypescript-eslint team and that thedefault 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 theswitch-exhaustiveness-check rule, she understands more in depth what is going on - that thedefault 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 whydefault-case is a terrible rule to turn on in TypeScript codebases.
  • Alice now has the knowledge to safely turn off thedefault-case rule for the project (and all other TypeScript projects she manages).
  • Alice is thankful for the helpful nod in the right direction from thetypescript-eslint team, as she would have never realized that thedefault-case lint rule was actively harmful otherwise.

@Zamiell
Copy link
ContributorAuthor

Zamiell commentedOct 3, 2023
edited
Loading

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

@bradzacher
Copy link
Member

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 wither0 | 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 output0 | 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:

asyncfunctionfoo():string{constvalue=awaitgetValueFromBackend();switch(value){case0:returndoSomething();case1:returndoSomeOtherThing();}}

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 everyswitch statementthat deals with API data is enforced to be written with adefault -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.

asyncfunctionfoo():string{constvalue=awaitgetValueFromBackend();switch(value){case0:returndoSomething();case1:returndoSomeOtherThing();default:thrownewError("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 beLIVID. 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 ruleat all - it would have to be turned off for their entire codebase!

This is really bad!!!
Fun fact this is whyflow 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 adefault case.


Another use cases is that not every switched value will be a union/enum value - sometimes they might just bestring ornumber! For these cases a rule likedefault-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 toswitches 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 usedefault-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.

SimonSimCity reacted with eyes emoji

@Zamiell
Copy link
ContributorAuthor

Zamiell commentedOct 12, 2023
edited
Loading

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 theswitch-exhastiveness-check rule helps with this, but with all of the mandatory default cases, the rule becomes useless.)

@Zamiell
Copy link
ContributorAuthor

Zamiell commentedOct 12, 2023
edited
Loading

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

@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelDec 7, 2023
@Zamiell
Copy link
ContributorAuthor

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
JoshuaKGoldberg previously approved these changesDec 12, 2023
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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"`

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 🙂

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 👍

@JoshuaKGoldbergJoshuaKGoldberg requested review frombradzacher and removed request forbradzacherDecember 12, 2023 01:52
@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelDec 12, 2023
@JoshuaKGoldbergJoshuaKGoldberg merged commit6a219bd intotypescript-eslint:mainDec 29, 2023
@ZamiellZamiell deleted the switch branchDecember 29, 2023 18:20
auvred pushed a commit to auvred/typescript-eslint that referenced this pull requestDec 29, 2023
…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.tsCo-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>* fix: double options in docs* Update packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdCo-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>* feat: simplify code flow* Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.tsCo-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>* fix: grammar* Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.tsCo-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>* fix: wording on option* Update packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdCo-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>* docs: add playground link* Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.tsCo-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>* chore: add punctuation* Update packages/eslint-plugin/src/rules/switch-exhaustiveness-check.tsCo-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>
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJan 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg left review comments

@bradzacherbradzacherAwaiting requested review from bradzacher

Assignees
No one assigned
Labels
1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we mergeenhancement: plugin rule optionNew rule option for an existing eslint-plugin rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[switch-exhaustiveness-check] ban the default case to enforce all cases are handled and prevent accidental mishandling of new cases
4 participants
@Zamiell@Josh-Cena@bradzacher@JoshuaKGoldberg

[8]ページ先頭

©2009-2025 Movatter.jp