unawaited_future should not warn if .then block is present

[danrubel]

If a method returns a Future and a .then expression is attached to the result, the unawaited_future still reports a problem since the .then expression itself returns a Future. Yes, the developer could add an // ignore: unawaited_futures comment above the then expression, but that clutters the code for the 99% of situations where the developer is properly handling the result.

I propose that we change the definition of the unawaited_future lint to:

GOOD:

Future doSomething() => ...;

void main() async {
  await doSomething();

  doSomething().then((Result result) {
    // handle the result
  });

  // ignore: unawaited_futures
  doSomething(); // Explicitly-ignored fire-and-forget.
}

BAD:

void main() async {
  doSomething(); // Likely a bug.
}

[Hixie]

For extra bonus points, if we can distinguish a then block that always returns null from one that returns a Future of its own, that would really be awesome. Not sure how easy that would be though.

[pq]

cc @alexeieleusis

[alexeieleusis]

reviewing it, also cc @ochafik

[alexeieleusis]

I added the following snippet to the test with the second line linted:

Future fut() => ...

Future foo7() async {
  fut().then((_) {});
  fut().then((_) { return 42; }); // LINTED
}

@Hixie IIUC we want to lint handlers with an explicit return statement? The rule seems to behave as expected.

IIRC flutter has a fork of the dart sdk, Could it be that recent improvements to the analyzer itself fixed this issue?

Tested at dart-archive/linter@407d736

[lrhn]

Have you considered to make an unawaited future (even with a .then) cause a hint inside an asynchronous function (anything with a Future return type) and not in a sync function?

Whether the then-block returns null or 42 probably isn't as important as whether it throws an error, and that's much harder to detect when any function call can throw.

[Hixie]

@irhn raises a good point. I don't know what the right solution is.

[ochafik]

Can we make sure we're only muting lints for these then calls when the callback does not return a Future?

@lrhn: the lint currently only trigger inside async bodies.

[zoechi]

I would find this quite confusing.

I have a hard time seeing why

  doSomething();

and

  doSomething().then((Result result) {
    // handle the result
  });

should be treated differently.

The only difference is that the 2nd does "something" when the async call completes but both continue executing the following code immediately without waiting for the async call to complete, which might or might not be intended in both cases.

[Hixie]

And both fail to catch exceptions, leaving them as uncaught (even if that line is wrapped in a try/catch block). I guess the Future chain needs to end with something that handles errors for the lint to not complain.

The question is, though, how would you write the code if you did want to this to be done asynchronously with the rest of the function?

[lrhn]

I seem to remember something the discussions from when this hint was introduced.
If you assign the future to a variable, is it still considered unhandled?

// no hint, future is accounted for. Might get an unused variable warning, though?
var _ = asyncFunction(); 
restOfFunction();

It is true that the only way to know that a future is handled is to await it. Any other async operation generates another future or stream that you don't know anything about. You can add heuristics, but the simplest solution is probably to allow an // ignore comment to disable the hint.

[ochafik]

Assigning to a var will not result in a lint, as only non-awaited expression statements trigger the lint (bar some exceptions).

For reference:

• source of the lint
• tests of the lint

[Hixie]

Assigning to a variable is no good because (a) the variable will be unused, so we would still be getting a warning for that regardless, and (b) the bug is still there -- we're ignoring the errors from that future, and it will eventually result in those errors being uncaught top-level exceptions in the zone, so we should be getting an "unawaited future" warning.

[natebosch]

unawaited from package:pedantic is our canonical answer for cases like this. It's an explicit documentation that, among other things, asynchronous errors are being ignored.

As mentioned in this thread it's no safer to skip the await for a future with a chained .then than any other Future.

See further discussion in #57400, #57437, or #57784