Spurious unawaited_futures warnings

[tvolkert]

Steps to Reproduce

1. git clone git@github.com:google/file.dart.git
2. cd file.dart
3. dartanalyzer .

Expected Results
No analyzer warnings. At least, until very recently, there were no warnings.

Actual Results
https://gist.github.com/tvolkert/4ae91439e738207d3b566e758e7b7680

What's very strange is that many of the lines it complains about don't even have futures at all, like https://github.com/google/file.dart/blob/master/test/replay_test.dart#L57. Other lines do have futures and are of the form expect(futureValue, throwsFoo), which have never produced any lint warnings before.

[tvolkert]

@pq

[pq]

@alexeieleusis : any thoughts?

[zoechi]

That's a change in the expect() function that now returns a future
dart-lang/test#529 (comment)

[nex3]

In the short term, it might make sense to special-case expect() here. In the medium term, it would be great to have some sort of @optionalAwait metadata added to the meta package so that it's easy for packages to declare that particular futures don't always need to be handled.

[pq]

cc @alexeieleusis @bwilkerson

[alexeieleusis]

I don't have enough context on this rule, I've seen a few places where even though you are in an async method body you don't want to await, but probably is worth ignoring with a comment.

@ochafik who originally wrote the rule should have more context on the use cases for this. It would be nice to also have feedback from a readability team member.

[zoechi]

Shouldn't that rule also fire in non-async context when a future is returned that is not chained with .then()?

[zoechi]

Wouldn't the new FutureOr<T> help here, when the rule only fires on Future but not on FutureOrT?
I didn't have a closer look but AFAIK this was introduced for methods that might or might not return a Future.

[nex3]

@zoechi That seems like a hack, although one that may work in the short term. There's no reason in principle that FutureOr shouldn't always be awaited if Future should, on the principle that it will return a future some of the time and you ought to await that.

Also, it will hurt inference for cases (like expect()) that always do return futures, since the type system will believe the type is dynamic.

[tvolkert]

FYI, this lint, were it enabled in Flutter, would have saved me about ~4 hours of debugging today.

flutter/flutter#8227

[zoechi]

@nex3 is that really different to the annotation you (AFAIR) suggested, to annotate a method to indicate whether or not it should be awaited? (just from what I remember because I wasn't able to find the issue).

[ochafik]

The reason for the current behaviour of this lint is to not forget to await when "in an async state of mind" (i.e. inside an async method), while allowing people to fire-and-forget.

This does leave out code that forgot to use an async method at the first place, but we've found countless bugs in our codebase with this linter already.

Re/ spurious warnings: one can disable this warning, not use an async method, or wrap the fire-and-forget call in a local void-returning non-async helper.

[nex3]

is that really different to the annotation you (AFAIR) suggested, to annotate a method to indicate whether or not it should be awaited? (just from what I remember because I wasn't able to find the issue).

Yes; FutureOr indicates "this may or may not return a future", @optionalAwait Future would indicate "this Future doesn't need to be awaited".

Re/ spurious warnings: one can disable this warning, not use an async method, or wrap the fire-and-forget call in a local void-returning non-async helper.

None of these seem to be satisfactory for users of this lint using extend().

@pq Can we prioritize this? It's blocking the Google3 roll of test.

[devoncarew]

It's not clear to me that the right fix isn't to roll back dart-lang/test#529.

[pq]

@nex3 :

@pq Can we prioritize this? It's blocking the Google3 roll of test.

Are you suggesting specifically we special-case expect()?

That said,

It's not clear to me that the right fix isn't to roll back dart-lang/test#529.

I'm wondering about this too. What about rolling back and the regrouping? If the API change sticks we can talk about how to coordinate to soften the transition...

[nex3]

We can't "roll back" a released package, and lints can't render an API change invalid anyway. The linter's role is to help people work with real-world code, not to put constraints on what code can exist. There's no way for API designers to know when a change might break one of the many (sometimes contradictory) lints that exist, so the lints that are sensitive to those changes need to be able to react quickly when they happen.

Are you suggesting specifically we special-case expect()?

As I mentioned above, I think the best solution here would be to provide an @optionalAwait annotation that would explicitly tell this lint which futures don't always need awaits. Special-casing expect() may

[matanlurey]

Why do we need to add yet-another annotation because test made a breaking change?

I'm still holding out for expectLater, and I think if you took a poll of users they'd also agree almost unanimously. There's value sometimes in admitting a mistake (I make lots of them, continuously) and rolling back to think about the design more and talk to users.

[nex3]

Why do we need to add yet-another annotation because test made a breaking change?

If we consider any API change "breaking" if it causes optional potentially-contradictory lints to produce new warnings, then it becomes next to impossible for authors to know whether or not their changes are "breaking". It also puts egregious constraints on otherwise reasonable and useful API patterns. It's not a tenable restriction. We must instead work to make lints that can be affected by upstream APIs more flexible.

I'm still holding out for expectLater, and I think if you took a poll of users they'd also agree almost unanimously.

I don't think user referenda are a road to good design under any circumstances, but I strongly stand behind my API for reasons I've gone over in detail elsewhere. Future expect() is the right API for expressing "you can await an expectation", and while it's unfortunate that it ran into flaws in our ecosystem, it's good to have those flaws exposed as early as possible so we can address them.

[zoechi]

If we consider any API change "breaking" if it causes optional potentially-contradictory lints to produce new warnings, then it becomes next to impossible for authors to know whether or not their changes are "breaking".

I don't think it's about API changes not to be allowed to break lints.
It's about a real problem that had an acceptable solution, that your API change rendered useless.

I still find introducing an asyncExpect() the best suggestion so far.

We can't "roll back" a released package

You can just release another update with the changes removed. That wouldn't cause much harm considering how recent the previous release was published.

[ochafik]

I might miss some context, but I think the best way to go is to just special-case expect from package:unittest & package:test, given their pervasiveness and given that the lint already special-cases other APIs. I'll prepare a PR.

[nex3]

@ochafik No need; @dgrove told me to remove the API so I'm doing so. We do still need a user-serviceable way to disable this lint on an API-by-API basis, though, to avoid future compatibility problems.

[pq]

@nex3 's change landed in dart-lang/test#546. I think we're on our way to closing this...

@tvolkert: can we bump Flutter to test 0.12.20?

[ochafik]

It's not clear to me if it's currently possible to disable a lint with an // @ignore[_once] lint_name comment. I think it would be important to support this, and it would provide a cheap workaround for users before a better api-whitelisting mechanism is put in place. WDYT?

[alexeieleusis]

It is supported, intellij has a keyboard shortcut for it Alt+Enter

[pq]

@ochafik : totally possible.

Lints (like all errors) can be suppressed per-line.

https://www.dartlang.org/guides/language/analysis-options#excluding-lines-within-a-file

[natebosch]

I've hit this as well, I think it would be a good idea to give this option to method authors.

In package:build we have an AssetWriter class with methods returning Future. We have a BuildStep which implements AssetWriter.
most of the code which uses an object as an AssetWriter should await, all of the code that uses a BuildStep doesn't need to await.

We considered removing the implements and changing the BuildStep version to void, but being able to pass it around as an AssetWriter is convenient enough in some narrow circumstances that we have left it.

[matanlurey]

Just so I understand correctly, FutureOr still triggers this lint. Can we fix please?

[alexeieleusis]

I think it has been fixed, probably just a matter of releasing a new version.

[pq]

New version just pushed to the SDK:

dart-lang/sdk@8fba8cc

Should be in the next dev roll.

[zoechi]

Can this be closed?

[nex3]

Please re-open this—we still need a way to mark methods that return futures as not needing awaits.

[nex3]

Thanks!

[tvolkert]

@nex3 it seems like more modern constructs of the Dart SDK that didn't exist 15 months ago make this problem easily solvable now -- and obviate the need for expectLater(). What if expect were just changed to:

FutureOr<void> expect(...);

Then, callers could await if they needed to, but the linter wouldn't force them to.

Thoughts?

[nex3]

It seems like the idea of whether a given future should always be awaited should be orthogonal to the type of that future. Also, FutureOr is something we discourage APIs from returning, because it makes it harder for callers to deal with it in a uniform way, so keying off that as a return type feels strange to me. I'd much rather give API authors a way of explicitly indicating whether a future can be left dangling.

[jamesderlin]

Is this still a problem? I haven't noticed any problems with using unawaited_futures and expect(). Also, I tried the reproduction steps from #419 (comment) with unawaited_futures enabled, and I don't see any unawaited_futures warnings.

[eernstg]

tl;dr Summary at the end rd;lt

@nex3 wrote:

we still need a way to mark methods that return futures as not needing awaits.

That part has been resolved: A function can have the async modifier on its body and return void:

void fireAndForget() async {
   ...
}

and that combination is specifically allowed in order to support the scenario where the caller should not await the returned future (that is, fire-and-forget by design, not by accident).

The lint avoid_void_async tries to get rid of these functions entirely (and I agree that they are quite likely to arise by accident, e.g., when someone adds async to a void function and forgets to change the return type to Future<void>). But each such function can still be marked // ignore: avoid_void_async along with an explanation why this particular function is actually intended to perform a fire-and-forget action.

Concerning the return type of expect:

The type FutureOr<void> may look good here, but it is basically a bad idea to use that type at all. It implies (by essentially being the union type Future<void> | void) that we can receive a result of type Future<void> or a result of type void. But there is no way to discriminate which one we have (because void means "any object whatsoever, but it is intended to be ignored", so even if we get a Future<T> for some T, it could have been obtained by evaluating an expression of type void), so there is no safe way to handle a value of that type (so FutureOr<void> is just a verbose way to spell void that furthermore disables some useful static checks).

If you want to say that a function may return a Future<void> (that should be awaited) or it may return "not a Future<void>" (and that shouldn't be awaited), the obvious approach is to have return type Future<void>? (when we get NNBD, such that we can actually say this explicitly). As long as we don't have NNBD we can use Future<void> as the return type and actually return null as needed. This ensures that the "don't await" case can be detected safely by == null.

An async function can't return null, of course, so it's a separate issue (dart-lang/language#606) whether they could be adjusted to do so. But currently that Future<void>? returning function would have to omit async and use one of the obvious workarounds (like doing the things that rely on await in a separate helper function which is async).

With that, it would make sense to think carefully about lints on values of type Future<T>?. This particular scenario seems to imply that it should be allowed to discard such values, but that could also be controlled by something like an @allowIgnoreReturnedValue annotation on the function if it turns out to be more useful to push developers in the direction of (1) checking whether the value is null, and (2) awaiting the future, if any.

This covers Future<void> quite well, but it doesn't cover the situation where a function needs to return a Future<T> or a T; but in that case we would consider the type FutureOr<T> where T would presumably not be a top type, and certainly not void, and in that case there's nothing wrong with the type FutureOr<T>.

I think the summary would then be:

• Fire-and-forget by design can be expressed in Dart today: void f(...) async ....
• expect could return Future<void> (come NNBD: Future<void>?).
• Linter support for allowing the returned value from expect to be ignored should exist, either by making unawaited_futures abstain from flagging discarded values of type Future<T>?, or by allowing expect itself to carry metadata that makes the linter suppress that lint.

So if we keep this issue open it should probably focus on that third bullet: How to treat values of type Future<T>? in unawaited_futures.

[natebosch]

That part has been resolved: A function can have the async modifier on its body and return void

This doesn't resolve the original issue. The original issue is to be able to design an API that may be awaited, but to express that it is safe to not await it. A return type of void can be awaited, but this is a bug in the language. We had hoped to make it a static error but couldn't clean up code in time for Dart 2.

That said, I'm only personally aware of 2 use cases and both have been just fine as is. expect no longer returns a Future, we have expectLater for that use case. Ideally we could have kept those as the same API, but the workaround has been satisfactory. In the AssetWriter case we've stopped worrying about letting authors know that they don't need to await it. Performance differences are only going to come up in rare situations, and then are not likely to be significant, so we switched our samples and our own usages to include the await.

• expect could return Future<void> (come NNBD: Future<void>?).

Future<void> would not work, it would trigger the problem that spawn this thread. Future<void>? may work depending on how we decide to treat it in the linter, but I think it's not a great solution. We don't want to express "you can await this if it's non null and don't need to await if it was null", we don't know on the API side whether to return null or not. It's the consumer side that has the information to decide whether to await. The problem was that unawaited is too noisy for this case because the majority of consumers of the API don't need to await, and shouldn't need to express that every time.

• Linter support for allowing the returned value from expect to be ignored should exist, either by making unawaited_futures abstain from flagging discarded values of type Future<T>?, or by allowing expect itself to carry metadata that makes the linter suppress that lint.

The metadata was the current focus of this thread. IMO Future? is an orthogonal discussion since neither of the 2 cases that have been brought up would be better off with that as their return type. Maybe we should start a separate thread with a discussion of Future? and how it should be handled in this lint? I do think that, similar to FutureOr, Future? should be avoided as a return type the vast majority of the time.

[eernstg]

@natebosch responded to (| |) and wrote (|):

That part has been resolved: A function can have the async modifier on its
body and return void

This doesn't resolve the original issue.

Right, I mentioned it because this issue was reopened specifically in order to address that particular need. However, the other part remains:

to express that it is safe to not await it .. Future<void>? may work .., but
I think it's not a great solution.

Agreed, and thanks for making that point more forcefully than I did. I wasn't sure, and said that we'd need to 'think carefully', but at this point I agree that it's not a good use of that type.

The robust way to express that it's safe to not await a function result would be to use metadata (like @allowIgnoreReturnedValue) that the linter knows, on the declaration of the function. There is no return type which will adequately describe this situation.

OTOH, Future<void>? is the most appropriate description of the situation where it's up to the callee to indicate whether the result should be awaited: If a future is returned then it should be awaited, otherwise null is returned, and the caller can safely use the == null test to detect which case is at hand.