-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
consider an annotation to tag async members as not needing await #46218
Comments
Do we want something fairly specific, like |
Excellent question. My gut says we want the specificity of something like |
In another thread where this was discussed (dart-lang/linter#2513 (comment)) Hixie suggested |
Which leads to the next question: are there any static checks associated with For example, I could imagine checking some or all of the following:
|
I think order of execution is more of a concern with missing 'await' than exception handling? At least, in web and web test code that was the concern. I don't remember anything specifically about exception handling. So what I was expecting is:
and for that I'm not sure about the requirement to not throw; it seems to me like it's a valid design to have unawaited futures that throw, and rely on the zone exception handler to catch them? |
Also, it's quite possible that you have a method that can never throw that you always want to await, e.g.
So I don't think that works by itself. |
Just noted this issue, and I can't help mentioning a thing: A fire-and-forget async function can be expressed by using the return type void fireAndForget() async { ... }
void main() async {
fireAndForget(); // No complaints about unawaited futures.
} |
Thanks @eernstg! I've wondered this too and it's exactly why I think some specific API examples would be handy. |
In dart-lang/lints#25 (comment), @goderbauer mentioned https://api.flutter.dev/flutter/animation/AnimationController/forward.html In this and related cases a |
These methods return Futures because sometimes you do want to await them (although that's rare), but in the most common cases you should be able to just drop them on the floor without having to litter your code with |
@goderbauer: I'm curious how you'd feel about adopting an |
|
I would like to see For example: Map.remove called on a |
See also: dart-lang/linter#2513. |
/fyi @lrhn @leafpetersen @natebosch @jakemac53 If this is something we agree we want to do, it will suggest the annotation living in core (not EDIT: pragmatically, if there are only a few core library declarations we want to tag, we could bake that awareness into the lint and sidestep the challenge of landing a new annotation in the SDK. (Not tidy but there's certainly precedent. 😬 ) |
As for @jacob314's example, I do wonder if that might not be better addressed in the lint. That one feels like a false positive to me. |
Another request that came up for this would be that if a @awaitNotRequired
Future<void> doNotAwaitMe() async {
print('ok');
}
Future<void> awaitMe() async {
print('I sure hope you awaited this');
}
// Should be transitively @awaitNotRequired
Future<void> someFunction() async {
await doNotAwaitMe(); // or don't await it, shouldn't matter.
print('hi');
}
// Lint error to not await this function
Future<void> someOtherFunction() async {
await awaitMe();
print('hi');
}
|
@dnfield Do we have an actual use case for that in the framework? |
I think that request means that such a function is treated as if it had been explicitly annotated without requiring an explicit annotation. If so, I'm not positive, but I suspect that that's something we can't efficiently implement. (The only implementation I'm currently thinking of would be to create and incrementally maintain the call graph for all the code being analyzed, or at least for What we could do is add a lint to flag any such functions that aren't explicitly annotated as needing to be annotated, but such a lint might produce a lot of noise. |
@Hixie was suggesting this. My impression is that we'd want to avoid needing to go up the chain of every function in the framework that calls annotated functions and avoiding awaiting them. As I think about it, I wonder why that wouldn't be a good thing - we'd explicitly make choices about whether we really meant for a specific function to be awaited or not. But maybe @Hixie has some example I'm not thinking of. |
Adding inference for this behavior would make it too fragile - local edits could have unpredictable consequences. |
A Likewise, a function only calling I don't particularly want lint-related annotations in the platform libraries. A Ignoring methods which happen to return a future due to generics, but which aren't inherently asynchronous seems somewhat reasonable, but also risky. It is a future, and it's impossible to know whether someone waited for it or not. |
Hi, is there any updates :) |
It was noted that even though we have support for fire-and-forget One possible approach for this could be to use a nullable return type: // The return type `Future<...>?` could be used to indicate that "`await` is not required".
// We _can_ do this even in the case where the returned object will never actually be null,
// e.g., because the function body has the modifier `async`. It may seem odd to do so, but
// we would do it exactly because it's a signal to the lint, and it wouldn't be much of a problem
// that the call site has an artificial null-check if the return value is almost always ignored.
Future<int>? foo() async {...}
// This kind of return type is already usable in the case where we may or may not receive
// a `Future` at run time.
Future<int>? foo2() {
return someCondition ? Future<int>.value(someExpression) : null;
} An invocation of Note that A very similar approach is to use the other union type, // Use the return type `FutureOr<...>` to indicate that "`await` is not required".
FutureOr<int> foo3() {
return someCondition ? Future<int>.value(someExpression) : 24;
}
// With `FutureOr`, it is almost always confusing to use it as the return type
// of an `async` function.
FutureOr<int> foo4() async {
// The returned object is a `Future<int>` in any case.
return someCondition ? Future<int>.value(someExpression) : 24;
} Today, an invocation of (So I might actually recommend use FutureOr, rather than 'use a nullable return type'. It just seems really misleading to say that That said, I think we should lint the situation where a function has an This means that the declaration of a fire-and-forget function would be linted, and that's probably a useful approach because they are expected to be rare anyway, and it is probably fine to ask for a special exception in order to declare one. Similarly, "fire-and-forget-with-nontrivial-type" functions like |
It is much better imo to handle this via an explicit annotation compared to lying about the return type as a workaround. Using a nullable type requires users to deal with the null that will never appear, and FutureOr similarly requires users to do a type check that is not necessary. |
I agree that an explicit annotation would be better. My expectation is that when a users sees an |
FWIW the place where this comes up a lot in Flutter framework APIs is around animations. There are lots of APIs that return |
Why wouldn't returning void work? |
@jakemac53 wrote:
@bwilkerson wrote:
I mentioned here that all these functions (both fire-and-forget and fire-and-forget-with-nontrivial-type) should be linted at the declaration (because fire-and-forget-of-any-variant is expected to be rare). That could serve as a reminder to developers (of any such function) that this is an exceptional declaration, and its special nature should be documented. It may still be safer to use an approach where each of the fire-and-forget-with-nontrivial-type functions has an At call sites we have nearly the opposite situation: If the property is signalled by the type of an expression then it will propagate in expressions, which is not the case if we rely on metadata. import 'dart:async';
@awaitNotRequired
Future<int> fooMeta() async => 0;
// Fire-and-forget, should be treated as ignorable.
void bar() async {}
// Use the type to cancel the "should `await`" and "don't discard" lints. Ugly as it is, we use
// `FutureOr`, because it already works.
FutureOr<int> fooType() async => 0;
var someCondition = true;
X id<X>(X x) => x;
void main() async {
fooMeta(); // No lint, as desired.
(fooMeta()); // Lint.
someCondition ? fooMeta() : fooMeta(); // Lint.
id(fooMeta()); // Lint.
bar(); // No lint, which is ok: fire-and-forget.
id(bar()); // Still no lint, as desired.
fooType(); // No lint, as desired.
(fooType()); // Still no lint, in various expressions.
someCondition ? fooType() : fooType();
id(fooType());
} |
If the return type is void, how do you get a hold of the Future if you do want to await it? |
You can still await a void returning function, but you won't be able to get the future object I guess |
|
Ok but that's ten times worse than just not enabling the lint that requires awaits, so, people just don't bother enabling the lint instead of doing that. |
Is this also true in the case where almost all invocations of |
If the API returns a Future we should not document it as returning |
That's lowballing it! I very much want to change the language to automatically throw away values assigned to the static type If you have something that someone sometimes want to await, but mostly don't, I think returning a future is fine, and annotating the function with something to make it not-a-warning to ignore the future is also fine. Heck, we could allow annotation identifiers to refer to method declarations, not just const variable declarations, then you could do |
Implementing this annotation would have a large impact. One code base has over 10K calls to |
💯 In addition to the existing uses of |
(Forward pointer / note to self: if/when we do land this, we should consider a diagnostic to help us identify unnecessary |
As per dart-lang/linter#3651 my use case is in the package dcli which is a heavy user of waitfor wrapped in it's own method waitforex. Users of the dcli library use waitforex which calls waitfor. As such waitforex needs to be marked as not needing to be unawaited whilst being allowed to throw. So the explicit awaitNotRequired would be required in my use case. Here is how waitForEx is used: void main() {
waitForEx(someAsyncFunction());
} Here is the code: T waitForEx<T>(Future<T> future) {
final stackTrace = StackTraceImpl();
late T value;
try {
value = cli.waitFor<T>(future);
}
// ignore: avoid_catching_errors
on AsyncError catch (e) {
Error.throwWithStackTrace(e.error, stackTrace.merge(e.stackTrace));
// ignore: avoid_catches_without_on_clauses
} catch (e, st) {
Error.throwWithStackTrace(e, stackTrace.merge(st));
}
return value;
} Edit: added code example. |
Follow-up from conversation in dart-lang/lints#25.
The idea is to add an annotation that allows API authors to mark declarations as not requiring an
await
at call sites. This could greatly reduce the number of uses of functions likeunawaited
to silence theunawaited_futures
lint.The text was updated successfully, but these errors were encountered: