missing_return should know about enums and switches

[Hixie]

If you assert that a variable of enum type is non-null before a switch, and then you have a switch where every enum value is handled and terminates (returns, throws, or exits), then it should not flag that function as not ending with a return. (The usual pattern today is to return null after the switch to silence the analyzer, but that's what would happen anyway if the statement wasn't there, in the case of the variable being null in a release build.)

For example:

enum Foo { a, b, c }

String hello(Foo value) {
  assert(value != null);
  switch (value) {
    case Foo.a: return 'A';
    case Foo.b: return 'B';
    case Foo.c: return 'C';
  }
  // This should not be flagged by missing_return                                                                            
}

cc @a14n

[matanlurey]

I believe missing_return is a feature of the analyzer proper (dart-lang/sdk) and not the linter.

(Also the analyzer isn't allowed to use assert statements to change program flow)

[Hixie]

Feel free to move the bug to the other repo, I don't really understand the difference between lints and errors in the analyzer.

This doesn't affect program flow. The flow of the program and the return value with and without the assert are identical, regardless of the value, in every mode (though obviously debug and release would behave differently from each other). This is just about whether the missing_return lint/error/whatever should be flagged in a case where the code is redundant anyway.

[bwilkerson]

Yes, missing_return is part of the analyzer, not the linter (too many moving parts?).

And, yes, analyzer does not currently trust asserts.

... the code is redundant ...

Only if you trust that the assert means that the value will never be null. If it is null, then none of the cases will be selected and you'll end up implicitly returning null, the very thing the hint was designed to catch. That said, this is a hint, so it isn't mandated by the spec and we have some latitude to change it.

[Hixie]

What I mean is that return null is redundant with implicitly returning null.

In general I agree that we want to catch that (hence missing_return), but in the specific case of a switch where the only way to fall through to the return is if the assert is false, the code is cleaner if we omit the explicit return.

This doesn't require trusting the assert; the behaviour is the same in release mode whether you have the explicit return or not. It's just about avoiding an extra (redundant) line of code.

[bwilkerson]

Personally, I don't see it as being redundant, I see it as being explicit. Just because there is an implicit default behavior, that doesn't mean that I intended the implicit behavior.

[Hixie]

"Redundant" isn't a value judgement. It's literally redundant in the sense that it's the same as the implicit behaviour. It can be explicit and redundant.

I think most of the time this redundant explicit statement is the right thing to do. It's just in this specific case with the switch that covers every possible value, it's actually misleading because assuming there's no bugs, it's dead code (and, once this bug is fixed, could even be called out as such, see #57781).

[srawlins]

If explicit return values of null are redundant because it duplicates the implicit behavior, what is the request around enums and switches? It seems like #57782 would be a better solution. E.g.

String f() {
  if (1+1==2) {
    return "hello";
  }
  return null
}

has just as much redundant behavior as the enum/switch example.

[bwilkerson]

The purpose of this hint is to prevent code from implicitly returning null under the assumption that doing so is unlikely to be what the author intended. I don't know how often the above pattern occurs, but assuming that it's a problem that really does need to be solved, I can think of a couple of ways to address it.

1. We could suppress the hint in the case where asserts or other analysis could be used to prove that the return will not be reached. That would address the original case, but I don't know whether it would address all of the necessary cases.

2. We could convert the hint to a lint so that it won't be produced unless requested.

I will also point out that it is possible that this will be less of a problem if we get non-nullable types:

• If the return type of a method is non-nullable, then an implicit return of null won't be allowed, and if it's nullable then perhaps implicitly returning null will no longer be something to guard against.

• In the type of a switch statement's expression is non-nullable, then we will know that null isn't a possible value and won't need to guard against that case.

Which leads to another option:

3. Provide a way to annotate that the value is expected to be non-null that can be used until non-nullable types exist as part of the language.

[Hixie]

3 would be awesome. We use assert(foo != null) all over the place for this.

lint vs hint wouldn't resolve this issue. I don't understand the difference between errors and hints: both can be turned off, if desired (by post filtering if nothing else), both show up in the analyzer output. shrug

[pq]

This issue was moved to #35710