lint to flag default clauses in enum-like switch statements

[pq]

Avoid default clauses in enum-like switch statements

Good:

  switch (testEnum) {
    case TestEnum.A:
      return '123';
    case TestEnum.B:
      return 'abc';
  }
  // Default here.
  return null;

Bad:

  switch (testEnum) {
    case TestEnum.A:
      return '123';
    case TestEnum.B:
      return 'abc';
    default:
      return null;
  }

Related to #2083, default should be forbidden in enum and enum-like case switches.

[bwilkerson]

There's an interesting wrinkle that we should consider. Prior to Null Safety, switch statements over enums are required to cover all possible values, and that includes null. (Even after Null Safety, if the type of the switch expression is nullable, null will be a valid value and presumably will still need to be handled.) The default clause has often been used to handle nulls.

As written, this line would require the default clause to be replaced by an explicit case null:. That isn't necessarily a bad thing, but I want to make sure that that's understood and the intent of the customer.

The alternative would be to test whether Null Safety is enabled and change the behavior depending on that case. We should also think about whether to generate a lint when Null Safety is enabled and the switch expression has a nullable type.

[pq]

Ah. This is really interesting. Thanks for bringing it up.

cc @jefflim-google for input...

[pq]

Keeping open to discuss NNBD semantics.

[jefflim-google]

Yes, the null case is something we're aware of, and we didn't have any thoughts about 'case null' because I didn't think that was valid in dart? Even testing in dartpad now, I can't write 'case null'.

We're fine if null cases fall through the switch, the concern is mostly to understand the blast radius of enum additions to avoid blind spots.

[bwilkerson]

Wow. It used to produce an error if the null case wasn't covered, and didn't complain about case null.

@scheglov Is this something new with Null Safety that's leaked through, or was it changed independently?

[scheglov]

See 17.9 Switch in the specification. Null is not an instance of the class MyEnum.

// @dart = 2.7

enum MyEnum { a, b }

void f(MyEnum e) {
  switch (e) {
    case MyEnum.a:
      // TODO: Handle this case.
      break;
    case MyEnum.b:
      // TODO: Handle this case.
      break;
    case null:
      // TODO: Handle this case.
      break;
  }
}

[jefflim-google]

I'm unclear from the previous comment if that code is expected to compile or not. The comment Null is not an instance of the class MyEnum comment makes me think it's not expected to compile, but I just wanted to make sure.

Pasting that code above into dartpad (2.7.2) produces the expected:

Case expressions must have the same types, 'null' isn't a 'EnumTest' - line 10

[scheglov]

The way the spec is written, the code with case null has a compile-time error.

[eernstg]

Considering code with null-safety, cf. https://github.com/dart-lang/linter/issues/2084#issuecomment-624086290, I think the rules are in good shape:

@scheglov wrote:

The way the spec is written, the code with case null has a compile-time error.

Right. But if we're considering null-safety there are changes to the rules about switch statements in this section of the null-safety spec into account, in particular:

It is no longer required that the ei evaluate to instances of the same class.

With these updates we can have the following:

enum E { one, two }

main() {
  E? e = E.one;
  switch (e) {
    case E.one: return;
    case E.two: return;
    case null: return;
  }
}

With respect to the null case, it is included for switches over an expression of a nullable type:

If T is Q? where Q is an enum type, it is a warning if the switch does not handle all enum cases and null, either explicitly or via a default.

So the safe style in the null-safety world would be to use the explicit null case when switching over a nullable enum, such that it will be a warning to forget one of the enum values.

This is consistent with a lint that flags default cases for enum switches, both nullable and non-nullable.

[scheglov]

@eernstg Yes, the example in https://github.com/dart-lang/linter/issues/2084#issuecomment-624491318 has no errors when non-nullable experiment is enabled.

My example in https://github.com/dart-lang/linter/issues/2084#issuecomment-624330259 was specifically for legacy code.

In analyzer we have separate logic for verifying switch in legacy libraries, and in opted-in libraries. Is this wrong, and switch in legacy libraries should be verified using the same rules as in opted-in libraries?

[eernstg]

@scheglov wrote:

In analyzer we have separate logic for verifying switch in legacy libraries, and in opted-in libraries

That's fine! Sorry about overinterpreting the degree to which this issue is about null-safety. ;)

[jjoelson]

What's the status of this? Avoiding default: when switching over an enum is something that I'm frequently reminding every developer on our project to avoid, but it crops up again and again and causes bugs. It's a perfect scenario for a lint rule!

[bwilkerson]

Under null safety the default should be flagged as dead code without needing to enable a lint when all of the valid values are explicitly handled. There's an issue open for that work: https://github.com/dart-lang/sdk/issues/45437.

While the example given is one in which an actual enum is being used, the title suggests a different lint for "enum-like" classes. I think we'd want to handle that case as a lint given that all the other checks related to enum-like classes are lints, so I'm going to leave this issue open for that case.

[jjoelson]

Thanks @bwilkerson! Unfortunately my current project hasn't migrated to null-safety yet, and developers keep writing functions like this that get warned for not returning a value when someEnum is null:

enum SomeEnum { first, second }

int someFunction(SomeEnum someEnum) {
  switch(someEnum) {
    case SomeEnum.first:
      return 1;
    case SomeEnum.second:
      return 2;
  }
}

The tendency is to add a default case, but that breaks the enum exhaustive check and causes bugs when a new enum case is added. I'd love a lint that forces them to return a value below the switch statement in order to handle the null case. Sounds like I might be out of luck until we migrate to null safety.

[bwilkerson]

Sorry, but it does seem unlikely that we'll invest the time to create a lint that's only useful in an older version of the language.