-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Enhancement: [switch-exhaustiveness-check] Turn on requireDefaultForNonUnion by default #7966
Comments
IMO you always want to cover all cases in your switch, either by exhausting all potential cases, or by having a default statement. If a type doesn't have type information or overly broad type information (such as number) and the switch doesn't have a default case, then you are
I did a non-exhaustive-check on this repository for [curious]: Can somebody please point me to a real piece of code/repository, that has a non-exhaustive-switch on a non-union-type, but has otherwise most of their union-type switches exhaustive? |
Thanks for creating the issue to discuss this. My personal opinion is that the option should be removed (and "always on"). I'm pretty sure that if the rule was created first with this check nobody will have ask to create this intermediate check. |
We cannot cause extra errors in minor versions, unless the error is highly expectable. People may not expect non-union types to be checked at all. Of course, unless other team members disagree with me, but I think this is sufficiently debatable that it can only be enabled by default (or held up for removal) in the next major version. |
Yeah I know that this is the current consensus that updates should not help you catch new bugs unless you read carefully the changelog to enable new flags, I'm just giving my opinion by phrasing it this way. |
Yup happy to talk about this as a breaking change! My main concern here is that doing so is very strict for a case that doesn't directly catch bugs. It just often indirectly catches them (which is why folks are in favor of it existing!). We (linters) have learned the hard way that if you add a behavior that "just" restricts some code pattern without provably showing the bug it fixed, that'll annoy folks. You need to really be able to show that it's >99% of the time definitely useful immediately. Again, not saying this issue's proposal is wrong, just that we've learned it's very hard to put changes like this in front of people. Trying this out in the
In both those cases the code is working fine as-is. There's nothing directly to be gained from adding a Marking as
Aside: I love it when people look at the data for suggestions. This is very helpful to know 😄 thanks! |
I see why you consider this stylistic. IMO both examples are bad switches because they only operate on a very tiny subset of the available data and might take just as much characters as written as an if statement (and then they wouldn't need a comment regarding their codeflow). But I see why you don't want to add extra characters to any stylistic choice.
I hope this gets easier/less time consuming/less patchy with AI in the future. |
Yeah ok I see and i agree that when you write switch in place of |
It's been ~4 months since last comment and there haven't been any new motivators for this. Closing it out as having not had enough community feedback to suggest we should take action. If anybody sees this and thinks they have a good reason not yet mentioned, please do file a new issue! We'd be happy to take a look. Thanks for the discussion all! |
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Link to the rule's documentation
https://typescript-eslint.io/rules/switch-exhaustiveness-check/
Description
This issue is there to discuss whether the
requireDefaultForNonUnion
option should be enabled by default.This issue is not about whether the
switch-exhaustiveness-check
should be part of any preset or the option to be removed entirely.Fail
Pass
Additional Info
Based on #7880 (comment)
Partial previous discussion: #2959
The text was updated successfully, but these errors were encountered: