Discussion: throwing away Futures into void locations

[MichaelRFairhurst]

This could be part of void_checks or unawaited_futures.

Given, the following function which takes a void function:

void foo(void Function() bar) {
  ...
}

and the following call site:

  foo(() async {
    ...
  });

We can confidently say that the resulting future will never be awaited. We can also say, this is a value that probably shouldn't be thrown away into a void location.

There's therefore an argument that this should be flagged by either void_checks or unawaited_futures.

It applies to other cases too:

final List<void> voidList = <Future>[...];
// or even what about.. ?
final List<Object> objectList = <Future>[...];

though these probably don't have to be covered right away.

As an argument against this, I expect the following type of code to be common:

stream.listen((x) async {
  ...
});

in the case where (x) async {...} needs to do async work, there's no better way to write this.

And yes it may have race conditions, but if the code needs synchronization, it looks wrong to me for it to not have synchronization explicitly:

stream.listen((x) async {
  await lock.aquire();
  ...
  lock.release();
});

But the lint may still catch cases where a the misuse an API, and hopefully there's a correct way to use the API that they can find after they see the lint.

[...].forEach((x) async {
  // this won't be awaited! What if these need to execute serially?
  ....
});
// there's no forEachAsync, however, there are other ways of doing what you want:
await [...].fold(Future<void>.value(null), (future, next) async {
  await future;
  ....
});

Counterpoint: the lack of await forEach() tells you that the code won't be serial, and so maybe that's another clue that this type of error isn't common.

[greglittlefield-wf]

Another similar case is classes overriding void functions to be Future:

class A {
  void foo() {}
}

class B extends A {
  @override
  Future foo() async {}
}

Code that calls with A.foo will likely never handle the future returned by B.foo().

Should I open a separate issue for this?

[parren-google]

Another example that I've come across quite a bit in UI tests is this:

someList.forEach((i) => i.click()); // where i.click() returns Future
someList.forEach((i) async {
  await i.click();
});
Future<void> click(Elt elt) {}
someList.forEach(click);

and also the same for someMap.forEach((x, y) async { ... });.

[eernstg]

Note that this issue is somewhat related to dart-lang/language#3715.

If the type inference of function literals is adjusted to match the current specification, I think the following example will behave as requested in this issue, because the function literal will now have return type void:

void foo(void Function() bar) {
  ...
}

void main() {
  foo(() async {
    ... // Inferred return type: `void`.
  });
}

So the function literal would be flagged by avoid_void_async because it is a function whose body is async and whose return type is void.

Note, though, that

someList.forEach((i) => i.click()); // where i.click() returns Future

may still be accepted silently, because there is no constraint at all on the type of the returned value in an arrow (=>) function whose return type is void. It might be flagged by discarded_futures, depending on how "a non-async block" is interpreted.

[bwilkerson]

@scheglov Another example of where inference is wrong in the analyzer.

[eernstg]

See also #3170 (comment).