Sdk: Poor message when hinting about a missing return statement when used with a switch statement and enum

[devoncarew]

The following code produces a hint (This function declares a return type of 'int', but does not end with a return statement):

enum TestEnum {
  VAL1,
  VAL2
}

int f(TestEnum val) {
  switch (val) {
    case TestEnum.VAL1:
      return 0;
    case TestEnum.VAL2:
      return 1;
  }
}

but the switch statement covers all enum values; the method always returns a result.

(copied from https://github.com/dart-atom/dartlang/issues/1067)

[bwilkerson]

null is also a valid value for val, in which case the method will fall off the end and return an implicit null, which might not be what the user intended.

NNBD would solve this problem.

[skybrian]

Perhaps there should be a lint that explains the issue?

[bwilkerson]

I'd rather just improve the message produced by the hint.

[devoncarew]

/cc @mehmetf, for @bwilkerson's explanation

[bergwerf]

I have this issue as well. Adding a default means adding unreachable code and hurts my coverage. I don't see why switch statements of enums should have a default.

[bwilkerson]

It wouldn't be unreachable in the example above: just pass null in to the method f and the default case would be executed.

As it was written above, passing in an argument of null would cause the function to fall off the end, resulting in an implicit return of null. This particular hint attempts to find places where you explicitly specify a return type but could implicitly return null, which this code does.

[jakemac53]

Given that this is only a hint, I think it would be reasonable to simply not consider the null case. Nobody in dart writes null checks for every single parameter, it would be far to cumbersome to do. The general assumption is that you are not passed nulls.

This hint actually encourages bad practices as it stands now, because it encourages people to use the default case to silence the hint. This hides the actual useful hint that ensures you are handling all the possible enum values.

What is the recommendation for silencing this? Add a throw at the end of the body? Return null at the end of the body? Use // ignore? None of those give any improvement imo in terms of the code.

[bergwerf]

Right now I often throw an exception after the switch statement. Although to me this is not the perfect solution since the code is practically unreachable (and therefore will show up as missing lines in coverage reports). I would find it ideal if the analyzer could detect an assertion before the switch where it is asserted that the value is not null, but perhaps that kind of behavior is too specific for the analyzer.

[zoechi]

See also dart-lang/linter#1139, dart-lang/linter#1141

[jtlapp]

I also just bumped into this problem. Here's how I'm dealing with it:

enum AppTab { Items, ItemSets, Reminders }

  int _tabToInt(AppTab tab) {
    switch (tab) {
      case AppTab.Items: return 0;
      case AppTab.ItemSets: return 1;
      case AppTab.Reminders: return 2;
    }
    throw FallThroughError();
  }

Will this problem go away once we have non-nullables?

[bwilkerson]

Yes. If the enum type is non-nullable then we should be able to know that null isn't a possibility and that everything else has been covered.

[srawlins]

But in general, the issue can still be reported, even after the Null Safety type system.

enum TestEnum {
  VAL1,
  VAL2
}

int f(TestEnum? val) {
  switch (val) {
    case TestEnum.VAL1:
      return 0;
    case TestEnum.VAL2:
      return 1;
  }
}

This will still report MISSING_RETURN, and may still confuse users.