missing_return should know about enums and switches

[pq]

From @Hixie on August 20, 2018 23:11

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

Copied from original issue: dart-lang/linter#1139

[pq]

From @matanlurey on August 20, 2018 23:13

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)

[pq]

From @Hixie on August 21, 2018 0:11

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.

[pq]

From @bwilkerson on August 21, 2018 0:15

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.

[pq]

From @Hixie on August 21, 2018 0:26

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.

[pq]

From @bwilkerson on August 21, 2018 4:43

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.

[pq]

From @Hixie on August 21, 2018 5:24

"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).

[pq]

From @srawlins on December 29, 2018 19:54

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.

[pq]

From @bwilkerson on December 29, 2018 21:45

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.

[pq]

From @Hixie on December 29, 2018 23:21

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

[lrhn]

In Dart 2, the "implicit" return of null at the end of a function is only allowed in void functions. In all other functions, a missing return is considered an error (or at least a warning) because it could be hiding an actual programming error. You need to be explicit about returning a value from non-void functions.

[Hixie]

Yes, this bug is about fixing that.

[eernstg]

@bwilkerson wrote:

this will be less of a problem if we get non-nullable types

Right, and it matches up with the plans for how to introduce such types. Here is the original example again:

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
}

NNBD (non-null by default) is a language feature that will be added to Dart. At first, we will most likely get a level of support which includes static checks, and soon after that also a level that allows manual checks (like the assert(value != null) above) to be removed, because the compiler can generate them (and, preferably, developers can enable such checks independently of --enable-assert).

If the library that contains hello opts in to use NNBD then the parameter value will have type Foo (not Foo?, that is, its type will be non-nullable). This means that call sites in opted-in code cannot pass null or an argument whose type is nullable without being detected (it's a downcast), so dynamic checks can be generated, and hints etc. can be emitted.

For call sites in legacy code (which does not contain an NNBD opt-in marker) we cannot expect to be able to enforce soundness in general, but with a compiler-generated null check we would actually know for sure that value is not null.

In any case (with or without soundness), the static analysis would consider value to have a non-null type, which means that the switch can be considered exhaustive, and the missing_return message can be avoided.

In summary, we shouldn't need to worry so much about null here, that's coming.

The other part is about exhaustiveness and reachability (specifically, it's about the property 'statically known to not complete normally'), and that's being addressed here: dart-lang/language#139.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/149606