proposal: extend unawaited_futures to catch discarded futures when upcast to void

[pq]

extend unawaited_futures to catch discarded futures when upcast to void

Description

Future results upcast to void are not awaited.

Details

Future results upcast to void are not awaited and are a source of bugs. Commonly, for example, asynchronous callbacks in forEach don’t get awaited which is surprising.

To catch these issues, we propose extending unawaited_futures to flag cases where Futures are upcast to void (unless specifically unawaited).

Kind

Error

Bad Examples

Future<void> click() async => ... ;

things.forEach((thing) => thing.click()); 

Future<void> click() async => ... ;

things.forEach((thing) async {
  await thing.foo.bar.click();
});

Good Examples

Desired unawaited futures can be wrapped in an unawaited function call:

things.forEach((thing) => unawaited(thing.click()));

Alternatively, if awaits are desired, iteration can be re-written like so:

for (var thing in things) {
  await thing.click();
}

Discussion

See also: #3217

---

Updated 02/23/2022 to broaden the proposal from forEach to embrace all upcasts to void.

[natebosch]

Should we consider any place a Future Function(...) gets passed to a void Function(...) argument? Special casing forEach feels too limited to me.

Desired unawaited futures can be wrapped in an unawaited function call:

This behaves very differently from unawaited today. unawaited impacts the behavior of the analysis of exactly the Future you pass to it, the analyzer doesn't need to understand uanwaited in any special way. In your example here the unawaited is acting as a marker to trigger different behavior in the analyzer, and the impact is not limited to the single Future which is the argument.

I think the rewrite would be to things.forEach((thing) => unawaited(thing.click())).

[leafpetersen]

Should we consider any place a Future Function(...) gets passed to a void Function(...) argument? Special casing forEach feels too limited to me.

Looking at forEach seems like a reasonably starting experiment, but agreed that generalizing feels like something we should strongly consider unless there is good evidence that the generalization has substantially more false positives.

[pq]

Thanks for the fix @natebosch. I updated the example.

I totally agree that generalizing is preferred and was kind of wanting to babystep but maybe that's too cautious. An experiment where we flag all places where a Future Function(...) gets passed to a void Function(...) argument would be really interesting so I might just try that and we can retreat to a safer analysis limited to forEach in case too many false positives shake out.

[eernstg]

If the feature is generalized a bit, we might as well try to handle the type relationships in a more general manner and see how much it hurts:

In a value transfer situation (initialization of variable, assignment to variable or parameter, return), if the expression yielding the value has type S and the declared type of the receiver is T, and if S <: T (so we ignore S == dynamic), we check the relationship between S and T, and flag the situation if "the transition from S to T drops the type Future".

We could say something along the following lines: The transition from a type S to a type T drops the type Future if:

• S is Future<U> for some U, and T is not Future<V> for any V, [perhaps: and not FutureOr<V> for any V]. Example: Object o = Future<int>.value(0);.
• S contains Future<U> for some U in a covariant position, and the corresponding position in T does not exist. Example: Object os = <Future<int>>[], and Future<int> f() => Future.value(0); followed by Function g = f;.
• S contains Future<U> for some U in a covariant position, and the corresponding position in T is V, and the transition from Future<U> to V drops the type Future. Example: Iterable<Object> os = <Future>[]; and void Function() g = f;, where f is defined as in the previous bullet.
• T contains Future<V> for some V in a contravariant position, and the corresponding position in S does not exist. Example: C<Future<int>> c = D(); where class D extends C<Object> {} and the type parameter of C is contravariant (so we're considering a version of Dart that supports declaration-site variance).
• T contains Future<V> for some V in a contravariant position, and the corresponding position in S is U, and the transition from Future<V> to U drops the type Future. Example: void Function(Future<int>) f = (Object o) {};.

(We could of course spell this out in terms of specific forms of types, but I tend to think that we should be able to reuse the fundamental structure of "an X-variant position" ;-).

We might wish to support a similar notion of "the transition from a type S to a type T drops the type U" where U stands for a member of a more general set of types, but I wouldn't be surprised if Future needs enough special casing (e.g., FutureOr) to justify taking that generalization as a separate step.

[lrhn]

The rule for unawaited_futures is that inside and asynchronous function only, a future should not be dropped on the floor.

Would this forEach rule also apply outside of async/async* functions?

Erik has done the full analysis. I came to basically the same conclusions before reading his. Any place a future is up-cast, it loses future-ness. That happens where a subtype relation depends on a sub-deduction of the form Future<?> <: non-Future-supertype. That means that whichever value corresponds to that type at runtime will be able to be a future, and not know it.
(Actually, I think the rule should say that if S is a subtype of Future<U>, because it is possible to subclass Future, and that's OK to upcast, as long as upcast it to at most Future<V> or FutureOr<V>).

And we should not include dynamic in that, because dynamic is expected to lose static type knowledge, but be correct anyway because the author knows what they are doing. (If not, they shouldn't be using dynamic).

If we want to not risk false positives, we might just want to consider upcasts to void. And upcast to Object does lose the static knowledge that the value is a future, but the code might know and might cast the object back.
For void, the value should be assumed lost. It's very bad style to use a void value in any way.

We originally considered doing "higher order void-preservation", but ended up with just the basic level of "don't use a void expression for its value". The higher order version would be something like, don't assign a void Function() to Object? Function().
It wasn't really that useful, so we never did anything more. The "don't lose a Future" rule makes more sense in practice.

[pq]

Thanks all for the thoughtful feedback!

In our last conversation we discussed an experiment to vet the impact of a constrained generalization which lines up w/ @lrhn's proposal to focus on upcasts to void:

If we want to not risk false positives, we might just want to consider upcasts to void.

The current plan is to implement that, test internally and externally and then consider broadening to embrace some of @eernstg's further-reaching ideas.

[pq]

See also: #1123