[VM parser] Insert missing result type check in async functions using arrow

[VM parser] Insert missing result type check in async functions using arrow
style instead of 'return' keyword (fixes #30339).
Add regression test.

R=asiva@google.com, cbernaschina@google.com

Committed: https://github.com/dart-lang/sdk/commit/a18b318fed04a383392d3805326f60fd1bb73bcd

[regis, 2017-08-07 22:09:09]

regis@google.com changed reviewers:
+ asiva@google.com, cbernaschina@google.com

[cbernaschina, 2017-08-07 22:19:39]

Should we foresee another possible future regression?

https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
File tests/language_2/regress_30339_test.dart (right):

https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
tests/language_2/regress_30339_test.dart:24: Future<int> bar() async => x;
Should we test even against possible future optimization on async/await?

By testing:

```dart
Future<String> _foo() async {
    return x;
}

Future<int> awaitFoo() async {
    return await _foo();
}
```
and

```dart
Future<String> _bar() async => x;

Future<int> awaitBar() async => await _bar();
```

[regis, 2017-08-07 22:37:38]

https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
File tests/language_2/regress_30339_test.dart (right):

https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
tests/language_2/regress_30339_test.dart:24: Future<int> bar() async => x;
On 2017/08/07 22:19:38, cbernaschina wrote:
> Should we test even against possible future optimization on async/await?
> 
> By testing:
> 
> ```dart
> Future<String> _foo() async {
>     return x;
> }
> 
> Future<int> awaitFoo() async {
>     return await _foo();
> }
> ```
> and
> 
> ```dart
> Future<String> _bar() async => x;
> 
> Future<int> awaitBar() async => await _bar();
> ```

What kind of optimizations do you have in mind?
The type check is inserted in the ast. Any inlining will preserve this original
ast and will only remove the type test if it is correct to do so. I do not think
we need to make the test more complicated.

[cbernaschina, 2017-08-07 22:47:18]

On 2017/08/07 22:37:38, regis wrote:
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> File tests/language_2/regress_30339_test.dart (right):
> 
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> tests/language_2/regress_30339_test.dart:24: Future<int> bar() async => x;
> On 2017/08/07 22:19:38, cbernaschina wrote:
> > Should we test even against possible future optimization on async/await?
> > 
> > By testing:
> > 
> > ```dart
> > Future<String> _foo() async {
> >     return x;
> > }
> > 
> > Future<int> awaitFoo() async {
> >     return await _foo();
> > }
> > ```
> > and
> > 
> > ```dart
> > Future<String> _bar() async => x;
> > 
> > Future<int> awaitBar() async => await _bar();
> > ```
> 
> What kind of optimizations do you have in mind?
> The type check is inserted in the ast. Any inlining will preserve this
original
> ast and will only remove the type test if it is correct to do so. I do not
think
> we need to make the test more complicated.

I was thinking about the fact that a
```
return await future;
```
can be potentially optimized removing the 'await' because it has no practical
effect.

[regis, 2017-08-07 22:52:05]

On 2017/08/07 22:47:18, cbernaschina wrote:
> On 2017/08/07 22:37:38, regis wrote:
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > File tests/language_2/regress_30339_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > tests/language_2/regress_30339_test.dart:24: Future<int> bar() async => x;
> > On 2017/08/07 22:19:38, cbernaschina wrote:
> > > Should we test even against possible future optimization on async/await?
> > > 
> > > By testing:
> > > 
> > > ```dart
> > > Future<String> _foo() async {
> > >     return x;
> > > }
> > > 
> > > Future<int> awaitFoo() async {
> > >     return await _foo();
> > > }
> > > ```
> > > and
> > > 
> > > ```dart
> > > Future<String> _bar() async => x;
> > > 
> > > Future<int> awaitBar() async => await _bar();
> > > ```
> > 
> > What kind of optimizations do you have in mind?
> > The type check is inserted in the ast. Any inlining will preserve this
> original
> > ast and will only remove the type test if it is correct to do so. I do not
> think
> > we need to make the test more complicated.
> 
> I was thinking about the fact that a
> ```
> return await future;
> ```
> can be potentially optimized removing the 'await' because it has no practical
> effect.

The presence of the await is irrelevant for the insertion of the type test. Note
that the regression test simply returns 1 without await.

[siva, 2017-08-07 22:58:42]

LGTM.

When I run your test case with the new front end I don't get an error from foo
or bar, may need some status file adjustment and a bug for that.

[cbernaschina, 2017-08-07 23:01:31]

On 2017/08/07 22:52:05, regis wrote:
> On 2017/08/07 22:47:18, cbernaschina wrote:
> > On 2017/08/07 22:37:38, regis wrote:
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > File tests/language_2/regress_30339_test.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > tests/language_2/regress_30339_test.dart:24: Future<int> bar() async => x;
> > > On 2017/08/07 22:19:38, cbernaschina wrote:
> > > > Should we test even against possible future optimization on async/await?
> > > > 
> > > > By testing:
> > > > 
> > > > ```dart
> > > > Future<String> _foo() async {
> > > >     return x;
> > > > }
> > > > 
> > > > Future<int> awaitFoo() async {
> > > >     return await _foo();
> > > > }
> > > > ```
> > > > and
> > > > 
> > > > ```dart
> > > > Future<String> _bar() async => x;
> > > > 
> > > > Future<int> awaitBar() async => await _bar();
> > > > ```
> > > 
> > > What kind of optimizations do you have in mind?
> > > The type check is inserted in the ast. Any inlining will preserve this
> > original
> > > ast and will only remove the type test if it is correct to do so. I do not
> > think
> > > we need to make the test more complicated.
> > 
> > I was thinking about the fact that a
> > ```
> > return await future;
> > ```
> > can be potentially optimized removing the 'await' because it has no
practical
> > effect.
> 
> The presence of the await is irrelevant for the insertion of the type test.
Note
> that the regression test simply returns 1 without await.

I was thinking about this edge case (because i actually encountered it):
```
Future foo() async {
  return 1;
}

Future<String> bar() async {
  return await foo();
}

Future main() async {
  int a = await bar();
}
```
This case throws and exception.
```
Future foo() async {
  return 1;
}

Future<String> bar() async {
  return foo(); // no await
}

Future main() async {
  int a = await bar();
}
```
This case instead works fine because Future is downcasted to Future<String> and
when it is resolved in an integer the type check passes.

If an optimization "remove needless await" is introduced the behavior will
change.

[regis, 2017-08-07 23:05:11]

On 2017/08/07 22:58:42, siva wrote:
> LGTM.
> 
> When I run your test case with the new front end I don't get an error from foo
> or bar, may need some status file adjustment and a bug for that.

I am able to run this command without error:
./tools/test.py --time --progress=color --mode=debug --arch=x64 --compiler=dartk
--runtime=vm language_2/regress_30339

Is there another one involving kernel I should try?

[regis, 2017-08-07 23:12:00]

On 2017/08/07 23:01:31, cbernaschina wrote:
> On 2017/08/07 22:52:05, regis wrote:
> > On 2017/08/07 22:47:18, cbernaschina wrote:
> > > On 2017/08/07 22:37:38, regis wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > File tests/language_2/regress_30339_test.dart (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > tests/language_2/regress_30339_test.dart:24: Future<int> bar() async =>
x;
> > > > On 2017/08/07 22:19:38, cbernaschina wrote:
> > > > > Should we test even against possible future optimization on
async/await?
> > > > > 
> > > > > By testing:
> > > > > 
> > > > > ```dart
> > > > > Future<String> _foo() async {
> > > > >     return x;
> > > > > }
> > > > > 
> > > > > Future<int> awaitFoo() async {
> > > > >     return await _foo();
> > > > > }
> > > > > ```
> > > > > and
> > > > > 
> > > > > ```dart
> > > > > Future<String> _bar() async => x;
> > > > > 
> > > > > Future<int> awaitBar() async => await _bar();
> > > > > ```
> > > > 
> > > > What kind of optimizations do you have in mind?
> > > > The type check is inserted in the ast. Any inlining will preserve this
> > > original
> > > > ast and will only remove the type test if it is correct to do so. I do
not
> > > think
> > > > we need to make the test more complicated.
> > > 
> > > I was thinking about the fact that a
> > > ```
> > > return await future;
> > > ```
> > > can be potentially optimized removing the 'await' because it has no
> practical
> > > effect.
> > 
> > The presence of the await is irrelevant for the insertion of the type test.
> Note
> > that the regression test simply returns 1 without await.
> 
> I was thinking about this edge case (because i actually encountered it):
> ```
> Future foo() async {
>   return 1;
> }
> 
> Future<String> bar() async {
>   return await foo();
> }
> 
> Future main() async {
>   int a = await bar();
> }
> ```
> This case throws and exception.
> ```
> Future foo() async {
>   return 1;
> }
> 
> Future<String> bar() async {
>   return foo(); // no await
> }
> 
> Future main() async {
>   int a = await bar();
> }
> ```
> This case instead works fine because Future is downcasted to Future<String>
and
> when it is resolved in an integer the type check passes.
> 
> If an optimization "remove needless await" is introduced the behavior will
> change.

Both cases fail in the same way for me. As mentioned earlier, the await is not
relevant in inserting the type test.
We generate a type test because we return from an async function that is
declaring a return type of the form Future<T>. We therefore check that the
result (await or not) is of type T or of type Future<T>.

[regis, 2017-08-07 23:13:40]

On 2017/08/07 23:12:00, regis wrote:
> On 2017/08/07 23:01:31, cbernaschina wrote:
> > On 2017/08/07 22:52:05, regis wrote:
> > > On 2017/08/07 22:47:18, cbernaschina wrote:
> > > > On 2017/08/07 22:37:38, regis wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > > File tests/language_2/regress_30339_test.dart (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > > tests/language_2/regress_30339_test.dart:24: Future<int> bar() async
=>
> x;
> > > > > On 2017/08/07 22:19:38, cbernaschina wrote:
> > > > > > Should we test even against possible future optimization on
> async/await?
> > > > > > 
> > > > > > By testing:
> > > > > > 
> > > > > > ```dart
> > > > > > Future<String> _foo() async {
> > > > > >     return x;
> > > > > > }
> > > > > > 
> > > > > > Future<int> awaitFoo() async {
> > > > > >     return await _foo();
> > > > > > }
> > > > > > ```
> > > > > > and
> > > > > > 
> > > > > > ```dart
> > > > > > Future<String> _bar() async => x;
> > > > > > 
> > > > > > Future<int> awaitBar() async => await _bar();
> > > > > > ```
> > > > > 
> > > > > What kind of optimizations do you have in mind?
> > > > > The type check is inserted in the ast. Any inlining will preserve this
> > > > original
> > > > > ast and will only remove the type test if it is correct to do so. I do
> not
> > > > think
> > > > > we need to make the test more complicated.
> > > > 
> > > > I was thinking about the fact that a
> > > > ```
> > > > return await future;
> > > > ```
> > > > can be potentially optimized removing the 'await' because it has no
> > practical
> > > > effect.
> > > 
> > > The presence of the await is irrelevant for the insertion of the type
test.
> > Note
> > > that the regression test simply returns 1 without await.
> > 
> > I was thinking about this edge case (because i actually encountered it):
> > ```
> > Future foo() async {
> >   return 1;
> > }
> > 
> > Future<String> bar() async {
> >   return await foo();
> > }
> > 
> > Future main() async {
> >   int a = await bar();
> > }
> > ```
> > This case throws and exception.
> > ```
> > Future foo() async {
> >   return 1;
> > }
> > 
> > Future<String> bar() async {
> >   return foo(); // no await
> > }
> > 
> > Future main() async {
> >   int a = await bar();
> > }
> > ```
> > This case instead works fine because Future is downcasted to Future<String>
> and
> > when it is resolved in an integer the type check passes.
> > 
> > If an optimization "remove needless await" is introduced the behavior will
> > change.
> 
> Both cases fail in the same way for me. As mentioned earlier, the await is not
> relevant in inserting the type test.
> We generate a type test because we return from an async function that is
> declaring a return type of the form Future<T>. We therefore check that the
> result (await or not) is of type T or of type Future<T>.

Wait, I was missing an import and the error I am getting is unrelated.

[regis, 2017-08-07 23:17:26]

On 2017/08/07 23:13:40, regis wrote:
> On 2017/08/07 23:12:00, regis wrote:
> > On 2017/08/07 23:01:31, cbernaschina wrote:
> > > On 2017/08/07 22:52:05, regis wrote:
> > > > On 2017/08/07 22:47:18, cbernaschina wrote:
> > > > > On 2017/08/07 22:37:38, regis wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > > > File tests/language_2/regress_30339_test.dart (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > > > tests/language_2/regress_30339_test.dart:24: Future<int> bar() async
> =>
> > x;
> > > > > > On 2017/08/07 22:19:38, cbernaschina wrote:
> > > > > > > Should we test even against possible future optimization on
> > async/await?
> > > > > > > 
> > > > > > > By testing:
> > > > > > > 
> > > > > > > ```dart
> > > > > > > Future<String> _foo() async {
> > > > > > >     return x;
> > > > > > > }
> > > > > > > 
> > > > > > > Future<int> awaitFoo() async {
> > > > > > >     return await _foo();
> > > > > > > }
> > > > > > > ```
> > > > > > > and
> > > > > > > 
> > > > > > > ```dart
> > > > > > > Future<String> _bar() async => x;
> > > > > > > 
> > > > > > > Future<int> awaitBar() async => await _bar();
> > > > > > > ```
> > > > > > 
> > > > > > What kind of optimizations do you have in mind?
> > > > > > The type check is inserted in the ast. Any inlining will preserve
this
> > > > > original
> > > > > > ast and will only remove the type test if it is correct to do so. I
do
> > not
> > > > > think
> > > > > > we need to make the test more complicated.
> > > > > 
> > > > > I was thinking about the fact that a
> > > > > ```
> > > > > return await future;
> > > > > ```
> > > > > can be potentially optimized removing the 'await' because it has no
> > > practical
> > > > > effect.
> > > > 
> > > > The presence of the await is irrelevant for the insertion of the type
> test.
> > > Note
> > > > that the regression test simply returns 1 without await.
> > > 
> > > I was thinking about this edge case (because i actually encountered it):
> > > ```
> > > Future foo() async {
> > >   return 1;
> > > }
> > > 
> > > Future<String> bar() async {
> > >   return await foo();
> > > }
> > > 
> > > Future main() async {
> > >   int a = await bar();
> > > }
> > > ```
> > > This case throws and exception.
> > > ```
> > > Future foo() async {
> > >   return 1;
> > > }
> > > 
> > > Future<String> bar() async {
> > >   return foo(); // no await
> > > }
> > > 
> > > Future main() async {
> > >   int a = await bar();
> > > }
> > > ```
> > > This case instead works fine because Future is downcasted to
Future<String>
> > and
> > > when it is resolved in an integer the type check passes.
> > > 
> > > If an optimization "remove needless await" is introduced the behavior will
> > > change.
> > 
> > Both cases fail in the same way for me. As mentioned earlier, the await is
not
> > relevant in inserting the type test.
> > We generate a type test because we return from an async function that is
> > declaring a return type of the form Future<T>. We therefore check that the
> > result (await or not) is of type T or of type Future<T>.
> 
> Wait, I was missing an import and the error I am getting is unrelated.

You are right that without the await, the run time type of the result of foo()
is Future. But I do not think it is related to this bug, and I do not think
reporting an error here is correct.

[cbernaschina, 2017-08-07 23:29:46]

On 2017/08/07 23:17:26, regis wrote:
> On 2017/08/07 23:13:40, regis wrote:
> > On 2017/08/07 23:12:00, regis wrote:
> > > On 2017/08/07 23:01:31, cbernaschina wrote:
> > > > On 2017/08/07 22:52:05, regis wrote:
> > > > > On 2017/08/07 22:47:18, cbernaschina wrote:
> > > > > > On 2017/08/07 22:37:38, regis wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > > > > File tests/language_2/regress_30339_test.dart (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2993223002/diff/1/tests/language_2/regress_30...
> > > > > > > tests/language_2/regress_30339_test.dart:24: Future<int> bar()
async
> > =>
> > > x;
> > > > > > > On 2017/08/07 22:19:38, cbernaschina wrote:
> > > > > > > > Should we test even against possible future optimization on
> > > async/await?
> > > > > > > > 
> > > > > > > > By testing:
> > > > > > > > 
> > > > > > > > ```dart
> > > > > > > > Future<String> _foo() async {
> > > > > > > >     return x;
> > > > > > > > }
> > > > > > > > 
> > > > > > > > Future<int> awaitFoo() async {
> > > > > > > >     return await _foo();
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > and
> > > > > > > > 
> > > > > > > > ```dart
> > > > > > > > Future<String> _bar() async => x;
> > > > > > > > 
> > > > > > > > Future<int> awaitBar() async => await _bar();
> > > > > > > > ```
> > > > > > > 
> > > > > > > What kind of optimizations do you have in mind?
> > > > > > > The type check is inserted in the ast. Any inlining will preserve
> this
> > > > > > original
> > > > > > > ast and will only remove the type test if it is correct to do so.
I
> do
> > > not
> > > > > > think
> > > > > > > we need to make the test more complicated.
> > > > > > 
> > > > > > I was thinking about the fact that a
> > > > > > ```
> > > > > > return await future;
> > > > > > ```
> > > > > > can be potentially optimized removing the 'await' because it has no
> > > > practical
> > > > > > effect.
> > > > > 
> > > > > The presence of the await is irrelevant for the insertion of the type
> > test.
> > > > Note
> > > > > that the regression test simply returns 1 without await.
> > > > 
> > > > I was thinking about this edge case (because i actually encountered it):
> > > > ```
> > > > Future foo() async {
> > > >   return 1;
> > > > }
> > > > 
> > > > Future<String> bar() async {
> > > >   return await foo();
> > > > }
> > > > 
> > > > Future main() async {
> > > >   int a = await bar();
> > > > }
> > > > ```
> > > > This case throws and exception.
> > > > ```
> > > > Future foo() async {
> > > >   return 1;
> > > > }
> > > > 
> > > > Future<String> bar() async {
> > > >   return foo(); // no await
> > > > }
> > > > 
> > > > Future main() async {
> > > >   int a = await bar();
> > > > }
> > > > ```
> > > > This case instead works fine because Future is downcasted to
> Future<String>
> > > and
> > > > when it is resolved in an integer the type check passes.
> > > > 
> > > > If an optimization "remove needless await" is introduced the behavior
will
> > > > change.
> > > 
> > > Both cases fail in the same way for me. As mentioned earlier, the await is
> not
> > > relevant in inserting the type test.
> > > We generate a type test because we return from an async function that is
> > > declaring a return type of the form Future<T>. We therefore check that the
> > > result (await or not) is of type T or of type Future<T>.
> > 
> > Wait, I was missing an import and the error I am getting is unrelated.
> 
> You are right that without the await, the run time type of the result of foo()
> is Future. But I do not think it is related to this bug, and I do not think
> reporting an error here is correct.

LGTM

You are right that is out of the scope of this CL.

[siva, 2017-08-07 23:32:26]

On 2017/08/07 23:05:11, regis wrote:
> On 2017/08/07 22:58:42, siva wrote:
> > LGTM.
> > 
> > When I run your test case with the new front end I don't get an error from
foo
> > or bar, may need some status file adjustment and a bug for that.
> 
> I am able to run this command without error:
> ./tools/test.py --time --progress=color --mode=debug --arch=x64
--compiler=dartk
> --runtime=vm language_2/regress_30339
> 
> Is there another one involving kernel I should try?

That is the one but you are not using --checked option.

[regis, 2017-08-07 23:38:00]

On 2017/08/07 23:32:26, siva wrote:
> On 2017/08/07 23:05:11, regis wrote:
> > On 2017/08/07 22:58:42, siva wrote:
> > > LGTM.
> > > 
> > > When I run your test case with the new front end I don't get an error from
> foo
> > > or bar, may need some status file adjustment and a bug for that.
> > 
> > I am able to run this command without error:
> > ./tools/test.py --time --progress=color --mode=debug --arch=x64
> --compiler=dartk
> > --runtime=vm language_2/regress_30339
> > 
> > Is there another one involving kernel I should try?
> 
> That is the one but you are not using --checked option.

Ah, good catch! Thanks! I've updated the status file.
Too many configurations to test.

[regis, 2017-08-08 16:15:33]

Description was changed from

==========
[VM parser] Insert missing result type check in async functions using arrow
style instead of 'return' keyword (fixes #30339).
Add regression test.
==========

to

==========
[VM parser] Insert missing result type check in async functions using arrow
style instead of 'return' keyword (fixes #30339).
Add regression test.

R=asiva@google.com, cbernaschina@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a18b318fed04a383392d3805326f60fd1bb73bcd
==========

[regis, 2017-08-08 16:15:34]

Committed patchset #3 (id:40001) manually as
a18b318fed04a383392d3805326f60fd1bb73bcd (presubmit successful).