No analyzer hint for function returning nullable non-Null type which implicitly falls off the edge

[srawlins]

Before null safety, the analyzer would report a Hint here:

int f() {}

It was perfectly legal to implicitly return null in this function. But a Hint was added because it looks highly suspicious (and the linter package was just a dream).

In the null safe type system, the analyzer does not report a Hint here:

int? f() {}

Again, this is perfectly legal because of an implicit return when falling off the edge of a function body. But again, it is highly suspicious. I think there should be a Hint.

If we decide that the second function is less suspicious than the first (it implicitly returns null for an explicitly nullable type, rather than an implicitly nullable type), then I'd say it still deserves a Hint; a Lint at the least.

[srawlins]

CC @bwilkerson @pq who I hope have some thoughts. 😄 I know you're also acutely aware of the linter's coverage for null safety concepts.

[bwilkerson]

I remember having a discussion in which we decided that the hint (missing_return) was not needed under null safety when the return type was non-nullable because it became a compile time error. I don't remember explicitly deciding that we shouldn't have it in the nullable case, so it's possible that it was disabled too broadly.

I would tend to agree that it's better to have an explicit return because it indicates that the author actually considered what would be returned if the end of the function was reached. As for the hint vs. lint question, I can see arguments for both.

It might be interesting to re-enable the hint and analyze a large body of code to see whether people have started to rely on being able to omit the return at the end. It might also be interesting to get an opinion from the language team.

@lrhn

[lrhn]

I would personally prefer to force you to write return in any non-voidy non-generator function (any return type except FutureOrn<void>, or Future<voidy> for async functions).

We made exceptions for Null and dynamic, but mostly for backwards compatibility reasons. I'd love to get rid of those (at least the Null exception)

Can't speak for the entire language team, I'm definitely stricter than average.

... and yet, the same day I wrote this, I ended up designing an API where I returned Future<void>? (optionally a future, but not FutureOr<void>, because I wanted to use ?? on the null result). And while using that, I very much wanted to not have to write return null; in the no-future-returned case. So, just maybe, allowing not returning anything from a nullable function is reasonable in some cases. (Like, my cases, not yours! 😈)

[davidmorgan]

+1 for the hint; this looks like an accidental regression to me.

It was noticed by an internal customer, they were about to create style guidance around it but I suggested we first check if there is a possibility of fixing it :)

[srawlins]

I'd say important to do it sooner rather than later, as the pace of migrating to null safety is increasing internally; implementing this check earlier means less clean-up.

[eernstg]

About the idea that this situation should be flagged: 👍

Here is a situation which is related: int? f() { return; } is actually a compile-time error, so we don't allow the null to be implicit in a return statement (because the return type in this case isn't void, dynamic, or Null).

I believe the only reason why it isn't an error to return null silently by reaching the end is that this situation wasn't well characterized in pre-null-safety Dart (we did not have a precise control-flow analysis).

So it would be nice to have a lint (or hint) now, especially because it seems to be non-breaking. Then we could make it an error later on, for consistency.

[jefflim-google]

+1, would love to see this hint or even error be implemented.

[srawlins]

This turns out to have a lot of existing violations arleady in Flutter and even the Dart SDK repo. :( I might need to implement this as an opt-in Lint or a newly-named default opt-in Hint (warning). I found in the analyzer codebase a common pattern:

Foo? getFoo(List gadgets) {
  for (var gadget in gadgets) {
    if (...) return gadget.foo;
  }
}

The fix for such methods is to add a return null; at the end. This felt unfortunate, but does still make the code more clear. In any case, we might face resistance to this as a default opt-in Hint, if it just means making implicit return null statements explicit.

[lrhn]

I don't personally think having to add a return null; is unfortunate. If anything, I'm very sorry it's not already required. It really should have been since Dart 2.0. (Only void functions can omit returns, this is omitting a return!)

[bwilkerson]

Given the potential impact on the ecosystem, I believe that we should initially implement it as a lint, add it to the recommended / core lint set at some later point, and consider making it a warning or error in some future language version.

[pq]

Given the potential impact on the ecosystem, I believe that we should initially implement it as a lint

+1.