Enhancement: [switch-exhaustiveness-check] Turn on requireDefaultForNonUnion by default

[ST-DDT]

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

• I have searched for related issues and found none that match my proposal.
• I have searched the current rule list and found no rules that match my proposal.
• I have read the FAQ and my problem is not listed.

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

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

declare const value: number;
switch (value) {
  case 0:
    return 0;
  case 1:
    return 1;
}

Pass

declare const value: number;
switch (value) {
  case 0:
    return 0;
  case 1:
    return 1;
  default:
    return -1;
}

Additional Info

Based on #7880 (comment)

• feat(eslint-plugin): [switch-exhaustiveness-check] add requireDefaultForNonUnion option #7880 (comment)

Partial previous discussion: #2959

• [switch-exhaustiveness-check] Require default case if discriminant type is not a union #2959

[ST-DDT]

IMO you always want to cover all cases in your switch, either by exhausting all potential cases, or by having a default statement.
If you don't think, that exhausting all potential cases is the right way to go, then why would you enable the rule in the first place?

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

• either implicitly casting the type to more specific type
• or implicitly hide the "else do nothing" branch and thus lead future readers of the code astray, that only these cases exist (especially if all of your other switches are exhaustive).

I did a non-exhaustive-check on this repository for switch statements and they either had an implicit default (by having a return like statement directly after the switch) or an explicit default.

[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?
(I know plenty repositories, that do or do not care about exhaustiveness, but I don't know any that decide that on the type of the value)

[ArnaudBarre]

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.

[Josh-Cena]

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.

[ArnaudBarre]

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.

[JoshuaKGoldberg]

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 typescript-eslint repo by enabling requireDefaultForNonUnion in our root-level ESLint config, we do see a few cases where the option doesn't make sense. Looking at the first two:

• A switch over a string, where there are a couple of cases for known special casing characters:

  typescript-eslint/packages/ast-spec/tests/util/serializers/string.ts

  Line 21 in 09c04fb

  switch (character) {

• A switch over a child node in AST analysis, where we want to run some logic only for a couple kinds of child nodes:

  typescript-eslint/packages/scope-manager/src/referencer/Referencer.ts

  Line 322 in 09c04fb

  switch (left.type) {

In both those cases the code is working fine as-is. There's nothing directly to be gained from adding a default case. One could argue that always having a default in a non-exhaustive switch is helpful stylistically... but that's a stylistic opinion that not everyone would have. I would go for the opposite: less code.

Marking as evaluating community engagement to see if anybody has strong counterpoints here. But unless there's a big swell of user interest, I don't see the arguments in favor of the requireDefaultForNonUnion option existing being powerful enough to justify it being on by default.

I did a non-exhaustive-check on this repository for switch statements and they either had an implicit default (by having a return like statement directly after the switch) or an explicit default.

Aside: I love it when people look at the data for suggestions. This is very helpful to know 😄 thanks!

[ST-DDT]

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.

Aside: I love it when people look at the data for suggestions. This is very helpful to know 😄 thanks!

I hope this gets easier/less time consuming/less patchy with AI in the future.

[ArnaudBarre]

Yeah ok I see and i agree that when you write switch in place of else if it doesn't really lead to bugs to omit the default. So I understand the choice to not enable it by default.

[JoshuaKGoldberg]

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!