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

proposal: discarded_futures #58747

Closed
5 tasks done
pq opened this issue May 26, 2022 · 44 comments · Fixed by dart-lang/linter#3431
Closed
5 tasks done

proposal: discarded_futures #58747

pq opened this issue May 26, 2022 · 44 comments · Fixed by dart-lang/linter#3431
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-proposal P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@pq
Copy link
Member

pq commented May 26, 2022

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, #58512 and unawaited_futures.

/fyi @eernstg @diegovar @dowski @bsutton

Discussion checklist

  • List any existing rules this proposal modifies, complements, overlaps or conflicts with.
  • List any relevant issues (reported here, the SDK Tracker, or elsewhere).
  • If there's any prior art (e.g., in other linters), please add references here.
  • If this proposal corresponds to Effective Dart or Flutter Style Guide 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.
@eernstg
Copy link
Member

eernstg commented May 27, 2022

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. ;)

@bwilkerson
Copy link
Member

I assume that unawaited would still work to mark exceptions to the rule, even in a sync environment.

@pq
Copy link
Member Author

pq commented May 27, 2022

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.)

@bwilkerson
Copy link
Member

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.

@pq
Copy link
Member Author

pq commented May 27, 2022

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! 🚀

@eernstg
Copy link
Member

eernstg commented May 27, 2022

Is there a lint family just below the surface here, with the common topic "you forgot to make this function async"?

@pq
Copy link
Member Author

pq commented May 28, 2022

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...)

@pq
Copy link
Member Author

pq commented Jun 14, 2022

Re-opening for further discussion...

Playing around with this on analyzer, I'm seeing some interesting patterns.

1. Future.then

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.)

2. Async Constructors

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!

@pq
Copy link
Member Author

pq commented Jun 14, 2022

3. Fire and Forget Functions

The 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.

@scheglov
Copy link
Contributor

Why do we want to ignore Futures 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 :-)

@diegovar
Copy link

I feel these 3 are good examples where adding unawaited makes things clearer, so the lint is working as intended?

@bwilkerson
Copy link
Member

Why do we want to ignore Futures returned from then?

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.

copybara-service bot referenced this issue Jun 15, 2022
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>
@eernstg
Copy link
Member

eernstg commented Jun 16, 2022

About the declaration site opt-out that @pq mentioned here: We could also use a union type in the declaration to out out, as described here.

@lrhn
Copy link
Member

lrhn commented Jun 16, 2022

(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.)

@mateusfccp
Copy link
Contributor

mateusfccp commented Oct 26, 2022

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.

@bsutton
Copy link

bsutton commented Nov 16, 2022

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);

@gabrielgarciagava
Copy link

gabrielgarciagava commented Nov 25, 2022

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.

@eernstg
Copy link
Member

eernstg commented Nov 25, 2022

@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.

@bsutton
Copy link

bsutton commented Nov 26, 2022

@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.

@incendial
Copy link
Contributor

@bsutton
Copy link

bsutton commented Nov 27, 2022

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.

async_callbacks.zip

@bsutton
Copy link

bsutton commented Nov 28, 2022 via email

@eernstg
Copy link
Member

eernstg commented Nov 28, 2022

@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?

@incendial
Copy link
Contributor

incendial commented Nov 28, 2022

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.

@bsutton
Copy link

bsutton commented Nov 28, 2022

@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).

@srawlins srawlins added the P3 A lower priority bug or feature request label Jan 3, 2023
@lrhn
Copy link
Member

lrhn commented Jan 8, 2023

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:

  • A potential future (an expression of type 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:
    • Expression of an expression statement
    • Initializer or increment expressions of a for(here;;here)-loop
    • Expression of a parenthesized expression whose value is not used.
    • Non-test expression of a conditional expression (... ? here : here) whose value is not used.
    • Element of a set or map literal whose value is not used.
    • Key or value of a map literal whose value is not used.
    • (To come: field value of a record expression whose value is not used.)
  • A potential future must not be assigned to a non-potential-future type other than dynamic.
    • Must not occur in a context expecting an Object, Object? or void type.
  • Must not be assigned to a local variable which is never read again.
    • (Piggyback on the existing analyzer "unused value" analysis).

The "expression of expression statement" probably covers 99% of actual use-cases.

@eernstg
Copy link
Member

eernstg commented Jan 8, 2023

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.

@domesticmouse
Copy link
Member

domesticmouse commented Jan 17, 2023

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.

@bsutton
Copy link

bsutton commented Jan 17, 2023 via email

@domesticmouse
Copy link
Member

@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
Copy link

bsutton commented Jan 17, 2023

@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.

@domesticmouse
Copy link
Member

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 Futures.

@russellminnich
Copy link

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
);

@bsutton
Copy link

bsutton commented Apr 20, 2023 via email

@saibotma
Copy link

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
}

@FMorschel
Copy link
Contributor

I think maybe #59105

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 27, 2024
@FaFre
Copy link

FaFre commented Apr 10, 2024

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();

@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 19, 2024
@bsutton
Copy link

bsutton commented Nov 19, 2024 via email

@bsutton
Copy link

bsutton commented Nov 19, 2024 via email

@FMorschel
Copy link
Contributor

@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.

@pq
Copy link
Member Author

pq commented Jan 8, 2025

Yeah. I think it would be reasonable to close this and move ongoing conversation to new issues.

@FMorschel
Copy link
Contributor

FMorschel commented Jan 9, 2025

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.

@pq
Copy link
Member Author

pq commented Jan 9, 2025

Ah, great. I'll close this one and we can follow-up on the listed issues.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-proposal P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.