discarded_futures lint discussion

[FMorschel]

After some discussion over at #59877, #59331 and #58747, I've seen many users expecting discarded_futures to work similarly to unawaited_futures but inside synchronous code.

That is, inside synchronous code, if the Future is assigned to something that explicitly asks for a Future, do nothing, if not, trigger.

That would solve all of these duplicates that expect the lint not to warn when using it on FutureBuilder from Flutter for example:

• discarded_futures lint false positive #59504
• Possible false positive in discarded_futures lint #58889
• update discarded_futures lint rule to not trigger when a widget expect a futur #59455
• [analyzer] False positives for discarded_futures when passing future-returning function call as an argument #59151
• discarded_futures false positive? #59387
• discarded_futures : Unwaited return for when Future is expected #59331

Making the behaviour match unawaited_futures would also probably fix: #59204.

I've opened https://dart-review.googlesource.com/c/sdk/+/403901 and I've made some changes to address this assignability issue, but @lrhn commented on the CL:

There is no change to the description, documentation or specification of this lint, but a significant change to its behavior.
(As much as a specification exists.)

Among my many beefs with this lint is that its name, description and behavior seem to be three different things. I can't tell if this change makes that better or worse.

The name is "discarded_futures". It's description (the line above and the text at https://dart.dev/tools/linter-rules/discarded_futures) is very clear that it really means "don't call async functions in non-async code".

The implementation seems to refer to the unawaited function in some places, which is not mentioned in the description, and isn't relevant to the specified behavior, so the implementation is either a third thing, or maybe it matches the title better than the description does. (But not well. The unawaited_futures lint doesn't mention the unawaited function it the code. It doesn't have to.)

I'm all for changing the specification and behavior of this lint, because "don't call async functions in non-async code" is a rule I break all the time (I know how to use Future.then). It's not a good lint as specified.

This change seems to try to move it closer to the behavior of unawaited_futures, which is about not leaving a Future unawaited by accident.
That lint understands, I think, that passing a Future as an argument to a parameter of type Future or FutureOr is a way of handling that future.

If this lint was to be changed, I'd probably want to change it to the exact same behavior as unawaited_futures, just in a non-async function (and therefore without the ability to use await). That is, don't throw away a future, which means boils down to to not use an expression with type Future (maybe also FutureOr) in a context that isn't typed as Future or FutureOr. It being a future is lost whether the context is Object or void.

Then unawaited(someFuture) just works because someFuture is in a Future<void> context, and unawaited(someFuture) has type void.

All this special-casing of when it's OK to have a future feels like overkill.
It's OK to have a future in a place where a future is expected, anything else won't know to treat > it as a future.

I'm opening this issue to track this discussion and hopefully to make that change.

// CC @lrhn @pq

[FMorschel]

Just to keep things in one place. The current CL fixes all of the above issues. I'll read through https://gist.github.com/lrhn/0316c1c6c5102dc900d635d08ec31188 and see if there is any missing test here.

I'll also address to the linter/analyzer team the following question:

With the current changes, if the Future is assigned to an explicit Future, we won't trigger the lint. If it is a variable assignment and the variable is unused, I'd expect the unused_local_variable to trigger. I'd expect that to be enough to tell the user this is still not handled. Do you agree? Should we do something (with this lint) in this case still?

[FMorschel]

One more issue that is a duplicate from the list above #59949.

[FMorschel]

To note:

• This being done this way (as described above) will have some conflicts with omit_local_variable_types and related lints when it asks for an explicit Future cast.

