Analysis of void return type with arrow causes bad API restrictions

[nex3]

Right now, void foo() => bar() is allowed if bar() returns void or dynamic, and produces an error if it has any other return type. This makes sense on the surface, but it has pretty negative consequences. In particular, it means that it's not safe for an API to add a return type where none previously existed. This is a common refactor that would otherwise be totally backwards-compatible.

I can see three solutions to this problem:

1. Eliminate the error if bar() has a return type. If it's valuable information, it could be downgraded to a warning or a hint.
2. Add a warning or error when using => in a void context, so that the void foo() => bar() pattern doesn't occur.
3. Keep the behavior as-is and forbid APIs from adding return types without major version increments.

I strongly prefer option 1, but I'd still choose option 2 over option 3. Under option 3 (the current state of the world), it's likely that API designers won't consider this edge case and will continue to add return types to their void methods, thus potentially breaking downstream users.

[natebosch]

1. Would we limit it to the narrow case of forwarding to a single method call? If we make this a warning we'd still have to fix a lot of code since we don't allow warnings most places - I think if we can make it a hint, or allowed with no hint, it'd be easiest to roll through.
2. We would have a lot of code to clean up.

My vote would be to allow void foo() => nonVoid() but I can see arguments for this being a bad idea.

[nex3]

There's also a runtime component to this. The following code:

void foo() => bar();
dynamic bar() => 1;

void main() {
  foo();
}

throws

Unhandled exception:
type 'int' is not a subtype of type 'void' of 'function result' where
  int is from dart:core

in checked mode. So in order to make option 1 work, we may also need some sort or rule for the checked-mode VM that it nulls out a value returned through void.

[nex3]

@natebosch I also think a hint or nothing is a better option than a warning. void foo() => nonVoid() is totally reasonable in many cases where you're calling nonVoid() for its side-effects, and generally I think users think of => for void functions as shorthand for { } and are surprised when that's not the case.

[natebosch]

dart2js, dartium, and vm are likely all impacted in checked mode. DDC might just work as long as the analyzer is OK with it

[jmesserly]

(fyi -- I don't think it makes sense to tag other impls until we know what the language team perspective is on changes w.r.t. checked mode. There's already a process in place to go from language team issue -> farm out to implementations.)

[jmesserly]

Some thoughts regarding the two options:

Eliminate the error if bar() has a return type. If it's valuable information, it could be downgraded to a warning or a hint.

@leafpetersen -- language team has discussed "void" treated as "Object" in runtime checks, right? Is there an issue for it? DDC has already implemented it.

Once that is available, we could eliminate the error as this suggests. (Currently the error protects against the runtime failure, if I'm understanding correctly.)

Add a warning or error when using => in a void context, so that the void foo() => bar() pattern doesn't occur.

Without the above checked mode change, this would be the right approach. My guess, we didn't think through the API evolution implications when void foo() => barThatReturnsVoid() shorthand was allowed.

[jmesserly]

FYI -- these analyzer changes are quite simple either way. I'm happy to throw together a quick patch once we decide how "void" runtime subtype check is going to work.

[leafpetersen]

@lrhn is looking at various aspects of our treatment of void from the language perspective. Lasse, will your proposal cover the issues discussed here?

[lrhn]

The current design will allow anything to be assigned to void, so it's option 1 - void foo() =>bar(); is allowed no matter what the static type of bar() is and won't complain a runtime either, not even in checked mode.
That's consistent with allowing a non-void function to override a void function, and we should just do that in any case, even if we wouldn't make void useful in other locations too.

[jmesserly]

Ah that sounds great! Is there an existing issue to get void changed on VM/dart2js in checked mode? If not, can we file them?

edit: an issue like this one, for moving Null to bottom? #28024

[jmesserly]

Analyzer side: https://codereview.chromium.org/2699053002

[matanlurey]

I think this is stale/resolved.

[eernstg]

It's resolved: #28939 allows void arrow functions to have a returned expression (its "body") of any type, void and non-void.