Possible false positive in discarded_futures lint

[bartekpacia]

Describe the issue

I believe that the discarded_futures lint is too aggressive. Read on.

To Reproduce

Let's see the following simple Flutter code:

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(primarySwatch: Colors.blue),
      home: const MyWidget(),
    );
  }
}

class MyWidget extends StatelessWidget {
  const MyWidget({super.key});

  Future<String> createFuture(String someParam) async {
    await Future<void>.delayed(const Duration(seconds: 1));
    return Future.value('dash');
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: FutureBuilder<String>(
        future: createFuture('hi'),
        builder: (context, snapshot) {
          return const Text('just a placeholder');
        },
      ),
    );
  }
}

And let's enable discarded_futures lint in analysis_options.yaml:

include: package:flutter_lints/flutter.yaml

linter:
  rules:
    discarded_futures: true

And let's run dart analyze:

$ dart analyze
Analyzing discarded_futures_lint_bug... 1.0s

   info • lib/main.dart:35:17 • Don't invoke asynchronous functions in non-async blocks. •
          discarded_futures

1 issue found.

That's how it looks like in my VSCode:

Expected behavior

I'd expect no warnings. FutureBuilder.future expects a future, I'm giving it the future, we're happy, but there's discarded_futures yelling at us.

Additional context

$ flutter --version
Flutter 3.3.2 • channel stable • https://github.com/flutter/flutter.git
Framework • revision e3c29ec00c (3 weeks ago) • 2022-09-14 08:46:55 -0500
Engine • revision a4ff2c53d8
Tools • Dart 2.18.1 • DevTools 2.15.0

Also:

This might be another issue, but I often see code like this:

BlocProvider<AuthCubit>(
  lazy: false,
  create: (_) => AuthCubit(
    loginClient: context.read(),
    forIntegrationtest: _forIntegrationTest,
  )..init(), // init is async
),

The discarded_futures lint also warns about that ..init(), but we never care about it. Implementing the lint that is talked about in this issue would solve this problem.

[bwilkerson]

It isn't entirely clear to me what discarded_futures is attempting to catch. The name of the lint implies that it's looking for places where a future is ignored, but the message implies that it's purely about ensuring that uses of futures are in async functions so that unawaited_futures can catch problems with it.

It's certainly the case that the example code is not discarding a future; it ought to be fine to pass the future as an argument to another function. So if that's the purpose of the lint, then this is clearly a bug.

But if the purpose is to ensure that future-returning functions are only invoked in async functions, then the example code is clearly in violation.

If anyone has context on the purpose of the lint, I'd appreciate hearing more about it.

[srawlins]

CC @pq who implemented discared_futures in #3431

[jonmountjoy]

I've just added this lint to a fairly large code-base, and the only false-positive (after adding many unawaited) was the one highlighted in this issue.

The entire purpose of the FutureBuilder is to invoke an asynchronous function in a non-async block, so the warning of "Don't invoke asynchronous functions in non-async blocks" for this particular context of this lint, feels wrong.

[eernstg]

As far as I can see, discarded_futures does very nearly what the error message says, but it doesn't make any attempt to detect whether or not any futures are discarded. Here's an example error message:

   info • n009.dart:11:3 • Asynchronous function invoked in a non-'async'
          function. Try converting the enclosing function to be 'async' and
          then 'await' the future. • discarded_futures

It lints situations where a function returning a Future<...> is called in the body of a function whose return type isn't Future<...>. It doesn't care whether the caller or callee has an async body or not, but it is true that every function whose body is async must return a future.

Object f() async => 1; // Returns a `Future`, but the return type doesn't show it.

void g() {
  f(); // No lint.
}

This means that the lint allows functions returning a future to be synchronous and still call some async bodied functions:

Future<int> f() async => 1;

// Returns a future.
Future<int> g() {
  f(); // Calling an `async` function and discarding a future. No lint.
  return new Future<int>.value(0);
}

// Returns a non-future.
void h() {
  f(); // Lint!
}

I think the lint is working in a manner that makes sense: It is basically nudging developers in the direction of keeping all asynchronous computations in async function bodies. It is a simple test, and it yields advice which is probably a good habit in general.

It also makes sense that it isn't a core lint: There are lots of situations where it does make sense that a synchronous function body calls a function that returns a future and then, say, stores that future or passes it as an actual argument to some other function. In other words, false positives are no surprise, and special application domains or coding styles may generate many false positives.

However, it should perhaps be renamed to something like asynchronous_call_in_synchronous_function, in order to avoid the misleading reference to 'discarded'?

Or perhaps the lint should try to detect whether or not a future is actually discarded? Do the analyzer and/or the linter have the appropriate APIs to detect any such situations? It's probably a lot of work to write it from scratch...

[westito]

Renaming won't match the actual operation. When you return the future as parameter it is actually not called! So asynchronous_call_in_synchronous_function nether a good name. discarded_futures simply not working properly. Should not lint in this case. However, when it is encapsulated in try-catch it should lint because if the function not awaited, the try-catch will not catch. There is lint in DCM for this: https://dcm.dev/docs/rules/common/prefer-return-await. It should be checked in discarded_futures too.

[andy-clapham]

I'm struggling with using this lint too - in theory it's so important it should be core. But in practice it doesn't seem to be handling the right thing.

Flutter's FutureBuilder is a classic case - the Future is simply not being discarded, it's being handled very nicely.

Synchronously taking a reference to a future in any code in a variable (sync or async) shoudn't be linted. Where the bugs occur is if the future is no longer referenced before it is considered completed via an await, precompleted, sync'd, or another mechanism like unawaited.

e.g. this is fine

Future<void> doAsyncFuture() async {
  await Future<void>.delayed(const Duration(seconds: 1));
}

Future<void> nonAsyncFuture() {
  return Future.value(null);
}

void main() {
  final myFuture = doAsyncFuture();
  unawaited(myFuture);
  final useThisLater = nonAsyncFuture(); 
}

Whereas these are potential bugs, yet hugely common especially in Flutter callback handlers.

  Future<void> waitFor1sMaybe() async {
    doAsyncFuture();
  }

  final widget = Thing(
    onPressed: () async {
      doAsyncFuture();
      Navigator.pop(context);
    },
  );

Discarding incomplete futures as they go out of scope is where the bugs happen. It can be considered separate from anything to do with async/await, except as await being one mechanism for ensuring a future is complete.

With deeper analysis, method-side opt out shouldn't be needed in the majority of cases, as the linter should see the future is being passed elsewhere or returned and not being discarded. But I suppose it's one way of systemizing the non-core unawaited library call into the linter without taking a dependency. Probably more relevant for dart-lang/sdk#46218