• One new issue came up recently ([linter][discarded_futures] false positive #59949) asking for special casing for variable assignments, saying any assignment should be considered handling. I'm unsure I agree or disagree.

I'd appreciate some inputs here @pq.

[pq]

My quick 2 cents:

This being done this way (as described above) will have some conflicts with omit_local_variable_types and related lints when it asks for an explicit Future cast.

My gut says if this route is considered valuable we could explore updating omit_local_variable_types to not flag these cases iff discard_futures is enabled. That's a little unfortunate but I think OK.

One new issue came up recently (#59949) asking for special casing for variable assignments, saying any assignment should be considered handling. I'm unsure I agree or disagree.

I'm not sure either. I'd be curious, in practice, if this happens a lot. The downside to a blanket special-case is that we may miss flagging unintended behavior. I wonder if a more principled approach might be something along the lines of function-side opt outs as discussed for unawaited_futures in #58348.

[jakemac53]

I also just ran into this not working as I expected... in particular in my case it interacts poorly with dart code metrics "avoid-redundant-async" lint, which will trigger if I add an async to the function since I am not awaiting the Future (but I am passing it to a function, expect from package:test).

I ended up having to ignore the lint on that line as I could not find any way to make both things happy.

[FMorschel]

In that case, since expect parameters are dynamic, I think the current approach I was taking is not valid to solve this. I wonder if any assignment to a function/method would be fine. Since most of the time users already write down the types they expect from the parameters. Do we have any numbers on this? On the ammout of times users don't place any expected type on parameters (not even adding dynamic).

I'm still unsure about any assignment at all because that could make users assign that to var _ and ignore this value. I'd want to be sure they know this is a Future by writing Future _ instead. Any takes here are appreciated.

[jakemac53]

Imo, if you use the future in any way, regardless of the type it is being assigned to, the lint should not trigger.

[FMorschel]

Basically the same as unawaited_futures but in non-async bodies then. Once I come back here I can make that change then if nobody else has any takes against it.

@pq, any takes on why this wasn't done from the start? Not sure if you remember it but maybe there was some reason?

[pq]

@FMorschel: sorry, I can't recall. Possibly just an incomplete thought.

[FMorschel]

I've refactored the CL for the lint itself. Could someone please take a look? Thanks a lot!

---

This CL is simply to review the current two fixes for discarded_futures. When adding async it will also add await and when wrapping with unawaited it will not be suggested for FutureOr.

https://dart-review.googlesource.com/c/sdk/+/416240

[FMorschel]

Three more things to consider here:

• I'd like to point out that after this CL lands, both unawaited_futures and discarded_futures work pretty much the same and it sounds like we want them to and to only differ when to trigger (asynchronous or synchronous code). We should probably take a look at making them share code. This would help with the following point and unawaited_futures false negative for FutureOr #59105.
• Currently, even with the above CL, discarded_futures doesn't trigger for Future sub-types
• At the tests for discarded_future we have both: DiscardedFuturesTest.test_newMethod_invocation and DiscardedFuturesTest.test_propertyAccess:
  About these, @lrhn seems to agree, considering his initial comments on the CL. For these points, I'd also like you to take into consideration unawaited_futures, if it should work the same or not.
  • At test_newMethod_invocation: @srawlins said:

    This seems like a very sane, standard way to not discard a Future: by providing a then-callback. (Indeed, this then-callback uses a wildcard, haha, but meh.)

    But the lint rule is firing, not on g(), but on .then, right? So I guess the rule is firing for the Future returned by then. :/

  • At test_propertyAccess @srawlins said:

    To me this is analogous to the string interpolation case: if you write "$someFuture", you probably meant to await it. If you write someFuture.runtimeType or .hashCode, you probably meant to await it. If you call someFuture.someExtensionGetter, then you obviously cannot await it, when someExtensionGetter is only available on Future.

[lrhn]

If you invoke a member on a Future which is defined by Future, not by Object, then you are "using" the future.
If that returns a new Future, then you need to use that too.
Maybe even a member declared originally by a subclass of Future, if the member is not overriding a member that was originally declared by a non-Future class. But most likely subclasses are not relevant, because you shouldn't be subclassing Future.

If you invoke an extension member on a Future and the extension has an uninstantiated on type which is (a subtype of) Future, then you're using the future too. The member of written knowing that it is acting on a Future.

If you're passing the future as an argument to a parameter which is typed as Future or Future Or, then you're using the future.
Same if you assign it to a local variable currently typed as Future or FutureOr, or return it from a function with a Future or FutureOr return type (even if it's tired like that because of promotion). Unused variable warnings should ensure that you're using the variable later, and then these lints will apply to that expression.
(May want to ensure that a Future<int?> is not assigned to FutureOr<Object>, since then it's really assigned to the non-future part of the FutureOr, and pregnant won't be awaited.)

(Not sure how to handle parameters with a generic type that is instantiated to be Furure. Probably have to accept those. Otherwise you can't store a Future in a list of futures, which you'll .wait in a moment.)

Wrapping a Future in an.extension type should be treated as the call to the constructor. If that constructor's parameter expects a future, it's fine. Otherwise it cannot be assumed to know that it's getting a future. If the extension type implements Future, then it counts as a new future, and all extension methods declared on it, or on super-extension types which also implement Future, count as using that future.

An interpolation like "...$someFuture..." should be treated like s call to .toString().

Awaiting is like an operand with a parameter type of FutureOr.

(We may want to handle variables specially, allowing you to do non-future things to a variable tired as a Future, af long as you also do at least one Future-specific thing on it. Like doing

if (cachedFuture != newFuture) { 
  newFuture.ignore(); 
  cachedFuture = cachedFuture.then((_) => newFuture);
}

Here the call to == (same if using identical) is calling a non-Future method on cachedFuture and passing newFuture as argument to a parameter types as Object. That's Ok because it's not the only use of either variable. They're also used to call Future members.)

Basically, we want to make sure that a Future is properly consumed. That means it's not assigned to a type which doesn't remember that it's a Future, or potentially a Future, and it's not (only) consumed by calling a member on it that isn't a Future-specific member.

[srawlins]

If you invoke a member on a Future which is defined by Future, not by Object, then you are "using" the future.
If that returns a new Future, then you need to use that too.

Ah ok, so it would be standard to write unawaited(someFuture.then(...));.

But most likely subclasses are not relevant, because you shouldn't be subclassing Future.

I only bring it up because I know of some (I believe popular) subclasses: package:async's DelegatingFuture (and ResultFuture?), Flutter's AsynchronousFuture, TickerFuture, some internal classes (DisposableFuture, CancelableFuture, ZonedFuture, InterruptableFuture, and more), gRPC's ResponseFuture, firebase_storage's Task. It just seems like the unawaited_futures and discarded_futures should behave the same for these: f(); when f returns a DelegatingFuture should definitely be flagged the same as if it were a Future.

[lrhn]

If a class implements Future, then it should be treated as a Future. My comment was not to say that you wouldn't be using those implantation class types as types, the f function you mention returning a DelegatingFuture should just be typed as returning Future, the caller doesn't need to know implementation details.
(But if the type leaks, it's still a future.)

If a class extends the interface of Future with more members, then I think it's making a mistake. If that flows into code that expects a future, it won't matter. If it flows through code that expects a future and out the other side, it's optimistic to assume that it will be the same object, so the extra functionality is easily lost. It's better to have something that contains a normal future and some other stuff, than something that is a future and some other stuff at the same time.

But if it implements Future we should treat it as one, and then any of those other members which are only declared on classes that implement future, should count as consuming the future.

[FMorschel]

Do we have any other lints today that share part of their code? In any way similar to what we want here, I mean.

---

Bots broke after the landing of the two CLs at the same time so I opened https://dart-review.googlesource.com/c/sdk/+/417900

[bwilkerson]

I haven't read this thread recently, but I'm guessing that you're asking about the question of whether a class is a "Future"-enough class, in which case the answer is "yes". We have lints that use the same concept and therefore share an implementation of that concept. For example, the recently added lints around "obvious" type annotations all share a common implementation of what it means for a type annotation to be "obvious".

[FMorschel]

Thanks! I'll take a look. I'll see if I can make both unawaited_futures and discarded_futures use the same base code and then we can only differ at the sync/async code condition.

After that, we can implement the above discussion about another meaning of "using" the Future. Calling non-Future members would not be considered "using" (e.g. .toString()).