Fix many bugs with closure conversion in checked mode.

Fix many bugs with closure conversion in checked mode.

Summary:

Previously, we use the "Vector" type in the kernel tree for the result of the
"VectorCreation" operation as as the parameter type for converted closure
functions. In the VM, we use the "Context" type instead, which the VM treats a
little differently than normal Dart-visible types, and it doesn't not handle
type checks against it correctly, breaking all closure converted code running in
checked mode.

Now, Since we are forced to use dynamic to represent the types of elements of
the context, we may as well just use dynamic for the context type itself.

Previously, we did not correct handle converted closure type checks for closures
that capture type parameters. The way we handled these type checks also had
several latent bugs that prevented type parameters being handled properly.

Now, we handle type parameters for converted closures similarly to normal
closures, and the places we treat them specially are fewer and more integrated
with the rest of the closure type checking code.

There is still a problem where assignments to captured variables are not
checked, because they are transformed to assignments into the context, whose
elements are necessarily untyped. This breaks many co19 tests, which expect type
errors on these assignments. The example below should error in checked mode, but
after closure conversion is runs with no errors.

int b;
bool c;
(() { b = c; })()

Test Plan:

- All test cases in "pkg/kernel/testcases/closures" now run in checked mode.

- Added a test "closures_types.dart" to check that captured type parameters are
  handled correctly in the converted closures' signature types.

R=dmitryas@google.com

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

[sjindel, 2017-08-26 17:49:45]

sjindel@google.com changed reviewers:
+ dmitryas@google.com, karlklose@chromium.org

[sjindel, 2017-08-26 17:50:06]

Description was changed from

==========
Fix many bugs with closure conversion in checked mode.

Summary:

Previously, we use the "Vector" type in the kernel tree for the result of the
"VectorCreation" operation as as the parameter type for converted closure
functions. In the VM, we use the "Context" type instead, which the VM treats a
little differently than normal Dart-visible types, and it doesn't not handle
type checks against it correctly, breaking all closure converted code running in
checked mode.

Now, Since we are forced to use dynamic to represent the types of elements of
the context, we may as well just use dynamic for the context type itself.

Previously, we did not correct handle converted closure type checks for closures
that capture type parameters. The way we handled these type checks also had
several latent bugs that prevented type parameters being handled properly.

Now, we handle type parameters for converted closures similarly to normal
closures, and the places we treat them specially are fewer and more integrated
with the rest of the closure type checking code.

There is still a problem where assignments to captured variables are not
checked, because they are transformed to assignments into the context, whose
elements are necessarily untyped. This breaks many co19 tests, which expect type
errors on these assignments. The example below should error in checked mode, but
after closure conversion is runs with no errors.

int b;
bool c;
(() { b = c; })()

Test Plan:

- All test cases in "pkg/kernel/testcases/closures" now run in checked mode.

- Added a test "closures_types.dart" to check that captured type parameters are
  handled correctly in the converted closures' signature types.

