proposal: avoid_returning_futureor_from_async_functions - Avoid returning FutureOr from async functions

[DanTup]

avoid_returning_futureor_from_async_functions

Description

Avoid returning FutureOr<T> from async functions.

Details

Returning FutureOr<T> from async methods may result in Futures that cannot be awaited for some Ts.

Kind

Does this enforce style advice? Guard against errors? Other?

Bad Examples

FutureOr<T> doWork(FutureOr<T> Function() operation) async {
  return await operation();
}

Good Examples

Future<T> doWork(FutureOr<T> Function() operation) async {
  return await operation();
}

Discussion

I filed #52621 after catching a bug in my code where a Future I expected to be awaited was not.

A contrived example looks something like this. There are no errors in this code and it appeared as if everything was correctly awaited but it prints FINISHED before DOING WORK.

import 'dart:async';

import 'package:test/test.dart';

Future<void> main() async {
  var runner = WorkRunner(() async {
    await pumpEventQueue(times: 5000);
    print('DOING WORK');
  });
  await runner.run();
  print('FINISHED');
}

class WorkRunner {
  final Future<void> Function() work;

  WorkRunner(this.work);

  Future<void> run() => run2(work);

  FutureOr<T> run2<T>(
    FutureOr<T> Function() operation,
  ) async {
    return await operation();
  }
}

My understanding (from discussion in the linked issue) is that in FutureOr<T> run2<T>() async the T is Future<void> and the returned value is FutureOr<Future<void>>. The await runner.run() is only awaiting the outer Future (created by run2 being async) and it doesn't actually await the work.

Using Future<T> instead of FutureOr<T> in the return types avoids the issue (and in several other issues I've found this advice generally being given).

I don't know how useful this would be to others. I think part of my issue was that I thought await would always unroll all futures (so the result of awaiting something would never be a Future), but it turns out that was incorrect. It's possible this is a Danny problem but I though it was worth filing for discussion.

[eernstg]

Makes sense!

Perhaps the lint could be even a bit stronger: Flag every function with an async body whose return type isn't a subtype of Future<Object?> and isn't void, and flag every function with an async* body whose return type isn't a subtype of Stream<Object?>.

(Aside: The case void f() async {...} is handled by avoid_void_async, so we shouldn't interfere with that particular signature here. Developers who do not want to use this signature can enable avoid_void_async, and developers who do want to use it can disable the lint. There are legitimate usages.)

The point is that those functions will return a future (async) or a Stream (async*), and any return type which is more general (like Future<T>?, FutureOr<T>, FutureOr<T>?, for any T, or Object, etc.) is just noise.

It is possible that such a return type is required with an instance member of a class or mixin because of the signatures of members that it is overriding and/or is overridden by, but this is probably rare enough to justify the need for an // ignore comment.

[lrhn]

The fact that it can also interact badly with type inference is not the primary reason to not return a FutureOr.

My recommendation (which has staid constant since the introduction of FutureOr) is to never return a FutureOr from any function, async or not, because it forces the caller to be ready for a future, and also, maybe, do extra work if it's not a future. It's better to just always return a future. The optimal behavior for the caller is to just blindly await the returned value, which may be less efficient if the returned value is not a future (because the specified behavior is to then allocate a future and wait for it, although a good runtime system should be able to no introduce any extra future).

I know someone might want an API that can get a value synchronously or asynchronously, to save time on awaiting a future. If always awaiting the future is a pain-point for your API, you may want to have a more specialized API, like

  bool get hasValue; // Whether the [value] getter works.
  V get value; // Throws if no value.
  Future<V> waitForValue(); // Waits until [hasValue], then provides it.

instead of forcing the user to do is-tests to figure out which case it is.

For async functions, what @eernstg says also applies.

The type problem here can't only happen for FutureOr, if you call a function like;

Future<T> churn<T>(FutureOr<T> value) async => await value;

with a Future<int> argument and somehow manage to get T bound to Future<int> instead of int,
you'll still get a Future<Future<int>> back, and the inner future won't be awaited.

The issue with FutureOr here is just that type inference can more easily make T be Future<int> for you.

The real issue, the one I think we should warn about, is that the return type ends up being Future<Future<int>> at all.
Any time you have a Future<Future<X>> and maybe even FutureOr<Future<X>>, Future<FutureOr<X>> or FutureOr<FutureOr<X>> — all union types where one of the types is a Future<Future<X>> — anywhere in your program, you should get a warning. It should never happen.

If you really, really have a good reason for a Future<Future<X>> ... please let me review your code 😉 . If you want to count ticks towards a result, use a stream.

[DanTup]

The real issue, the one I think we should warn about, is that the return type ends up being Future<Future<int>> at all.

If that would catch this case and more, should we change this request to be just that then? avoid_nested_futures (where futures means both Future and FutureOr)?

[eernstg]

I think the occurrence of nested future types (anywhere, not just as return types) is a completely different topic than using a return type on an async or async* function which is overly general. So avoid_nested_futures and avoid_too_general_async_return_type would presumably both be relevant.