Meta issue for returns in functions with void related return types

[leafpetersen]

This is a meta-issue for tracking errors associated with returns in block bodied sync and async functions whose return type is void, Future<void>, or FutureOr<void>.

Tests are here: https://dart-review.googlesource.com/c/sdk/+/52742

We say that a type T contains void if T is void, or if T is FutureOr<S> and S is void.

My understanding of the intended rules are as follows:

• In an asynchronous function with return type T:
  • return; is an error unless flatten(T) is void
  • return exp; is an error if exp has static type S and flatten(S) is not assignable to flatten(T)
  • return exp; is an error if exp has static type S and flatten(T) is void and flatten(S) is not void
• In a synchronous function with return type T
  • return; is an error unless T contains void
  • return exp; is an error if exp has static type S and S is not assignable to T
  • return exp; is an error if exp has static type S and T is void and S is not void
  • return exp; is an error if exp has static type S and T is FutureOr<void> and flatten(S) is not void

cc @lrhn @eernstg @munificent

@lrhn I'm not sure which side of the argument around return 3 in a void function you came down on. The last rule from the async clause and the last two rules from the sync clause are intended to address those cases. Let me know what you think.

[lrhn]

I agree with almost everything here, except it's not covering all the FutureOr<void> cases.
And I still want to ensure that return; is equivalent to return null as void; in every way.

There are three different return type cases for synchronous functions:

• is void
• contains void (is FutureOrⁿ<void>)
• does not contain void.

The "is void" case only allows returning void. That includes return;, return voidExp; and fall-through at end of function.

The "contains void" cast allows returning void, but also allows returning anything up to Futureⁿ<void>, but no further.
The handling of "is void" should be consistent with the "contains void" where /n/=0.

The "does not contain void" case does not allow returning anything that contains void.

In other words, define (probably with a less generic name):

  matches(T, S) ::=
      true if `T` is `void` and `S` is `void`
      true if `T` is `FutureOr<X>` and matches(X, flatten(S))
      false otherwise

and then:

• In a synchronous function with return type T
  • return; is an error unless T contains void
  • return exp; is an error if exp has static type S and S is not assignable to T
  • return exp; is an error if exp has static type S, T contains void, and not matches(T, S).

Since void is not assignable to anything but types containing void, the second item covers S being void and T not containing void.
The first item is compatible with return null as void since matches(T, void) would always be true when T contains void.

For asynchronous functions, we we really just flatten both types and do the same thing:

• In an asynchronous function with return type T:
  • return; is an error unless flatten(T) contains void
  • return exp; is an error if exp has static type S and flatten(S) is not assignable to flatten(T)
  • return exp; is an error if exp has static type S, flatten(T) contains void, and not matches(flatten(T), flatten(S)).

I haven't checked if there is any +/-1 errors in my counting here, but I think it should work, and it looks remarkably symmetric (which I take as a good sign).

Synchronous examples allowed by this:

void foo() { return; }
void foo() { return null as void; }
void foo() { }
FutureOr<void> foo() { return; }
FutureOr<void> foo() { return null as void; }
FutureOr<void> foo() { return Future<void>.value(); }
FutureOr<void> foo() { return null as FutureOr<void>; }
FutureOr<FutureOr<void>> foo() { return; }
FutureOr<FutureOr<void>> foo() { return Future<void>.value(); }
FutureOr<FutureOr<void>> foo() { return Future<Future<void>>.value(); }
Object foo() { return 42; }

Disallowed examples:

void foo() { return 42; }
void foo() { return null as FutureOr<void>; }
FutureOr<void> foo() { return 42; }
FutureOr<void> foo() { return Future<int>.value(); }
FutureOr<void> foo() { return Future<Future<void>>.value(); }
FutureOr<FutureOr<void>> foo() { return Future<Future<Future<void>>>.value(); }
Object foo() { return; }
Object foo() { return null as void; }

Asynchronous examples allowed:

void foo() async { return; }
void foo() async { return null as void; }
void foo() async { return Future<void>.value(); }
Future<void> foo() async { return; }
Future<void> foo() async { return null as void; }
Future<void> foo() async { return Future<void>.value(); }
Future<void> foo() async { return Future<void>.value() as FutureOr<void>; }
Future<FutureOr<void>> foo() async { return; }
Future<FutureOr<void>> foo() async { return null as void; }
Future<FutureOr<void>> foo() async { return Future<void>.value(); }
Future<FutureOr<void>> foo() async { return Future<FutureOr<void>>.value(); }
Future<FutureOr<void>> foo() async { return Future<Future<void>>.value(); }
Future<FutureOr<void>> foo() async { return Future<Future<void>>.value() as FutureOr<Future<void>>; }
Object foo() async { return 42; }

