Open pq opened 2 years ago
Looks good! I always thought that it is a footgun to allow futures to be discarded silently just because someone forgot to put async
on the enclosing function. ;)
I assume that unawaited
would still work to mark exceptions to the rule, even in a sync environment.
Thanks for taking a look @eernstg! If you have any thoughts for how we can best communicate the nuances in our docs, I'd really welcome any contributions. 😄
I assume that unawaited would still work to mark exceptions to the rule, even in a sync environment.
Ah! This is a great point and I admit I'd overlooked it. It does sound reasonable to me. Thanks!
(EDIT: description updated.)
I'm also happy to help with the docs, though it seems like it ought to be fairly easy to describe using the modifiers that go on the function body.
Note that I've started looking at re-writing some of the lint docs in preparation for integrating the two diagnostic documentation locations.
I'm also happy to help with the docs
Fantastic. Thank you!
Note that I've started looking at re-writing some of the lint docs in preparation for integrating the two diagnostic documentation locations.
Super! 🚀
Is there a lint family just below the surface here, with the common topic "you forgot to make this function async
"?
Maybe? I've been thinking about this too. I do wonder though if in practice if we can tell the difference between places where someone intends for a function to be async (and just forget to mark it so) and places where they didn't realize they were calling something async (and may not want to...)
Re-opening for further discussion...
Playing around with this on analyzer
, I'm seeing some interesting patterns.
For example:
void setupWatcher() {
var watchers = provider._pathToWatchers[path] ??= [];
watchers.add(streamController);
streamController.done.then((_) {
watchers.remove(streamController);
if (watchers.isEmpty) {
provider._pathToWatchers.remove(path);
}
});
ready.complete();
}
(In memory_file_system.dart
.)
~My inclination after chatting w/ @bwilkerson is to make a special case for Futures returned from .then
and not have them trigger this lint.~ (EDIT: reconsidered.)
There are a bunch of places where we're discarding futures in constructor bodies.
For example,
EvictingFileByteStore(this._cachePath, this._maxSizeBytes)
: _fileByteStore = FileByteStore(_cachePath) {
_requestCacheCleanUp(); // <= async
}
(In file_byte_store.dart
.)
I don't love this idiom and it seems pretty error prone. That said, it's so common and hard to neatly fix, I worry about occurrences making the lint too hard to adopt. (One option is to ignore these cases and add another rule to identify async_constructors
...)
/fyi @munificent @eernstg @scheglov
🆘 Comments welcome!
The tidiest way to cleanup analyzer would be to tag some fire-and-forget functions as proposed in https://github.com/dart-lang/sdk/issues/46218. My sense is that other API adoptions of this lint would similarly benefit from such an annotation.
Why do we want to ignore Future
s returned from then
? It is just another Future
which you might forget to await for its side effects, moreover this will also bypass awaiting for the original Future
to which then
is attached. It seems to me that we should use unawaited
as for any other function.
Using fire-and-forget for some methods sounds interesting. But I don't know where else we have such invocations in constructors, we definitely could have them, but I don't remember where :-)
I feel these 3 are good examples where adding unawaited makes things clearer, so the lint is working as intended?
Why do we want to ignore
Future
s returned fromthen
?
My original concern was that unawaited
is the only way to terminate a chain of then
calls. But I can see the argument that that's a good thing because it signals to the reader that the future is intentionally being ignored. I could live with that, and we can always relax the lint later.
(Disclaimer: I'm going entirely by the description, I haven't checked what the lint actually does.)
Calling asynchronous functions in non-async
functions is not the same as discarding a future. The first line of the details makes it sound like calling async functions is a problem. If the problem is an unhandled/dropped future in a non-async
function, like unawaited_future
is for async
functions, I think the lint should just focus on that.
Which means doing asyncFunction().ignore()
or unawaited(asyncFunction())
should not be a problem.
Neither should Future<int> foo() => test ? asyncFunc1() : asyncFunc2();
be. That's perfectly good code. Requiring it to be async
is a very strong style choice, and definitely not something I want to have enabled by default in the public.
Same for void doStuff() { logAsync(asyncFunction()); }
where logAsync
expects a future or dynamic
. (If it expects Object
or void
, I'd probably warn about dropping a future anyway.)
So, If we want the style lint of "always use async
if you deal with futures", then that can be a different lint from "don't drop futures in non-async
functions either" (the non-async
variant of unawaited_futures
).
I'd be fine with the latter in package:lints
recommended, but definitely not the former, so maybe having two separate lints is better than doing both in one lint. More focused, easier to get partially accepted.
Any tagging we use for unawaited_futures
to mark some futures "ignorable" would apply equally here.
(Undisclaimer: I have now checked a little. The lint does complain about stream.forEach(...).ignore();
, but not unawaited(stream.forEach(...));
. The similar unawaited_futures
doesn't complain for either.
On the other hand unawaited_futures
complains about Future.value(42);
and discarded_futures
does not. So it's not about discarded futures, it's about any async operation except if unawaited
, not expressions of type Future
(and it doesn't recognize a Future
constructor as an async operation). That's definitely too strong a lint for my taste.)
I enabled discarded_futures
in my project and overall it's a good lint for the case that it was proposed. However, there's a specific pattern that we use a lot and is being warned, which is assigning future to variables. This usually happens in Flutter's initState
or in class constructor.
late final Future<Something> myFuture;
// ...
@override
void initState() {
myFuture = asyncFunction();
}
// Use myFuture elsewhere, like in `build`
I can workaround it by using the Future
constructor:
@override
void initState() {
myFuture = Future(asyncFunction);
}
However, it does not seems ideal, and the result is exactly the same except that it's more verbose.
This causes like hundreds of warnings in our codebase where we had to wrap things inside Future
constructor, so I decided to enable the lint only to fix the proper cases (like the lint example) and then disable it.
Not certain this is the right lint but...
I have another unwaited scenario that the current lints miss.
Essentially you can pass an async callback to a function that expects a sync function.
The output from the below program is:
1
from 1
from 2
from 4
2
4
The expected output is:
1
after 1
2
after 2
4
after 4
void main(List<String> args) {
/// good - no error as expected
withSync<void>(() => syncCallback(1));
print('after 1');
/// bad - no error - should error on unawaited future
withSync(() => asyncCallback(2));
print('after 2');
// bad - has error as expected
// withAsync(() => syncCallback(3));
// good - no error as expected
withAsync(() => asyncCallback(4));
print('after 4');
}
Future<R> withAsync<R>(Future<R> Function() callback) => callback();
R withSync<R>(R Function() callback) => callback();
Future<void> asyncCallback(int count) =>
Future.delayed(const Duration(seconds: 3), () => print(count));
void syncCallback(int count) => print(count);
I faced a situation that seems to conflict with this linter rule. ~The only way to fix the warning is by supressing it.~
void main() async {
final x = createX(1);
print(await x.future);
}
X createX(int value) => X(createFuture(value));
Future<int> createFuture(int value) => Future.value(value);
class X {
X(this.future);
final Future<int> future;
}
Calling createFuture
inside createX
triggers the linter rule.
But the intention is indeed to inject a Future in X
constructor.
Both suggested fixes won't work.
unawaited
around createFuture
will of course return the wrong object to X
constructor.
Adding async
to createX also does not make sense, since I want to return X
, not Future<X>
[EDIT]: I can see that this issue is similar to the one raised by @mateusfccp .
With that in mind, I can fix the problem by modifying createX
to
X createX(int value) => X(Future(() async => createFuture(value)));
Which is not so pretty, and also not exactly the same since it seems to require more operations on the event queue than the original solution.
@bsutton wrote:
/// bad - no error - should error on unawaited future
If there's no lint for this case then it looks like a false negative:
R withSync<R>(R Function() callback) => callback();
Future<void> asyncCallback(int count) =>
Future.delayed(const Duration(seconds: 3), () => print(count));
void main(List<String> args) {
withSync(() => asyncCallback(2));
}
Inference should make it withSync<Future<void>>(() => asyncCallback(2))
, which makes the invocation an expression of type Future<void>
, and the value is discarded. Looks like it should be a case for discarded_futures
.
@eernstg so do i need to raise this as a separate issue or is the not here sufficient to get it actioned?
This is a significant problem which I managed to stumble over on a regular basis.
@bsutton any chance https://dartcodemetrics.dev/docs/rules/common/avoid-passing-async-when-sync-expected might help?
file@incendial It doesn't look like the lint helped.
I updated my analysis_options.xml to:
include: package:lint_hard/all.yaml
linter:
rules:
avoid-passing-async-when-sync-expected: true
Ran pub get.
No additional errors were displayed.
Edit: change false to true and re-confirmed that there was still no result. Edit2: I've attached my sample project for future reference.
@bsutton this rule is not a part of standard analyzer / linter set. In order to make it work, you need to install and configure dart_code_metrics
package https://dartcodemetrics.dev/docs/getting-started/installation
Ahh.
This is a fundamental problem so really belongs in the core lints.
On Mon, 28 Nov 2022, 7:10 pm Dmitry Zhifarsky, @.***> wrote:
@bsutton https://github.com/bsutton this rule is not a part of standard analyzer / linter set. In order to make it work, you need to install and configure dart_code_metrics package https://dartcodemetrics.dev/docs/getting-started/installation
— Reply to this email directly, view it on GitHub https://github.com/dart-lang/linter/issues/3429#issuecomment-1328690513, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OFQNWYRKEVVBMAQPG3WKRSGNANCNFSM5XB5JM7A . You are receiving this because you were mentioned.Message ID: @.***>
@lrhn mentioned the following distinction:
[This lint could mean] "always use async if you deal with futures", ... [or it could mean] "don't drop futures in non-async functions" ... (the non-async variant of unawaited_futures). I'd be fine with the latter in package:lints recommended, but definitely not the former,
I assumed that the goal was to express the latter, "don't drop futures in non-async functions", but taking a fresh look at the description, I get the impression that it is in fact the former.
@bsutton wrote, in response to my comment here:
do i need to raise this as a separate issue
If the lint is only expected to flag every expression in a non-async function body whose type is of the form Future<...>
then there's no false negative after all. This matches the description, and it matches the case mentioned here, where a future is computed and assigned to an instance variable.
However, in that case it could be useful to reconsider the name. There's no doubt that this lint is targeting function bodies that aren't async
or async*
; however, proceeding from that starting point, discarded_futures
really sounds like it's about flagging situations where a future is discarded, not just every situation where a future is computed.
Conversely, we could keep the name discarded_futures
and change the semantics of the lint to detect situations where a future is discarded. Surely that's more work, but the resulting lint could also, arguably, be more useful.
@pq, WDYT?
This is a fundamental problem so really belongs in the core lints.
@bsutton maybe, but I was wondering whether this rule actually does what you want. If so, you can enable it while this issue is being discussed, for instance. At least it will resolve your problem for now.
@incendial Understood and appreciated the thought.
The primary aim of my post was to help with the task of dart plugging all the holes where a user can accidentally drop a future hence my comment that this needs to be a core lint.
Drop futures are insidious bugs which are hard to find and can cause enormous damage (I spend a lot of time doing cli programming and out of order operations can do a lot of damage).
Let me try to write up how I'd specify a "future must not be forgotten" rule:
A future must always be handled, either by an await
, or by calling methods on it (where most create new futures), or by passing it to another function which is then expected to handle it.
It's undecidable in general whether a value is used, so these heuristics are used:
Future<T>
, or FutureOr<F>
or F?
where F
is such a type) must not occur in a position where its value is not used:
for(here;;here)
-loop... ? here : here
) whose value is not used.dynamic
.
Object
, Object?
or void
type.The "expression of expression statement" probably covers 99% of actual use-cases.
Sounds good!
A potential future
This concept could be built on top of the notion of 'the future type of' a given type, cf. https://github.com/dart-lang/language/blob/9bdd033fbb8d98543cd770bb3832f70f629dad2b/specification/dartLangSpec.tex#L11310.
Request to change how discarded_futures
is currently implemented:
Currently this code triggers the warning:
// State of a StatefulWidget, say.
Future<Uint8List>? imageData;
// Called in a Button callback, triggers the warning
imageData = PlatformImageFetcher.getImage();
I'd argue that assigning a future resulting from an async method call to a variable, should not be considered a discarded variable.
As long as this still generates a warning:
var imageData = PlatformImageFetcher.getImage();
then I would agree with Brett Morgan.
On Tue, Jan 17, 2023 at 2:04 PM Brett Morgan @.***> wrote:
Request to change how discarded_futures is currently implemented:
Currently this code triggers the warning:
// State of a StatefulWidget, say.Future
? imageData; // Called in a Button callback, triggers the warning imageData = PlatformImageFetcher.getImage(); I'd argue that assigning a future resulting from an async method call, assigned to a variable, should not be considered a discarded variable.
— Reply to this email directly, view it on GitHub https://github.com/dart-lang/linter/issues/3429#issuecomment-1384776104, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OB3KYX2WVPCORMAJG3WSYD3PANCNFSM5XB5JM7A . You are receiving this because you were mentioned.Message ID: @.***>
@bsutton can you explain why assigning a future should trigger a discarded future warning? I'm not meaning to be insulting, I'm just needing an ELI5 moment. Thanks!
@bsutton can you explain why assigning a future should trigger a discarded future warning? I'm not meaning to be insulting, I'm just needing an ELI5 moment. Thanks!
Because it's almost never the intent. I believe the lint should warn the user if they did something they didn't intend to do.
I regularly assign a future to a var and then only realise my mistake one I start to reference the var. There may be an argument that the subsequent error is sufficient .
I assume the follow will trigger the lint
callafutureAndIgnoreReturn();
My preference would still be that if you want the future then you need to explicitly define the return type as this will make for better static analysis by a human and the ide will do the work to convert the declared type to an explicit future, so they is no work in it for the developer.
Missing awaits in dart can be lethal and incredibly hard to debug which is why I would advocate on the side of an agressive lint.
The handling of futures is darts achilies heal and currently is incredibly clumsy.
With future we need to ensure that the developers intent is always explicit.
PS no need to apologise for a asking for a justification.
The issues I have in https://github.com/flutter/samples/pull/1572 and https://github.com/flutter/codelabs/pull/1348 are where I have code that assigns to variables that are explicitly typed as Future
s.
Missing awaits in dart can be lethal and incredibly hard to debug which is why I would advocate on the side of an agressive lint.
Can you elaborate on this point or point me to documentation that describes the problem? The Flutter examples I rely on mostly are navigator and they don't follow anything related to this lint. Are there side effects related to letting Futures be ignored and going out of scope?
https://api.flutter.dev/flutter/widgets/Navigator/pushNamed.html The easiest example is this and it doesn't do anything as async or awaited.
void _didPushButton() { Navigator.pushNamed(context, '/settings'); }
The other pattern I have been using (and may change due to this) is
ShowDialog
I do a lot of cli programming.
await cd('/');
cd('/tmp');
await deleteTree();
Potentially, you just deleted your entire file system.
On Fri, 21 Apr 2023, 7:04 am russellminnich, @.***> wrote:
Missing awaits in dart can be lethal and incredibly hard to debug which is why I would advocate on the side of an agressive lint.
Can you elaborate on this point or point me to documentation that describes the problem? The Flutter examples I rely on mostly are navigator and they don't follow anything related to this lint. Are there side effects related to letting Futures be ignored and going out of scope?
https://api.flutter.dev/flutter/widgets/Navigator/pushNamed.html The easiest example is this and it doesn't do anything as async or awaited.
void _didPushButton() { Navigator.pushNamed(context, '/settings'); }
The other pattern I have been using (and may change due to this) is ShowDialog().then( // extract any data that I need from the data returned by Navigator pop );
— Reply to this email directly, view it on GitHub https://github.com/dart-lang/linter/issues/3429#issuecomment-1516947599, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OC6CZHOYLZK5ZMKYK3XCGQHPANCNFSM5XB5JM7A . You are receiving this because you were mentioned.Message ID: @.***>
Don't know if this is the right place, however I noticed that discarded_future
does not warn when not awaiting a future inside a function returning FutureOr
.
Future<void> doSomething() async {}
FutureOr<void> foo() {
doSomething(); // <= Expecting warning
}
I think maybe https://github.com/dart-lang/linter/issues/4260
IMHO the linter should not warn when the future is converted into a Stream
via asStream()
. At the moment I need to set the ignores manually.
final initialStream = ReceiveSharingIntent.instance
// ignore: discarded_futures
.getInitialMedia()
// ignore: discarded_futures
.then((event) async {
await ReceiveSharingIntent.instance.reset();
return event;
}).asStream();
discarded_futures
Description
Don't discard futures in async functions.
Details
Making asynchronous calls in non-
async
functions is usually the sign of a programming error. In general these functions should be markedasync
and such futures should likely be awaited (as enforced byunawaited_futures
).In case you really do want to discard a Future in a non-
async
function, the recommended way is to useunawaited
fromdart:async
. The// ignore
and// ignore_for_file
comments also work.Kind
Error
Good Examples
Bad Examples
Discussion
See #836, #2953 and
unawaited_futures
./fyi @eernstg @diegovar @dowski @bsutton
Discussion checklist