Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

method-side opt-out for unawaited_futures #2513

Open
goderbauer opened this issue Mar 16, 2021 · 12 comments
Open

method-side opt-out for unawaited_futures #2513

goderbauer opened this issue Mar 16, 2021 · 12 comments
Labels
P2 A bug or feature request we're likely to work on set-none Does not affect any rules in a defined rule set (e.g., core, recommended, flutter) type-enhancement A request for a change that isn't a bug

Comments

@goderbauer
Copy link
Contributor

Flutter has some API methods (like Navigator.push, AnimationController.forward, AnimationController.reverse) that return a Future, but in a lot of cases it is perfectly normal to just drop that Future on the floor. The unawaited_futures lint creates a lot of noise there, especially in tests.

It would be nice if there's an API-side opt-out for that lint where we can annotate the methods mentioned above with @unawaitedOk (or similar) and the lint would ignore it if a Future returned by those annotated methods is unawaited.

/cc @pq @Hixie @mit-mit @munificent @devoncarew

@srawlins
Copy link
Member

Just to be clear, in other cases, you do want to wait for the Future, and access a contained value?

@goderbauer
Copy link
Contributor Author

Yes. For example, if a route pushed via Navigator.push returns a value, you may want to await the Future to retrieve that value.

@pq
Copy link
Member

pq commented Mar 16, 2021

There's some conceptual common ground with the open request for @CheckReturn or similar (see: #1888). It would be cool if we could find a synergetic solution.

@bwilkerson
Copy link
Member

Flutter has some API methods (like Navigator.push, AnimationController.forward, AnimationController.reverse) that return a Future, but in a lot of cases it is perfectly normal to just drop that Future on the floor.

Is there a way that we can statically tell whether it's reasonable to drop the Future? I'm just wondering whether we can avoid both false positives (the status quo) and false negatives (the risk to allowing an annotation on these methods to stop all checking).

@Hixie
Copy link

Hixie commented Mar 17, 2021

It's ok to ignore a future when that future will never complete with an exception. It would be good for the lint to make that clear (e.g. @cannotThrow rather than @unawaitedOk).

@pq
Copy link
Member

pq commented Jun 4, 2021

Annotation implementation in the SDK discussion going on here: dart-lang/sdk#46218.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Jun 18, 2021
@bwilkerson bwilkerson added the set-none Does not affect any rules in a defined rule set (e.g., core, recommended, flutter) label Nov 11, 2022
@pq pq added the P2 A bug or feature request we're likely to work on label Nov 14, 2022
@srawlins
Copy link
Member

srawlins commented Apr 6, 2023

This is a duplicate of what #2513 eventually became, as of #419 (comment). #419 (comment) is a good summary.

@natebosch
Copy link
Member

natebosch commented Jun 3, 2024

Do you have an example diff or diagnostics list from when this was explored in flutter/flutter or flutter/engine?

AnimationController.forward is not used in an async method in flutter/flutter. Navigator.push has a few uses in async methods, almost all were in tests and followed by await tester.pump(); or some variation. I had a hard time finding uses where an await would cause a problem, but there were a lot of non-async context uses to scan through and I didn't go through them all. Specific examples of where these APIs shouldn't be awaited would be helpful.

I'm surprised that the noise of unawaited for the few usages I saw out of the APIs mentioned is considered more of a problem than the risk of futures being silently unawaited today.

Is it intended that errors from these futures should be surfaced only by the zone error handler?

@goderbauer
Copy link
Contributor Author

Turning on the lint on flutter/flutter reports almost 800 issues: https://gist.github.com/goderbauer/2e74e4299b82a7106dc47c012ca9e153.

I haven't checked all of these, but the ones I spot-checked where reported on AnimationController and Navigator APIs, whose futures can usually just be dropped on the floor. Awaiting these in tests unsurprisingly makes the test hang. So, telling people that they are missing an await here is somewhat misleading.

Is it intended that errors from these futures should be surfaced only by the zone error handler?

See Hixie's earlier comment in this thread. These futures are designed to never complete with an exception.

@natebosch
Copy link
Member

These futures are designed to never complete with an exception.

In that case is there any concern with using .ignore() to make these explicitly ignored?

@goderbauer
Copy link
Contributor Author

It's noisy, verbose, and somewhat annoying. If we already know that these can be safely ignored, why do our developers have to spent mental energy on the question of whether ignoring these is fine or not? Also, if developers make the wrong choice and accidentally await these (the lint is pushing them more in that direction then towards ignoring them) they'll have to spent more time debugging why their test is hanging.

@Hixie
Copy link

Hixie commented Jun 6, 2024

The other problem with .ignore() is that it's defined on an extension so when you're looking at code that uses it for the first time, it's very unobvious how to even look it up (e.g. the word "ignore" doesn't even appear on the docs for the Future class). In general I would strongly discourage any use of extensions for this reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on set-none Does not affect any rules in a defined rule set (e.g., core, recommended, flutter) type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants