-
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
proposal: discarded_futures
#58747
Comments
Looks good! I always thought that it is a footgun to allow futures to be discarded silently just because someone forgot to put |
I assume that |
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. 😄
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. |
Fantastic. Thank you!
Super! 🚀 |
Is there a lint family just below the surface here, with the common topic "you forgot to make this function |
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 1. Future.thenFor 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
2. Async ConstructorsThere 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 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 /fyi @munificent @eernstg @scheglov 🆘 Comments welcome! |
3. Fire and Forget FunctionsThe tidiest way to cleanup analyzer would be to tag some fire-and-forget functions as proposed in #46218. My sense is that other API adoptions of this lint would similarly benefit from such an annotation. |
Why do we want to ignore 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? |
My original concern was that |
Fixes: #49262 See also: https://github.com/dart-lang/linter/issues/3429 Change-Id: I0d3bf6c72ae1cf3e08f50b4deda10aba52e25eb4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/248480 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Phil Quitslund <pquitslund@google.com>
(Disclaimer: I'm going entirely by the description, I haven't checked what the lint actually does.) Calling asynchronous functions in non- Which means doing Same for So, If we want the style lint of "always use Any tagging we use for (Undisclaimer: I have now checked a little. The lint does complain about On the other hand |
I enabled late final Future<Something> myFuture;
// ...
@override
void initState() {
myFuture = asyncFunction();
}
// Use myFuture elsewhere, like in `build` I can workaround it by using the @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 |
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:
The expected output is:
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. 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 Both suggested fixes won't work. [EDIT]: I can see that this issue is similar to the one raised by @mateusfccp . 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:
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 |
@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 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. |
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
<#58747>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OFQNWYRKEVVBMAQPG3WKRSGNANCNFSM5XB5JM7A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@lrhn mentioned the following distinction:
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:
If the lint is only expected to flag every expression in a non-async function body whose type is of the form 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 Conversely, we could keep the name @pq, WDYT? |
@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 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
The "expression of expression statement" probably covers 99% of actual use-cases. |
Sounds good!
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 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<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,
assigned to a variable, should not be considered a discarded variable.
—
Reply to this email directly, view it on GitHub
<#58747>,
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! |
Because it's almost never the intent. I regularly assign a future to a var and then only realise my mistake one I start to reference the var. I assume the follow will trigger the lint
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 flutter/samples#1572 and flutter/codelabs#1348 are where I have code that assigns to variables that are explicitly typed as |
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 void _didPushButton() { The other pattern I have been using (and may change due to this) is |
I do a lot of cli programming.
```dart
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
<#58747>,
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 Future<void> doSomething() async {}
FutureOr<void> foo() {
doSomething(); // <= Expecting warning
} |
I think maybe #59105 |
IMHO the linter should not warn when the future is converted into a final initialStream = ReceiveSharingIntent.instance
// ignore: discarded_futures
.getInitialMedia()
// ignore: discarded_futures
.then((event) async {
await ReceiveSharingIntent.instance.reset();
return event;
}).asStream(); |
I have a class FuturebulderEx that takes a future and is intended to be
used in a build method.
So it appears to be a discarded future but it's not.
https://pub.dev/packages/future_builder_ex
…On Wed, 20 Nov 2024, 6:21 am Phil Quitslund, ***@***.***> wrote:
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 marked async and
such futures should likely be awaited (as enforced by unawaited_futures).
In case you *really do* want to discard a Future in a non-async function,
the recommended way is to use unawaited from dart:async. The // ignore
and // ignore_for_file comments also work.
Kind
Error
Good Examples
Future<void> recreateDir(String path) async {
await deleteDir(path);
await createDir(path);
}
Future<void> deleteDir(String path) async {}
Future<void> createDir(String path) async {}
Bad Examples
void recreateDir(String path) {
deleteDir(path);
createDir(path);
}
Future<void> deleteDir(String path) async {}
Future<void> createDir(String path) async {}
Discussion
See #57653 <#57653>, #58512
<#58512> and unawaited_futures
<https://dart-lang.github.io/linter/lints/unawaited_futures.html>.
/fyi @eernstg <https://github.com/eernstg> @diegovar
<https://github.com/diegovar> @dowski <https://github.com/dowski> @bsutton
<https://github.com/bsutton>
Discussion checklist
- List any existing rules this proposal modifies, complements,
overlaps or conflicts with.
- List any relevant issues (reported here, the SDK Tracker
<https://github.com/dart-lang/sdk/issues>, or elsewhere).
- If there's any prior art (e.g., in other linters), please add
references here.
- If this proposal corresponds to Effective Dart
<https://dart.dev/guides/language/effective-dart> or Flutter Style
Guide
<https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo>
advice, please call it out. (If there isn’t any corresponding advice,
should there be?)
- If this proposal is motivated by real-world examples, please provide
as many details as you can. Demonstrating potential impact is *especially
valuable*.
—
Reply to this email directly, view it on GitHub
<#58747>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OBTF5VL6PXEKOUEIP32BOFU5AVCNFSM6AAAAABSC32P72VHI2DSMVQWIX3LMV43ASLTON2WKOZSGY3TGMRWGE3TENI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Ideally I want to be able to mark the interface on FuturebulderEx as 'using
the future' which would then suppress this lint.
…On Wed, 20 Nov 2024, 7:15 am Brett Sutton, ***@***.***> wrote:
I have a class FuturebulderEx that takes a future and is intended to be
used in a build method.
So it appears to be a discarded future but it's not.
https://pub.dev/packages/future_builder_ex
On Wed, 20 Nov 2024, 6:21 am Phil Quitslund, ***@***.***>
wrote:
> 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 marked async
> and such futures should likely be awaited (as enforced by
> unawaited_futures).
>
> In case you *really do* want to discard a Future in a non-async
> function, the recommended way is to use unawaited from dart:async. The //
> ignore and // ignore_for_file comments also work.
> Kind
>
> Error
> Good Examples
>
> Future<void> recreateDir(String path) async {
> await deleteDir(path);
> await createDir(path);
> }
> Future<void> deleteDir(String path) async {}
> Future<void> createDir(String path) async {}
>
> Bad Examples
>
> void recreateDir(String path) {
> deleteDir(path);
> createDir(path);
> }
> Future<void> deleteDir(String path) async {}
> Future<void> createDir(String path) async {}
>
> Discussion
>
> See #57653 <#57653>, #58512
> <#58512> and unawaited_futures
> <https://dart-lang.github.io/linter/lints/unawaited_futures.html>.
>
> /fyi @eernstg <https://github.com/eernstg> @diegovar
> <https://github.com/diegovar> @dowski <https://github.com/dowski>
> @bsutton <https://github.com/bsutton>
> Discussion checklist
>
> - List any existing rules this proposal modifies, complements,
> overlaps or conflicts with.
> - List any relevant issues (reported here, the SDK Tracker
> <https://github.com/dart-lang/sdk/issues>, or elsewhere).
> - If there's any prior art (e.g., in other linters), please add
> references here.
> - If this proposal corresponds to Effective Dart
> <https://dart.dev/guides/language/effective-dart> or Flutter Style
> Guide
> <https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo>
> advice, please call it out. (If there isn’t any corresponding advice,
> should there be?)
> - If this proposal is motivated by real-world examples, please
> provide as many details as you can. Demonstrating potential impact is *especially
> valuable*.
>
> —
> Reply to this email directly, view it on GitHub
> <#58747>, or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAG32OBTF5VL6PXEKOUEIP32BOFU5AVCNFSM6AAAAABSC32P72VHI2DSMVQWIX3LMV43ASLTON2WKOZSGY3TGMRWGE3TENI>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
@pq why is this still open? I see the rule https://dart.dev/tools/linter-rules/discarded_futures is available as of Dart 2.18. |
Yeah. I think it would be reasonable to close this and move ongoing conversation to new issues. |
The most common comment I found regarding this feature is related to this issue: #59504. So if anyone still wants this to be addressed please upvote it. Edit@pq I think all of these are duplicates, but they have different priorities. I'll look at this lint and see if I can make that change.
|
Ah, great. I'll close this one and we can follow-up on the listed issues. Thanks! |
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 #57653, #58512 and
unawaited_futures
./fyi @eernstg @diegovar @dowski @bsutton
Discussion checklist
The text was updated successfully, but these errors were encountered: