New lint: prefer_async_when_returning_futures

[ianloic]

Methods and functions that return Future should be declared async so
that they don't inadvertently throw Exceptions rather than reporting
errors through Futures.

This is a rule we try to enforce through code review on the Fuchsia team but it would be better to have a lint.

[bwilkerson]

I'm concerned about the potential number of false positives. There are times when you really can't make a function async, such as when you're creating a Completer and returning the completer's future.

While that's not a common case, I wonder whether we might not be able to remove the possibility of false positives by instead having a lint for non-async functions that return a future and throw an exception. It's a little more work to check for violations, but it sounds like it would be more targeted toward what you're really concerned about and wouldn't have any false positives.

[bwilkerson]

This could also be

if (node.body is EmptyFunctionBody) {

[matanlurey]

I think the intention of this lint is awesome @ianloic, but I wonder if it could be solved another way.

In particular, I agree with @bwilkerson about false positives and his suggestion. I'd like to add another point of view, adding the async keyword, currently in the Dart VM, AOT, and Dart2JS/DDC, performs more work than just returning a Future directly. I believe in benchmarks done by @nex3 there was a quite big delta, so I'd hate to slow down all of Fuischa's code because of the intention of this lint.

[nex3]

I've never benchmarked async code versus manual Future-handling code, just async versus sync.

[ianloic]

I'm concerned about the potential number of false positives. There are times when you really can't make a function async, such as when you're creating a Completer and returning the completer's future.

You can return a Future from an async function. This works just as expected:

import 'dart:async';

Future<int> func() async {
  final c = new Completer<int>();
  c.complete(42);
  return c.future;
}

void main() async {
  print(await func());
}

While that's not a common case, I wonder whether we might not be able to remove the possibility of false positives by instead having a lint for non-async functions that return a future and throw an exception. It's a little more work to check for violations, but it sounds like it would be more targeted toward what you're really concerned about and wouldn't have any false positives.

Any method call or unconditional field access could throw an exception. We're more worried about errors like that than functions that explicitly throw from their body.

[bwilkerson]

You can return a Future from an async function.

That's true, you can. I would have expected that to be an anti-pattern, but I'm not an expert on style issues. If we consider the example you gave to be good code, then that resolves the potential for false positives.

Any method call or unconditional field access could throw an exception.

Good point.

... adding the async keyword, currently in the Dart VM, AOT, and Dart2JS/DDC, performs more work than just returning a Future directly.

Is that true? With the sync-async change that was added in Dart 2.0, I thought that functions marked with async would run synchronously up to the first await. The example above will still return a Future that needs to be awaited, but it will already be completed so there shouldn't be any need for it to run through the task queue. Of course, that's just my guess; I don't actually know the implementation details.

[matanlurey]

@bwilkerson:

Is that true? With the sync-async change that was added in Dart 2.0, I thought that functions marked

The timing is better, but it still does more work :(

[matanlurey]

(Also see dart-lang/sdk#34145, because I worry that lints like this (i.e. to enforce a team-specific style that is not part of the Dart style guide) are problematic to host in dart-lang/linter.)

[bwilkerson]

Problematic in what sense?

[matanlurey]

Having individual teams adding lint rules that are available to everyone [so the expectation is we support it] but don't necessarily comply (or even contradict) our own style guide, of course :)

[bwilkerson]

Well, that's one of the nice consequences of lints being opt-in. Each team (at least externally) can choose to enable only the lints that they agree with. In my mind it's a good thing that the linter can support lints that are not specified by the style guide.

[pq]

I think this needs to get scooted down after prefer_asserts_in_initializer_lists. Integration tests should pass then!

[srawlins]

Changes were requested in 2018 which haven't been addressed. I'm closing this just to tidy the issue tracker. If you would like to update the branch with main, and address comments, then please re-open with changes.