Disallowed examples:

void foo() async { return 42; }
void foo() async { return Future<int>.value(); }
Future<void> foo() async { return 42; }
Future<void> foo() async { return Future<Future<void>>.value(); }
Future<FutureOr<void>> foo()  async { return 42; }
Future<FutureOr<void>> foo()  async { return Future<Future<int>>.value(); }
Future<FutureOr<void>> foo()  async { return Future<Future<Future<void>>>.value(); }
Object foo() async { return; }

[leafpetersen]

The "is void" case only allows returning void. That includes return;, return voidExp; and fall-through at end of function.

I'm surprised that you don't want to allow returning FutureOr<void> from a void function since the analogy to regular assignability suggest it would (it "could work"), but it's fine with me.

The "contains void" cast allows returning void, but also allows returning anything up to Futureⁿ<void>, but no further.
The handling of "is void" should be consistent with the "contains void" where /n/=0.

Note that my definition of contains was not recursive as you seem to be assuming here. This was a deliberate choice to match the non-recursive nature of "flatten". I'm fine with making it recursive though. I think contains and matches might be better structured as the following then.

We say that T contains S iff one of the following is true:

• T is S
• T is FutureOr<X> and X contains flatten(S)

We say that T void contains S iff T contains void and T contains S.

Now we have the following rules:

• In a synchronous function with return type T

  • return; is an error unless T contains void
  • return exp; is an error if exp has static type S and S is not assignable to T
  • return exp; is an error if exp has static type S, T does not void contain S

• In an asynchronous function with return type T:

  • return; is an error unless flatten(T) contains void
  • return exp; is an error if exp has static type S and flatten(S) is not assignable to flatten(T)
  • return exp; is an error if exp has static type S and flatten(T) does not void contain flatten(S)

WDYT?

[lrhn]

I'd still prefer not to return anything from a void function, but we have to accept existing code which returns void expressions. Generalizing that, I want it to be allowed to return any type which is part of (when unfolding the union type) the return type (but not a subtype if it's only because of a subtype relation to void).

So a return type of FutureOr<FutureOr<void>> should allow any of:

• void
• Future<void>
• FutureOr<void>
• Future<Future<void>>
• Future<FutureOr<void>>
• FutureOr<Future<void>>
• FutureOr<FutureOr<void>>

and nothing else.

That exactly matches your contains relation, it's exactly {S | FutureOr<FutureOr<void>> contains S}, so I think it is a good combination of a recursive "contains" and "matches".

That allows you to return the result of a function with the same return type (or any part of a union-return type) directly, even if it contains void, but nothing else.

I think the void contains rule must be written as:

T void contains S if T contains void implies T contains S.

