Missing error when awaiting a void expression

[leafpetersen]

The snippet below should be an error per the static analysis section of https://github.com/dart-lang/sdk/blob/master/docs/language/informal/generalized-void.md .

import 'dart:async';

void returnsVoid() {}

main() async {
  await returnsVoid();
}

[natebosch]

Should we also have an issue filed for the CFE to reject this case?

[leafpetersen]

#33416

[MichaelRFairhurst]

This is a larger breaking change than I first thought. There were 6 files violating this in the SDK, and it may be common in g3 as well.

I will try to get a measure of the impact this would have on flutter, and on g3. May be worth another "breaking change RFC" in the flutter mailing lists etc.

[eernstg]

Here's a naive drive-by comment: Surely there are some beautiful fixes to each instance of this problem, involving various derived adjustments of declared return types and whatnot; but it should also be enough to replace await e by await (e as Object) in all the cases where we have await e for an e whose static type is void. That may not be so pretty, but it would make the changes very local. Would it be helpful to use this approach? Maybe you're already planning to do something similar?

[vsmenon]

@MichaelRFairhurst - are you the owner on this? Any updates?

[MichaelRFairhurst]

We've got ~500 failing targets in g3 for this. I'll go with @eernstg's suggestion of adding 'as dynamic' to get past these.

Of course even with that, that can take a while to roll out externally etc.

[mit-mit]

@MichaelRFairhurst got an update on this?

[MichaelRFairhurst]

Migration is taking a while here. Lots of breakages...about 2.5k in google3 so far. cl/202982389.

@leafpetersen @eernstg @lrhn that's probably at the scale that you guys should be aware...

[leafpetersen]

We may want to abandon this as a Dart 2 error and make it an opt in lint.

[natebosch]

Would we plan on making this an error in Dart 3?

[leafpetersen]

I have no idea about Dart 3. My hope would be that if

• we made this (and other related things) a lint in a standard set of lints
• the ecosystem mostly migrated to that standard set of lints

then we could fairly easily make this an actual part of the language in Dart 3. Bunch of ifs there though.

[lrhn]

The current void story is not very consistent, mainly due to all the legacy exceptions we've had to add.

I hope the await voidExpression cases are errors. It scares me to think that someone is awaiting a void expression because they know that a subclass might return a future. That would make my hope of actually making void expressions not evaluate to a value more breaking.

So, to make future improvements more viable, I'd prefer to keep this restriction if at all possible.

Casting a void expression to dynamic is still trying to use the value, and I really, really want to avoid that.
I'd prefer to fix it by just removing the await. That should not fail. If (when) it does, changing to expression; await Future.microtask((){}); is a better solution than awaiting the (completely unpredictable) value.

[eernstg]

It's a little bit like void foo() { ... return print('Hello!'); ... } in that it combines the control flow and effect in one statement, but we really just want the control flow, and we want the effect, and the fact that it looks like we are returning a value is pure obfuscation.

With await voidExpression we may want the delay, and we surely want the side-effects from evaluation of voidExpression, but it's pure obfuscation to pretend that the two are connected via a future, and if there is indeed a future there at run time it will probably be a big surprise (relative to a reasonable description of the intention).

We would get rid of that problem if we were to replace the result of a void typed expression by null in some well-defined set of situations.

[leafpetersen]

This didn't make the cut for Dart 2, closing as not planned. We may want to open an issue to make a lint for this.

[peter-ahe-google]

#30470 is still in the cut and I have implemented the error.

[natebosch]

This error started showing up in 2.1.0-dev.0.0 and broke code that was working and indicated correct in 2.0.0 - I don't think we can safely ship this until 3.0.0 unless it's hidden behind some kind of --strict flag. In 2.0.0 there is no error or warning in analyzer or VM. In 2.1.0-dev.0.0 there is an error in analyzer and a warning in the VM. (I don't think we should be printing warnings at all when running with the VM - filed #34137 to track that).

The revert at 8af2d70 is in the history for the tag for 2.1.0-dev.0.0 so I guess some other commit has reapplied it or a similar change, and separately something has introduced it as a warning in CFE.

I think it would be safe to:

• Make it a warning in analyzer instead of an error.
• Leave it as a warning in CFE as long as we aren't printing warnings in the Dart VM.

[MichaelRFairhurst]

Yeah this is definitely weird, I saw this today but hadn't had time to investigate.

I definitely did not reland this change. I was not able to get google3 fully clean. Keerti told me recently she saw like, one violation of this, which is weird that both it showed up and that it was missing.

The latest plan was to have it be caught by await_only_futures.

That's all from the analyzer side. From the CFE side, I forgot to coordinate with this. Just in general, @peter-ahe-google expressed that it was hard to follow what he was supposed to do over this topic, and this particular error was something I thought I would land up until the very end where I found I had apparently missed a few thousand errors in g3 and so it was not going to be ready in time.

[MichaelRFairhurst]

I'm also fine with making it a warning, and an error in 3.0.

[MichaelRFairhurst]

I never finished rolling back my CL here, it was only rolled back in th edev channel:

https://dart-review.googlesource.com/c/sdk/+/67100

[Stargator]

Should this get moved to Dart2.1?

[MichaelRFairhurst]

I don't think we plan on landing this until at least Dart 3, since its too large a breaking change in practice.

It can be caught with the lint rule, await_only_futures

[Stargator]

@MichaelRFairhurst Was this caught too late in the development of Dart 2.0 to be fixed there?

[MichaelRFairhurst]

@Stargator yep. I missed it. There were thousands of await voids in code we had access to, and I was not able to clean up enough of them fast enough.

[Stargator]

@MichaelRFairhurst Ah okay, well. I'd be curious if there might be a way to automate the clean up so whenever this change is done, it doesn't require as much direct intervention.

[MichaelRFairhurst]

Unfortunately its less automatable than you might think.

I saw a few fixes:

• usually, the await was bogus, people were defensively awaiting operations that seemed asynchronous, and added in an await where it neither was necessary nor affected timing in a meaningful way
• Next most commonly, people used void async instead of Future<void> async. However, its not desirable to automate this cleanup for 100% of cases. Sometimes "void async" means "an async method that should not be awaited" and no script can tell when that was the intention.
• Next most commonly, people made void abstract methods which were subclassed with async ones. There, the parent class definition has to change. And its always possible that not all subclasses agree -- one may be async, and the other may synchronously return an int. You have to get them to agree here.
• Lastly, awaited void methods sometimes indicated an async operation that should be awaitable, was not. These would be, for instance, void methods that used the Future API or didn't themselves await calls to other async functions. The correct fix is to make it possible for callers to await the async work done in the void method

So I don't have any automation on this that I'm able to share -- but mostly so far, we've done manual cleanup.

[MichaelRFairhurst]

Ah also, I saw some awaits in the wrong place, ie:

foo.bar().doAsyncThing();
expect(await foo.bar(), someMatcher);

here foo.bar() was synchronous, and it was required since otherwise doAsyncThing() won't be done before someMatcher. But the correct fix is to move the await from foo.bar() to await foo.bar().doAsyncThing().