proposal: `discarded_futures`

[pq]

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

/fyi @eernstg @diegovar @dowski @bsutton

Discussion checklist

• [x] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• [x] List any relevant issues (reported here, the SDK Tracker, or elsewhere).
• [x] If there's any prior art (e.g., in other linters), please add references here.
• [x] 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?)
• [x] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[eernstg]

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]

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

[pq]

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]

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]

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]

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

[pq]

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]

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]

3. Fire and Forget Functions

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.

[scheglov]

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]

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

[bwilkerson]

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.

[eernstg]

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]

(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]

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]

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]

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]

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

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

@bsutton any chance https://dartcodemetrics.dev/docs/rules/common/avoid-passing-async-when-sync-expected might help?

[bsutton]

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

[incendial]

@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

[bsutton]

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

[eernstg]

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

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]

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

[lrhn]

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]

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]

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]

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