Otherwise T = FutureOr<int> and S = int would be an error because T does not void-contain S (because it doesn't contain void).
If we do that, then I think it does indeed work exactly like what I specified. I'm not sure void contains carries its own weight, though, so maybe just say:

• return exp; is an error if exp has static type S, and T contains void and not T contains S.

(which is more readable than a negated implication, but then, almost anything is)

[leafpetersen]

SGTM.

[lrhn]

So, summary:

Define: T contains S:

• true if T is S,
• true if T is FutureOr<X> and X contains flatten(S), and
• false otherwise.

(Effectively this means that the unfolding of the FutureOr union types of T contains exactly S).

The rules for a synchronous non-generator function with declared return type T are:

• return; is an error unless T contains void.
• return exp; is an error if exp has static type S and S is not assignable to T.
• return exp; is an error if exp has static type S, T contains void and not T contains S.

The rules for an asynchronous non-generator function with declared return type T are:

• return; is an error unless flatten(T) contains void.
• return exp; is an error if exp has static type S and flatten(S) is not assignable to flatten(T).
• return exp; is an error if exp has static type S, flatten(T) contains void and not flatten(T) contains flatten(S).

[leafpetersen]

This LGTM, except that we need to say:

• return; is an error unless T contains void or T is dynamic.

and similarly for the async case.

[leafpetersen]

I've fleshed this out with some more details here: https://dart-review.googlesource.com/c/sdk/+/52742 .

I'm running presubmits now. I see some new errors that are good, but there's one class that I have a bit of a hard time justifying. Specifically, consider:

Future<int> firstThing() {...}
Future<int> nextThing() {...}
Future<void> waitForSomethings() async {
    var a = await firstThing();
    // Do stuff
    return nextThing();
}

This is now an error. Looking at the examples of this that I see in code, it doesn't look to me like bad code. Futures encapsulate two things: control, and data. It doesn't make much sense to say void a = 3, but it seems much more reasonable to say Future<void> a = returnsFutureBool(). The former is pointless, but the latter preserves the control aspect of the Future while discarding the data.

Do we really care to make this an error?

[munificent]

it seems much more reasonable to say Future<void> a = returnsFutureBool().

Yeah, that seems reasonable to me for exactly the reason you state. "I don't care what this returns, but I do care when it returns."

If you go on to await or call .then() on that future and attempt to use the result, you'll get an error at that point. So I don't think we lose any real safety by not showing the error earlier when assigning the Future<...> to Future<void>.

[eernstg]

If we're considering return statements in async functions to be concerned with completion of the returned future then we're out of "returned value" land and back in "calling a method" land:

Future<void> f() {
  // The following really means `myCompleter.complete(3)`, passing `3` 
  // to a parameter of type `void`.
  return 3; // OK because we always allow passing arguments to such a parameter.

  // The following really means 
  // `let v = await Future<int>.value(3) in myCompleter.complete(v)`
  // so we are again just passing an argument (of type `int`) to a parameter
  // of type `void`.
  return Future<int>.value(3); // OK, same as above
}

Using that perspective we would never complain about values being discarded, presumably unintentionally, because they are "returned to void". That's simply not an issue for return in an async function.

Another thing is that we could have void f() async {...} where the returned future is marked as "should be discarded", but we have explicitly chosen to consider that as an acceptable setup (it's a "fire-and-forget" operation). So that's the real violation that we could have with an async function, but we have settled that as ok already.

But I think it would be a really confusing situation if we start mixing the two perspectives (1: return e makes e a returned expression also in an async function, or 2: return e in an async function is concerned with a method call passing e or await e as an actual argument).

[lrhn]

The specification of async functions does not mention completers, and I'd prefer to look at async functions as if they return a value of the correct flattened type, which is then used to complete the actually returned Future ... somehow.
That is the body of Future<int> foo() async { should be treated (mostly) like a function returning int. Allowing Future<int> in the return expression is just backwards compatibility (I'd not add it today if we didn't already have it), but it just works as if each return e; has an implicit await inserted.

So it's 3: return e in an async function is actually return await e;. It is a return at the base type, not the future type.

[eernstg]

If we insist that return e in an async function should be as similar as possible to a return in a regular (sync) function then we should rely on the actual type argument T to Future in the return type when we decide how to handle it.

That type argument may be immediate, or it may be more or less hidden. For a declared return type of Future<T> it is T. When the return type is dynamic (e.g., omitted) it is less obvious, but we could say that dynamic is morally related to Future<dynamic>, and hence T is dynamic, by convention. For other cases like FutureOr<FutureOr<void>> we need some algorithm which will always produce a "returned type" T, and it might be reasonable to say that it's the most general T such that Future<T> is a subtype of the actual declared return type, with some suitable special casing for top types (maybe we just take the simple type "in the middle" such that FutureOr^n<Object> becomes Object, etc).

Let's say that we have determined the "async return type" T from some declared return type R. Then we can check each return statement statically and decide whether it is type correct: The static type of the returned value should be (1) assignable to T, or (2) assignable to Future<T>. At run time we may await the given future if it turns out to have a type which at Future is Future<S> such that S is assignable to T (so we could have a return type of Object, and it turns out to be a Future<num>, and the declared return type is Future<int>, so we await the Future<num> get lucky that it's completed with an int, and then use that int to complete the returned future).

So could we end up in a bad state with this, e.g., by awaiting a future because of this assignability property, and then fail to complete the returned future because we chose to wait, but it would actually have worked if we had just used the future directly? The run-time type S of the object to return would then have to be Future<U> for some U (or a clausal subtype thereof, say, MyFuture<U>) such that U is assignable to T (because otherwise we wouldn't have awaited it), and we would also need to have S <: T. That could certainly occur:

class MyFuture<U> extends Future<U> implements C { ... }

Future<C> f() async {
  return new MyFuture<Object>(); // Ignore the static type, this is about run time.
}

We could actually complete the returned future with the given MyFuture<Object> because it implements C, but we can also see that it has Future<Object> as a clausal supertype, so we could also try to await it and get that Object, which may or may not be a C. So we do get a future, but it might be wrong to await it.

Of course, we might also get a MyFuture<C> such that it would be guaranteed to deliver a C (or an exception), and then it might work no matter which path we take, but the semantics shouldn't be ambiguous and we don't really know which way is the one that the developer intended.

[eernstg]

I've created this CL, which is aimed at handling funny combinations like "return type Future<int>, got a Future<Object>" and MyFuture<C> above) in a meaningful manner.

[eernstg]

I abandoned said CL: We will suspend that topic for now, leaving implementations as well as specification unchanged.

[leafpetersen]

Agreed on spec has landed.