dart-lang/linter

discarded_futures - incorrectly reporting discarded future

bsutton opened this issue · 4 comments

I have the class FutureBuilderEx which takes a future as an argument:

 FutureBuilderEx(
                  future: DatabaseHelper().getVersion(),                   // triggers : discarded_futures
                  builder: (context, version) =>
                      Text('Database Version: $version')),

FutureBuilderEx takes a future and in the above example getVersion returns a future.

The problem is that the 'discared_futures' lint reports this as a discarded future when it isn't discarded as it is passed to a method that expects a future.

The above scenario should not trigger the lint.

lrhn commented

If you check the lint description, the rule that it states is:

DON'T invoke asynchronous functions in non-async blocks.

You're invoking an asynchronous function in a non-async block, so the lint triggers.
The solution is to make the surrounding function async.

The lint is not the analogue of unawaited_futures for non-async functions.
The lint rule is not saying that you should use the future, aka. not discard it, but that you shouldn't create one to begin with. A future being "discarded" is what you risk by not following the lint, not that it necessarily happens.
So, yes, the lint is also not particularly well named. 😉

So the problem is that this lint is generating false positives.

The full example would be:

Widget build(BuildContext context) {
 return FutureBuilderEx(
                  future: DatabaseHelper().getVersion(),                   // triggers : discarded_futures
                  builder: (context, version) =>
                      Text('Database Version: $version')),

So this is a legitimate use of a future in an sync method and of course being the build method you can't make it async.

I don't think this lint should be triggered if the future is being appropriately consumed in a non-ambiguous manner.

So :

var version = DatabaseHelper().getVersion();

This is ambiguous as you can't tell if the user intended to return a future - lint should trigger.

Future<int> version = DatabaseHelper().getVersion();

This is non-ambiguous - though might be a problem when combined with the lint that tells you to not use types on local variables as tools look vs-code when configured to fix on save will remove the type.

My original example using FutureBuiderEx is non-ambiguous so should not trigger.

The aim of this lint should be to ensure that user's are not creating a future that will not be consumed rather than just naively complaining if a future is created in a sync method.

lrhn commented

this lint is generating false positives.

Not according to the lint specification (which is what I consider the documentation to be, since there is nothing else to go by). This is clearly a call to an asynhronous function inside a non-async function, so a true positive for what the lint warns against.

I don't think the specified behavior is a particularly useful, and wouldn't want to enable the lint myself, but the lint is consistently doing what it says it does. (Well, almost, since using unawaited can repuitedly silence the lint in some cases.)

The aim of this lint should be to ensure that user's are not creating a future that will not be consumed rather than just naively complaining if a future is created in a sync method.

So what you want is a different lint, or a different definition for this lint (aka. a different lint with the same name).

I completely agree. The discarded_futures lint has been nothing but trouble because the behavior is too strict to be practical, it lacks the intelligence of unawaited_futures to recognize reasonable uses, and the name is confusing for what it actually does. See also discussions in #3429, #3651, #4375, #4866, #4773, #5053, and probably more.

Your possible workarounds are:

  • Disable the lint.
  • Ignore the lint locally:
    // ignore: discarded_future
  • or ugly hacks to do async operations inside an async function, that doesn't look async, and exfiltrating the result through side effects. (Or through dynamic calls, but even I won't suggest that for production code.)
    Future<Version>? databaseVersion;
    void setDatabaseVersion() async {
      databaseVersion = DatabaseHelper.getVersion();
    }
    setDatabaseVersion();
    assert(databaseVersion != null);
    (which is not calling an asynchronous function, it's calling a void function! Clearly not the same!)
    Makes // ignore: ... look good!

Closing as WAI. Thanks @lrhn