unawaited_futures doesn't work when method/function is not marked async

[zoechi]

Future<Null> sendInvitations() async { ... }

String doSomething() {
    inviteController
      ..sendInvitations() // <<<==== would expect a hint
      ..updateSearchTerm('');
}

but I don't get one.

Changing it to

Future<String> doSomething() async {

makes the lint show up.

[aemino]

The rule's description states:

Await future-returning functions inside async function bodies.

Await statements can only be used in function bodies that have been marked async.

[zoechi]

But then can still be used.
To me this limitation doesn't make any sense.
When I need to await, then I add async, otherwise I wouldn't.

[aemino]

In terms of my own code, I've rarely ever had to use then -- do you have a reason to use then rather than await? Either way, suggesting to use then on Futures seems somewhat out of the scope of this rule (it is called unawaited_futures, after all.)

[zoechi]

My point is that it shouldn't matter if async is used.
When an asyc call is made and the returned Future is ignored, the linter rule should warn.

[pq]

cc @bwilkerson

[bwilkerson]

Lints related to async behavior have been hotly discussed for a while. What everyone wants is to be told if they're making a mistake, but it's harder to statically determine whether a mistake has been made than might be expected.

Generally speaking, you don't want to create a future and then not do something when it completes. But sometimes that is an appropriate thing to do. I don't think we ever reached consensus as to the right way to allow some futures to be ignored without letting bugs go unnoticed.

I don't know what the right answer here is. We could try extending this rule to cover missing then invocations (which might be considered a breaking change by some users), or we could try writing a new rule that would cover those cases and just require that both be enabled to get full coverage. Neither seems ideal, but I can't think of any other options.

[zoechi]

A 2nd rule would be good enough for me.
I know that this rule is controversial because there valid cases where the returned future is ignored.
Devs who don't want to use // ignore: ... comments probably wont enable this rule at all.

I still have a hard time to understand why an ignored future in an async function is treated differently.
await is just syntactic sugar for then.

[bwilkerson]

I still have a hard time to understand why an ignored future in an async function is treated differently.

I don't know that there's any good reason for it. It's possible that the decision was largely political rather than technical.

[natebosch]

await is just syntactic sugar for then.

Not quite. It's then and ensuring the completion or error is accounted for in the return value of the function.

If you have a function that does not return Future but contains a Future we would never be able to escape the unawaited future lint.

Calling .then returns a new Future, which is now unawaited... Calling .whenComplete, .catchError, .timeout, similarly all return a Future which is now unawaited. Calling .asStream doesn't return a Future, but we certainly don't want to force this everywhere.

[zoechi]

I still don't see a difference between not awaited Futures in functions marked or not marked with async.
There is also no way to escape in functions marked async if a Future is not awaited.

There was a discussion that assigning the Future to var _ would be a good hint for the linter to ignore, but it seems that didn't get enough support. I find it a nice way to express the intent. that seems to work :)

[natebosch]

In a function not marked async it isn't possible to await a Future other than via return theFuture;.

I don't just mean this as "You can't use the 'await' keyword" - I mean this as "You cannot accomplish the semantics of 'await' without returning the Future or something chained from the Future".

Let's take your original example and imagine the lint showed up where you propose:

String doSomething() {
    inviteController
      ..sendInvitations() // imagine we have a lint
      ..updateSearchTerm('');
}

Now I want to fix this lint. How would I accomplish that?

String doSomething() {
  inviteController.sendInvitations().then(doSomethingElse); // Still a lint
  inviteController.updateSearchTerm('');
}

I suppose what I could do is:

Future<String> doSomething() {
  return inviteController.sendInvitations().then((_) {
    inviteController.updateSearchTerm('');
  });
}

If this is our goal I think we'd be better off with two lints, one as it exists and another that says "Don't call methods which return a Future from methods which aren't async" so we'd be forced to write the more readable (and easily linted) version with async.
I do think this second lint would have too many false positives to be useful.

[zoechi]

I don't just mean this as "You can't use the 'await' keyword" - I mean this as "You cannot accomplish the semantics of 'await' without returning the Future or something chained from the Future".

I still don't see how this makes a difference.

I made a call to a function foo that returns a Future, and the Future is not handled.
This is what this linter rule points out.

Future foo() async {
  return new Future.delayed(const Duration(sconds: 1, () => 5);
}

Future bar() async { 
  foo(); 
  doSomethingAfterFooCompleted();
}

void baz() {
  foo();
  doSomethingAfterFooCompleted();
}

bar has an ignored Future which causes doSomethingAfterFooCompleted() to run too early.
Because the linter rule warns about it, I am able to find the bug early and fix it.

baz has the same bug, but the linter doesn't help.

I can't see a reason why I should get a hint/warning in bar, but not in baz.
The only difference is, that in bar, there probably already was another async call I were aware of and therefore made it async, while in baz there wasn't a need previously to do something async and therefore it is still sync.

This is from a real issue I run into.
The code called an async function, but the developer wasn't aware that foo was async
and assumed that doSomethingAfterFooCompleted() is called after foo is done - which was a wrong assumption.
I had the linter rule enabled and expected to get notified about unhandled futures, but that didn't happen. So I looked for other causes until I found the actual bug.

In my opinion, this linter should hint about both or the rule causes more harm then benefit.

I don't care about one or two rules.
If they exist and they make sense, I enable them and have the safety net activated.
This is one decision.

[aemino]

@zoechi Thank you for providing this example; it gives some great insight into your perspective on this. Looking at the scenario you just described, perhaps it would be more apt to create a new rule that suggests marking the current function body as async when a Future is returned from a method call but not awaited?

[zoechi]

Not sure about that.
I think the issue is that a call to an async function looks the same as the call to a sync function, and in both cases (async or not) the issue is that a returned future is ignored.
All kinds of suggestions might be useful, but it's up to the developer how to handle it.
He might go with adding async, add // ignore: unawaited_future, add return and .then(...), search for a way to get the information sync, or completely refactor the code.
The important part for me is a hint that there is an ignored Future.

[gazialankus]

The important part for me is a hint that there is an ignored Future.

I fully agree. I also don't understand the distinction made between async and non-async functions with this rule. Forgetting to .then() after iosink.close() is equally bad as forgetting to await it.

Also, if a linter warning looks too much like something to be fixed, maybe an icon to the left of every line with an ignored future could be another alternative.

We have color icons for Colors.red, etc. Why not have a future icon for every line that lets a future slip?

[natebosch]

Forgetting to .then() after iosink.close() is equally bad as forgetting to await it.

The .then() returns a Future. Do you need to call .then() on it in an infinite chain?

Let me redo the example with the proposed problem function:

void baz() {
  foo(); // We imagine we want the unawaited future lint here.
  doSomethingAfterFooCompleted();
}

Let's say we get the lint, so we "fix" it.

void baz() {
  foo().then((_) {
    doSomethingAfterFooCompleted();
  }); // We're _still_ going to have an unawaited Future lint becauset .then() returned one.
}

How do we escape?

We probably don't want to allow you to not await the Future returned by a call to .then() in an async function, because that's still likely to mask bugs.

[zoechi]

How do we escape?

As far as I remember assigning the Future to a variable is a way to opt out

var _ = iosink.close();

or an // ignore: ... comment.

[gazialankus]

void baz() {
  foo().then((_) {
    doSomethingAfterFooCompleted();
  }); // We're _still_ going to have an unawaited Future lint becauset .then() returned one.
}

@natebosch I think this code piece you provided actually supports the case for the linter warning. This function above spawns an asynchronous task without returning its future. The caller of this function would falsely assume that the function returning means the task is completed. A linter warning here about the unused future is most welcome as it may remind the function's author that he/she may be misrepresenting what he/she is doing. Using the future by returning it would prevent that linter warning. If this function should really spawn something without returning its future, the var _ = or // ignore: would serve to point out that this is in fact an out of the ordinary situation.

[greglittlefield-wf]

How do we escape?

Could we not lint when the future uses an onError or catchError? That would allow you to "escape" by properly handling the error, since the returned future by .then is still unawaited.

// This would lint
foo().then((_) {
  doSomethingAfterFooCompleted();
});

// These would not

foo().catchError((_) {});

foo().then((_) {
  doSomethingAfterFooCompleted();
}, onError: (_) {});

foo().then((_) {
  doSomethingAfterFooCompleted();
}).catchError((_) {});

It might be difficult to catch more complex cases, but I'd imagine this would handle most usages.

[GreenAppers]

I'm worried about lurking bugs where I call async functions from non-async functions. In my case, I never actually want to do this. I want to make the enclosing function async, await the call, and repeat as necessary.

However I have no way to find these mistakes but line-by-line inspection.

Please either change the unawaited_futures lint to demand unawaited() wrappers on async calls in non-async functions, as proposed. They are indeed "unawaited", are they not?

Or provide some other means of identifying them. Perhaps avoid_async_from_sync.

I would appreciate any suggestion of a stopgap measure for identifying these cases automatically.

[om-ha]

I too agree with @zoechi and @GreenAppers

When not intentionally desired, It's very error-prone to call async functions from non-async functions.

As the developer invokes an async function from a sync one, he/she will presume state is synchronous and function call/invocation is blocking.

In my opinion this check should be automated and applied mandatorily as a warning and optionally as a tool/commandline. Hard to track this manually over old and new code.

[om-ha]

Any update regarding this?

[pq]

Hi @om-ha. This one has stalled a bit from lack of consensus.

Since unawaited_futures has been adopted by package:pedantic, I'd be curious if @davidmorgan has an insight on expectations for internal users. Failing a drive to updated unawaited_futures (for fear of false positives) I'm all for considering a new avoid_async_from_sync lint.

@bwilkerson: any fresh thoughts?

[om-ha]

@pq thanks for the insight, awaiting further thoughts from @davidmorgan and @bwilkerson.

Current situation is a slippery-slope especially for newcomers. Calling async functions from sync ones will result in asynchronous execution, since the invocation is of an asynchronous function. This is contrary to what the developer would expect as he/she is working within a synchronous function. This would result in dangerous side effects and things executing in an undefined order, causing realtime errors.

To overcome this slippery-slope, one has to remember that the function invoked is in fact async, or perform a sanity-check jumping to the definition of every function he/she invokes to make sure it's not async.

A relevant warning/lint rule would keep everyone safe.

[om-ha]

Because of the above, I vouch for enforcing avoid_async_from_sync lint rule. And making sure it reaches stable release.

[davidmorgan]

Here's my suggestion :)

It's fine to use async methods from a sync method provided you do one of:

1. pass the Future to another method; this includes the unawaited method in package:pedantic which you can use to signal you don't want to wait
2. return the Future so someone else can wait for it

This lint could be called something like unhandled_futures, since in this context you can't await it?

[om-ha]

@davidmorgan you raise valid points.

First point

1. pass the Future to another method; this includes the unawaited method in package:pedantic which you can use to signal you don't want to wait

Useful when delegating handling the future to another method.

Second point

2. return the Future so someone else can wait for it

This is very useful for example when using Future.wait

Warning name

As you said, unhandled_futures sounds very plausible.

Examples

Warning would be emitted (lint rule would apply) when future result is not used/handled inside a synchronous function:

  // Emits `unhandled_futures` warning
  void mySyncFunction() {
    executeSomeAsyncMethod();  
  }

There wouldn't be a warning when future result is handled inside a synchronous function:

  // Does NOT emit `unhandled_futures` warning
  void mySyncFunction() {
    final _ = executeSomeAsyncMethod();
    
    final someFuture = executeSomeAsyncMethod();
    
    List<Future> myFutures = [];
    myFutures += executeSomeAsyncMethod();
  }

[androidseb]

Hello,

I got bitten by bugs caused by this linter behavior I did not expect: unawaited_futures doesn't work when method/function is not marked async.

I know it doesn't resolve this with the official dart linter, but here is an immediate workaround I found to keep these bugs away from my code:

I had forked an MIT version of the Dart Code Metrics project and I built a custom rule to cover this use case, you can find the code here:
androidseb/dart-code-metrics@7f79507#diff-d3bfb9cb2cd8305db1085ca13ae2bcebd1e6a5fb018873aa8b9544dbd563cd33

If you want to use this MIT-modified version of Dart Code Metrics, you can use this in your pubspec.yaml file:

dev_dependencies:
  dart_code_metrics:
    git:
      url: https://github.com/androidseb/dart-code-metrics.git
      ref: dcm_mit_fork

and then use this in your analysis_options.yaml file:

analyzer:
  plugins:
    dart_code_metrics

dart_code_metrics:
  rules:
    # ... define other rules before or after as needed
    - avoid-unawaited-futures-in-future-sync-functions

I've also emailed a link to the code to the Dart Code Metrics authors in case it helps them make this available to their licensed users too.

Maybe this will help inspire someone to replicate this for the official dart SDK, I may attempt this at some point.