Linter: await_only_futures without avoid_void_async question

[parlough]

If await_only_futures is going to be included in the canonical score lint list(or any list), I think avoid_void_async should be included as well.

Users may have consciously specified await previously for a function that returns void but didn't specify the Future. Then when they enable this lint, they may remove the await(possibly with the quick fix even) to address it forgetting their original intentions, unintentionally changing the behavior.

@pq @bwilkerson Perhaps I'm missing something, but any thoughts here?

[bwilkerson]

I think that's a valid concern.

[pq]

Agreed.

FYI @munificent @natebosch @jakemac53 @lrhn @mit-mit

[lrhn]

Disagree.

If a function returns void, you should never have awaited the result. That's a bug right there, and that should be fixed, either by properly ignoring the result or changing the return type.
If you intended to make the return type Future<void>, but forgot, then await_only_futures will force you to do it.
If you intended to make the return type void, then (no matter whether the function implementation happens to be async), no-one should await the result. And if they don't, there is no problem.

It's an implementation detail whether a void-returning function is async or not. If anybody cares, that's a bug to fix.
I don't want avoid_void_async enabled by default because it blocks completely valid uses.

My test files often do void main() async { ... }, and that's completely reasonable. So is stream.forEach((event) async { something async }) which will give the function a return type of void.

So, fix the bug where the bug is, which is the code affected by await_only_futures, not necessarily the code flagged by avoid_void_async.

[jakemac53]

I agree with @lrhn here. If there is any reason to await the result of an async function, it should not have a return type of void.

The fact that you can actually use the result of a void function is just a weird quirk of the language, and not something we should double down on.

[lrhn]

It's not that we'd allow you to await the void in any case, but adding the avoid_void_async lint would disallow any void foo(args) async declaration. That would probably avoid the issue of anyone wanting to await a void, but it would also disallow completely valid uses of a void function which uses await internally.

(Why do we allow await voidExpression at all? @eernstg, do you remember?)

[parlough]

Thanks for the insight, I didn't consider how much it does limit those valid use cases. I suppose even if it does cause any problems due to user's forgetting to mark a function as returning a Future<void> if they intended for it to be awaited/used, they'll likely remember and notice the necessity as they address the await_only_futures lint. Even if they don't recall(or check) and blindly follow the lint, the difference in behavior will likely be apparent soon.

While I doubt many look at individual lint documentation, perhaps expanding on the documentation here could at least help avoid potentially behavior changing situations.

Either way, I agree you're right and it's not appropriate to include avoid_void_async by default. I just wonder if there's a good way to increase the awareness by users to know when to return Future<void> versus just void, as a lot of developers already have trouble understanding various async concepts.

[pq]

While I doubt many look at individual lint documentation, perhaps expanding on the documentation here could at least help avoid potentially behavior changing situations.

💯

Doc improvements here would be really welcome!

[natebosch]

(Why do we allow await voidExpression at all? @eernstg, do you remember?)

Because existing code with await voidExpression was not cleaned up in time for the Dart 2 release schedule.

[natebosch]

If you are impacted by await_only_futures and can't update the API to fix the return type to be non-void you can suppress the lint with:

await (voidExpression as dynamic);

[eernstg]

(Why do we allow await voidExpression at all? @eernstg, do you remember?)

Oops, I hadn't seen that question. Agreeing with @natebosch, I wasn't the only one who wanted to outlaw await voidExpression, but because of the tight schedule we allowed it. We ended up with this spec text in the list of allowed usages of void:

In an expression of the form await e, e may have type void. _This rule was
adopted because it was a substantial breaking change to turn this situation
into an error at the time where the treatment of void was changed. Tools
may choose to give a hint in such cases._

We should probably have taken the opportunity to make it an error as part of null safety, but await_only_futures should also catch it.

I think avoid_void_async is more tricky: As @lrhn mentions, it is perfectly meaningful to have a function returning void whose body is async, it just means that nobody should await the completion of the returned future, that is, "this is a fire-and-forget operation".

It would be really great to have some concrete data on the frequency of fire-and-forget async functions. If they are somewhat common then everybody needs to learn what they are, and everything is fine (and then avoid_void_async might be dropped altogether, and would certainly be omitted from lists of mandatory lints).

But if intentional fire-and-forget functions are exceedingly rare then almost all void-async functions would presumably arise by mistake:

// Old version of `f`.
void f() {... bar() ...}

// New version: `bar` was changed to return a future, so we must make `f` async.
void f() async { ... await bar() ...} // Oops, forgot to change the return type!

If almost all void-async functions arise by mistake then we might want to enable avoid_void_async, and then ensure that avoid_void_async does not flag a function declaration which is marked by @fireAndForget.

[srawlins]

I think the possible remaining action item here is documentation as per @parlough 's comments above.