- All co19 tests pass with closure conversion in checked mode (except the one
  that doesn't pass without closure conversion).
==========

to

==========
Fix many bugs with closure conversion in checked mode.

Summary:

Previously, we use the "Vector" type in the kernel tree for the result of the
"VectorCreation" operation as as the parameter type for converted closure
functions. In the VM, we use the "Context" type instead, which the VM treats a
little differently than normal Dart-visible types, and it doesn't not handle
type checks against it correctly, breaking all closure converted code running in
checked mode.

Now, Since we are forced to use dynamic to represent the types of elements of
the context, we may as well just use dynamic for the context type itself.

Previously, we did not correct handle converted closure type checks for closures
that capture type parameters. The way we handled these type checks also had
several latent bugs that prevented type parameters being handled properly.

Now, we handle type parameters for converted closures similarly to normal
closures, and the places we treat them specially are fewer and more integrated
with the rest of the closure type checking code.

There is still a problem where assignments to captured variables are not
checked, because they are transformed to assignments into the context, whose
elements are necessarily untyped. This breaks many co19 tests, which expect type
errors on these assignments. The example below should error in checked mode, but
after closure conversion is runs with no errors.

int b;
bool c;
(() { b = c; })()

Test Plan:

- All test cases in "pkg/kernel/testcases/closures" now run in checked mode.

- Added a test "closures_types.dart" to check that captured type parameters are
  handled correctly in the converted closures' signature types.
==========

[Dmitry Stefantsov, 2017-08-28 11:38:15]

LGTM with a few suggestions.

https://codereview.chromium.org/3007623002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/closure_types.dart (right):

https://codereview.chromium.org/3007623002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/closure_types.dart:1: class C<T> {
Please add a standard preamble and a description.

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (left):

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#oldcod...
runtime/vm/object.cc:15364: if (sig_fun.IsConvertedClosureFunction()) {
Nice to see this code transformed into something more reasonable!

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7197: // class C<T> {
We try to keep the formatting compatible with Markdown, so that dartdoc may
generate nice HTML code.  Usually code examples in comments are indented with 4
spaces (so that there are 5 spaces in total after "//") to be formatted as code.

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7209: // "<T>(T) => dynamic", because the signaute of the
converted closure function
Another Markdown-related adjustment may be changing "fn" to `fn` and "<T>(T) =>
dynamic" to `<T>(T) => dynamic`.

[Vyacheslav Egorov (Google), 2017-08-28 11:55:47]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2017-08-28 11:55:48]

drive by comments (VM code style nits - not a semantics review)

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6339: sig ^= Function::NewConvertedClosureFunction(
sig = Function::New...

^= is only used when down-casting (e.g. you have RawObject* and you want to
store it into Function& handle).

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6341: // TODO(sjindel): Kernel generic methods undone.
Handle type parameters
In VM code we these days encourage usage of TODO(dartbug.com/issue-number)
instead of TODO(username)

TODO(username) usually proliferate uncontrollably and it's also hard to find
which ones related to which issues. 

e.g. here it should probably be TODO(dartbug.com/30455) or TODO(30455) for
short.

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7216: if (genericity == kCurrentClass) return true;
in general single line if-statements are frowned upon in the VM code. 

usually it is recommended to fully brace the if-statement even if is trivial.

I suggest rewriting this code to avoid if entirely.

return (genericity == kCurrentClass) || (NumTypeParameters() == 0);

[sjindel, 2017-08-28 15:05:59]

https://codereview.chromium.org/3007623002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/closure_types.dart (right):

https://codereview.chromium.org/3007623002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/closure_types.dart:1: class C<T> {
On 2017/08/28 11:38:15, Dmitry Stefantsov wrote:
> Please add a standard preamble and a description.

Done.

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6341: // TODO(sjindel): Kernel generic methods undone.
Handle type parameters
On 2017/08/28 11:55:48, Vyacheslav Egorov (Google) wrote:
> In VM code we these days encourage usage of TODO(dartbug.com/issue-number)
> instead of TODO(username)
> 
> TODO(username) usually proliferate uncontrollably and it's also hard to find
> which ones related to which issues. 
> 
> e.g. here it should probably be TODO(dartbug.com/30455) or TODO(30455) for
> short.
> 

Understood, but this comment follows the pattern of the other places where we
have code that won't work with generic methods; i.e., it starts with:

"Kernel generic methods undone."

I could change all the other places with this pattern (there are several), but I
feel it's out of the scope of this revision.

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7197: // class C<T> {
On 2017/08/28 11:38:15, Dmitry Stefantsov wrote:
> We try to keep the formatting compatible with Markdown, so that dartdoc may
> generate nice HTML code.  Usually code examples in comments are indented with
4
> spaces (so that there are 5 spaces in total after "//") to be formatted as
code.

Done, but does dartdoc work on C++ code too?

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7209: // "<T>(T) => dynamic", because the signaute of the
converted closure function
On 2017/08/28 11:38:15, Dmitry Stefantsov wrote:
> Another Markdown-related adjustment may be changing "fn" to `fn` and "<T>(T)
=>
> dynamic" to `<T>(T) => dynamic`.

Done.

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7216: if (genericity == kCurrentClass) return true;
On 2017/08/28 11:55:48, Vyacheslav Egorov (Google) wrote:
> in general single line if-statements are frowned upon in the VM code. 
> 
> usually it is recommended to fully brace the if-statement even if is trivial.
> 
> I suggest rewriting this code to avoid if entirely.
> 
> return (genericity == kCurrentClass) || (NumTypeParameters() == 0);
> 

Done.

[sjindel, 2017-08-28 15:39:39]

Description was changed from

==========
Fix many bugs with closure conversion in checked mode.

Summary:

Previously, we use the "Vector" type in the kernel tree for the result of the
"VectorCreation" operation as as the parameter type for converted closure
functions. In the VM, we use the "Context" type instead, which the VM treats a
little differently than normal Dart-visible types, and it doesn't not handle
type checks against it correctly, breaking all closure converted code running in
checked mode.

Now, Since we are forced to use dynamic to represent the types of elements of
the context, we may as well just use dynamic for the context type itself.

Previously, we did not correct handle converted closure type checks for closures
that capture type parameters. The way we handled these type checks also had
several latent bugs that prevented type parameters being handled properly.

Now, we handle type parameters for converted closures similarly to normal
closures, and the places we treat them specially are fewer and more integrated
with the rest of the closure type checking code.

There is still a problem where assignments to captured variables are not
checked, because they are transformed to assignments into the context, whose
elements are necessarily untyped. This breaks many co19 tests, which expect type
errors on these assignments. The example below should error in checked mode, but
after closure conversion is runs with no errors.

int b;
bool c;
(() { b = c; })()

Test Plan:

- All test cases in "pkg/kernel/testcases/closures" now run in checked mode.

- Added a test "closures_types.dart" to check that captured type parameters are
  handled correctly in the converted closures' signature types.
==========

to

==========
Fix many bugs with closure conversion in checked mode.

Summary:

Previously, we use the "Vector" type in the kernel tree for the result of the
"VectorCreation" operation as as the parameter type for converted closure
functions. In the VM, we use the "Context" type instead, which the VM treats a
little differently than normal Dart-visible types, and it doesn't not handle
type checks against it correctly, breaking all closure converted code running in
checked mode.

Now, Since we are forced to use dynamic to represent the types of elements of
the context, we may as well just use dynamic for the context type itself.

Previously, we did not correct handle converted closure type checks for closures
that capture type parameters. The way we handled these type checks also had
several latent bugs that prevented type parameters being handled properly.

Now, we handle type parameters for converted closures similarly to normal
closures, and the places we treat them specially are fewer and more integrated
with the rest of the closure type checking code.

There is still a problem where assignments to captured variables are not
checked, because they are transformed to assignments into the context, whose
elements are necessarily untyped. This breaks many co19 tests, which expect type
errors on these assignments. The example below should error in checked mode, but
after closure conversion is runs with no errors.

int b;
bool c;
(() { b = c; })()

Test Plan:

- All test cases in "pkg/kernel/testcases/closures" now run in checked mode.

- Added a test "closures_types.dart" to check that captured type parameters are
  handled correctly in the converted closures' signature types.

R=dmitryas@google.com

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

[sjindel, 2017-08-28 15:39:41]

Committed patchset #2 (id:20001) manually as
7bf835b9d5cfc6849e97758b1dcb2cfdd2be589b (presubmit successful).

[Vyacheslav Egorov (Google), 2017-08-28 15:59:12]

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/3007623002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6341: // TODO(sjindel): Kernel generic methods undone.
Handle type parameters
I think this comment is the only one that does not follow the convention:

$ git grep "Kernel generic methods undone"
runtime/vm/kernel_binary_flowgraph.cc:    // TODO(30455): Kernel generic methods
undone. Currently we only support
runtime/vm/kernel_binary_flowgraph.cc:  // TODO(30455): Kernel generic methods
undone. Since the frontend can't yet
runtime/vm/kernel_binary_flowgraph.cc:  // TODO(30455): Kernel generic methods
undone. When generic methods are
runtime/vm/kernel_to_il.cc:// TODO(30455): Kernel generic methods undone. This
seems to work for static
runtime/vm/object.cc:    // TODO(sjindel): Kernel generic methods undone. Handle
type parameters

[regis, 2017-08-28 23:47:52]

regis@google.com changed reviewers:
+ regis@google.com

[regis, 2017-08-28 23:47:52]

Samir,

This cl broke reification of generic functions in the VM. I cannot blame you,
because it is not enabled yet, so it is difficult for you to notice. And since
the async zone cl by Florian is not submitted, you cannot test it yourself
either.

I have not looked into what exactly got broken, but it has to do with generic
closures declaring local generic closures. The parent fails to allocate a local
variable holding the passed in type arguments. I'll look at it tomorrow.

In the future, please cc me on such changes until reification of generic
functions is enabled and better tested in the VM.

Thanks,
Regis

[sjindel, 2017-08-29 13:11:42]

On 2017/08/28 23:47:52, regis wrote:
> Samir,
> 
> This cl broke reification of generic functions in the VM. I cannot blame you,
> because it is not enabled yet, so it is difficult for you to notice. And since
> the async zone cl by Florian is not submitted, you cannot test it yourself
> either.
> 
> I have not looked into what exactly got broken, but it has to do with generic
> closures declaring local generic closures. The parent fails to allocate a
local
> variable holding the passed in type arguments. I'll look at it tomorrow.
> 
> In the future, please cc me on such changes until reification of generic
> functions is enabled and better tested in the VM.
> 
> Thanks,
> Regis

Will do.