Linter rule to prevent returning futures from the try block

[DaniilAlpha]

There have been multiple times when I’ve accidentally written code like this:

Future<Foo> doSomethingUsefulAndGetFoo() async {
  try {
    /* do something useful */
    return getFooButMayFail(); // wrong!
  } on GettingFooFailedException {
    /* do something with the exception */
    rethrow;
  }
}

While the right code would look like so:

Future<Foo> doSomethingUsefulAndGetFoo() async {
  try {
    /* do something useful */
    return await getFooButMayFail();
  } on GettingFooFailedException {
    /* do something with the exception */
    rethrow;
  }
}

The problem can be somewhat hard to spot: we are returning a Future<Foo> from the function instead of awaiting it and returning the result. In the first case, the exception does not reach the catch block, causing it to be thrown outside without the intended handling. Even when the goal is only to catch and not rethrow, this remains an issue—and I can’t recall ever writing this intentionally.

I thought it would be great if there were a linter rule for this, but since there isn’t, I’m submitting this feature request.

[FMorschel]

I love this idea!

And I would actually prefer if we had the exact opposite of unnecessary_await_in_return (if the body is async we suggest people to add the await). I think there was a language issue around that (IIRC @lrhn was part of the discussion or the author), but I can't find it.

It is currently hard to find where an async error comes from (see #62448 and related) so this would definitely help with this.

I'd be willing to implement this if the team agrees. Otherwise, I'll probably do it myself on my plugin anyway.

CC @bwilkerson @srawlins

[FMorschel]

I found the issue I was referring to:

• Disallow returning futures from async functions. language#870

[eernstg]

The behavior as described in the OP is actually a bug (see #44395). The exception should be thrown during the execution of the return statement, and the catch block should run. In other words, the behavior should be the same as in the variant that includes an await.

The proposal in dart-lang/language#870 would make the code with the buggy behavior a compile-time error, and the proposed lint would make it a less severe diagnostic. Another alternative would be to make it a warning, such that the wrong behavior is flagged even for developers who haven't enabled the lint (and, perhaps, are not aware of the issue). All of those approaches would allow us to eliminate code that triggers this bug, but it is of course also an option to fix the bug in the first place. ;-)

Nevertheless, dart-lang/language#870 is quite likely to be implemented because it allows the context type for return statements in async functions to be simpler, which will presumably make the resulting code easier to read and maintain. Also, fixing the bug gives rise to some hard-to-find bugs in existing code—we tried it and then reverted the fix...

@bwilkerson, @srawlins, @scheglov, WDYT? Would you consider adding a warning for the known-buggy case? That is, a return statement return e; that occurs inside a try block in a function body which is marked async such that the static type of e is a subtype of Future<T> where T is the future value type of the nearest enclosing function.

This approach has false negatives; in particular, the static type of e could be dynamic, which is not flagged, but may still give rise to the buggy behavior. On the other hand, it does not have false positives; every case which is flagged will give rise to the buggy behavior if the evaluation of e yields a future which is completed with an error.

[FMorschel]

Oh, so this is basically a duplicate of (with a different name but basically the same idea):

• Warn when returning a function invocation when within a try block in an async function #58279

Found it at #44395.

[eernstg]

Right, I forgot about that one!

[FMorschel]

And as mentioned in this comment from @natebosch:

You shouldn't use unnecessary_await_in_return at all. I think we should probably remove that lint.

I strongly agree with this.

[bwilkerson]

Would you consider adding a warning for the known-buggy case?

I have no objection to adding a warning with no false positives.

And I have no objection to removing the lint.

[FMorschel]

I've made the new rule on this CL https://dart-review.googlesource.com/c/sdk/+/477660.

After it lands, I plan on making a new one for removing unnecessary_await_in_return.

[eernstg]

Sounds good!

However, it's worth noting that the 'no false positives' property only holds if the extra conditions are taken into account. In particular, if we want the breakage to be minimal, the diagnostic message should only be emitted for return statements that occur inside a try block.

[srawlins]

I don't want to scope-creep this 😅 but should this also consider yield Future.value(7);?

(Note: It can be harder to incrementally report more cases with a warning or lint rule, than just tackling them in one swoop, making reviewers look at cleanup CLs in one go.)

[srawlins]

Oh also (I'd prefer to have the discussion here rather than on the CL; forgive me if this is addressed in the CL already), do we report the following cases:

• return e; where the static type of e is FutureOr<whatever>
• return e; where the static type of e is a type variable, T, with bound Future<something>, or bound FutureOr<something>
• return e; where the static type of e is a subtype (or assignable to?) Future<something>; this would be like various Future subtypes that are around like SyncFuture I think, and CancellableFuture.

[FMorschel]

[S]hould this also consider yield Future.value(7);?

I'd say so. I can't see any reason not to, but I'll leave it up for the team to answer for certain.

[D]o we report the following cases:

* `return e;` where the static type of `e` is `FutureOr<whatever>`

Yes.

* `return e;` where the static type of `e` is a type variable, `T`, with bound `Future<something>`, or bound `FutureOr<something>`

I think it should, but I'll be sure to add a test case.

* `return e;` where the static type of `e` is a subtype (or assignable to?) `Future<something>`; this would be like various Future subtypes that are around like `SyncFuture` I think, and `CancellableFuture`.

This one, I'm pretty certain it should, but again, I'll add another test case.

Thanks!