[kernel] Support for top-level generic functions.

[kernel] Support for top-level generic functions.

Summary:

Previously, there was no support for generic methods in kernel. This prevented
us from being able to pass captured type arguments to the target top-level
function in converted closures, so these type arguments were always instantiated
to 'dynamic'.

Now, we save the type arguments to the closure creation operation in the
context, and read them out and forward them appropriately in closure wrapper
function. Since fasta doesn't currently support generic methods (their type
parameters are replaced by 'dynamic'), only top-level generic functions can
surface in kernel, as they are generated by closure conversion of closures that
capture type parameters of a class.

My focus here is enabling closure conversion to work in only these cases, and as
such, the code has some temporary "hacks" in the VM that may not work for
generic member functions or generic closures when they are enabled in fasta.

Test Plan:

I ran all the tests in closures/, and those which were previously expected to
crash due to missing VM support now pass and produce correct results.

Further testing is paused until we understand why the recent commit "[kernel]
Insert kernel bodies into VM heap" has broken all these tests.

Reviewers: regis@google.com, jensj@google.com, dmitryas@google.com

BUG=
R=dmitryas@google.com, jensj@google.com

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

[sjindel, 2017-08-10 18:18:05]

sjindel@google.com changed reviewers:
+ dmitryas@google.com, jensj@google.com, regis@google.com

[regis, 2017-08-10 23:16:56]

I will let people more familiar with kernel code give you approval. The
runtime/vm files look good to me (see comments).

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:480: intptr_t type_args_count = 0);
Please call this 'type_args_len' to be consistent with the VM code. A 'count'
could only be 0 (no type argument vector passed) or 1 (a vector is passed). We
actually provide a bit more information by passing the length of the type
argument vector, 0 meaning none is passed.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:571: intptr_t type_args_count = 0);
ditto

[jensj, 2017-08-11 06:24:55]

lgtm with comments, but to be honest I don't know a lot about the closure
transformation... I would probably wait for the ok from Dmitry.

What do you mean 'the recent commit "[kernel]
Insert kernel bodies into VM heap" has broken all these tests'?

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:451: if
(capturedTypeVariables[function] != null && accessContext is NullLiteral)
https://www.dartlang.org/guides/language/effective-dart/style#do-use-curly-br...

This doesn't fit on one line => braces.

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/rewriter.dart:78: // Index 1 of a context
always points to the parent.
As I read it there's an inconsistency here: Here you say that index 1 is the
parent. Above you say "the 0-th element works as a link to the parent context."
(vs "the 1-st element of it holds the vector of type arguments")

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1097: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel):
Maybe this should be a TODO(sjindel) instead?

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:2963: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel): Since the frontend can't yet emit
ditto

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:2986: if (type_param_count) {
could this be made explicit? I suppose it means "if (type_param_count != 0)" but
I'm not familiar enough with C++ to be sure... I suppose it would also make more
sense with something like
```
ASSERT(type_param_count >= 0)
if (type_param_count > 0) {
 bla bla bla
}
```
(though I may have misunderstood something).

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:3021: positional_argument_count +
named_argument_count + !!type_param_count;
"!!type_param_count"?

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:5858: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel): When generic methods are fully
todo?

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:797: // KERNEL_GENERIC_METHODS_UNDONE(sjindel): This
seems to work for static
todo?

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1579: intptr_t FlowGraphBuilder::StackDepth() {
I don't think this method is actually used.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:625: intptr_t StackDepth();
I don't think this method is actually used.

[sjindel, 2017-08-11 09:21:07]

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:451: if
(capturedTypeVariables[function] != null && accessContext is NullLiteral)
On 2017/08/11 06:24:54, jensj wrote:
>
https://www.dartlang.org/guides/language/effective-dart/style#do-use-curly-br...
> 
> This doesn't fit on one line => braces.

Done.

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/2998803002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/rewriter.dart:78: // Index 1 of a context
always points to the parent.
On 2017/08/11 06:24:54, jensj wrote:
> As I read it there's an inconsistency here: Here you say that index 1 is the
> parent. Above you say "the 0-th element works as a link to the parent
context."
> (vs "the 1-st element of it holds the vector of type arguments")

Good catch, I forgot to update the comment above.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1097: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel):
On 2017/08/11 06:24:54, jensj wrote:
> Maybe this should be a TODO(sjindel) instead?

I gave it this name so it'll be easy to find when I (or someone else) revisits
generic methods in kernel.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:2986: if (type_param_count) {
On 2017/08/11 06:24:54, jensj wrote:
> could this be made explicit? I suppose it means "if (type_param_count != 0)"
but
> I'm not familiar enough with C++ to be sure... I suppose it would also make
more
> sense with something like
> ```
> ASSERT(type_param_count >= 0)
> if (type_param_count > 0) {
>  bla bla bla
> }
> ```
> (though I may have misunderstood something).

It means "!= 0", but it can't be negative, because it's defined above as the
result of "PeekUInt()", which as the name suggests, doesn't return negative.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:3021: positional_argument_count +
named_argument_count + !!type_param_count;
On 2017/08/11 06:24:54, jensj wrote:
> "!!type_param_count"?

"!!" converts a number to 1 or 0, depending on whether it's positive or not.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:480: intptr_t type_args_count = 0);
On 2017/08/10 23:16:56, regis wrote:
> Please call this 'type_args_len' to be consistent with the VM code. A 'count'
> could only be 0 (no type argument vector passed) or 1 (a vector is passed). We
> actually provide a bit more information by passing the length of the type
> argument vector, 0 meaning none is passed.

Done.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1579: intptr_t FlowGraphBuilder::StackDepth() {
On 2017/08/11 06:24:55, jensj wrote:
> I don't think this method is actually used.

It's not -- I found it useful for debugging, so I left it in for others.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:571: intptr_t type_args_count = 0);
On 2017/08/10 23:16:56, regis wrote:
> ditto

Done.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:625: intptr_t StackDepth();
On 2017/08/11 06:24:55, jensj wrote:
> I don't think this method is actually used.

It's not -- I just found it useful for debugging, so I left it in.

[jensj, 2017-08-14 06:47:13]

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1097: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel):
On 2017/08/11 09:21:06, sjindel wrote:
> On 2017/08/11 06:24:54, jensj wrote:
> > Maybe this should be a TODO(sjindel) instead?
> 
> I gave it this name so it'll be easy to find when I (or someone else) revisits
> generic methods in kernel.

wouldn't 
"TODO(sjindel): Kernel generic methods undone. Possibly more information goes
here."
be just as easy to find? (maybe even more so because you can also find it by
searching for 'TODO'.)

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1579: intptr_t FlowGraphBuilder::StackDepth() {
On 2017/08/11 09:21:06, sjindel wrote:
> On 2017/08/11 06:24:55, jensj wrote:
> > I don't think this method is actually used.
> 
> It's not -- I found it useful for debugging, so I left it in for others.

While that's fine for developing, I don't think we want that in the landed code.
If everyone added unused but 'useful to me'-methods life probably wouldn't be
better.

https://codereview.chromium.org/2998803002/diff/60001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:3021: positional_argument_count +
named_argument_count + !!type_param_count;
Something like

```
intptr_t argument_count = positional_argument_count + named_argument_count;
if (type_param_count > 0) ++argument_count;
```
would read better.

[sjindel, 2017-08-14 11:05:50]

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1097: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel):
On 2017/08/14 06:47:12, jensj wrote:
> On 2017/08/11 09:21:06, sjindel wrote:
> > On 2017/08/11 06:24:54, jensj wrote:
> > > Maybe this should be a TODO(sjindel) instead?
> > 
> > I gave it this name so it'll be easy to find when I (or someone else)
revisits
> > generic methods in kernel.
> 
> wouldn't 
> "TODO(sjindel): Kernel generic methods undone. Possibly more information goes
> here."
> be just as easy to find? (maybe even more so because you can also find it by
> searching for 'TODO'.)

Done, but splitting the "kernel generic methods" part in separate words creates
the possibility that clang-format will split it into separate lines, making it
harder to search for.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:2963: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel): Since the frontend can't yet emit
On 2017/08/11 06:24:54, jensj wrote:
> ditto

Done.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:797: // KERNEL_GENERIC_METHODS_UNDONE(sjindel): This
seems to work for static
On 2017/08/11 06:24:55, jensj wrote:
> todo?

Done.

https://codereview.chromium.org/2998803002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1579: intptr_t FlowGraphBuilder::StackDepth() {
On 2017/08/14 06:47:13, jensj wrote:
> On 2017/08/11 09:21:06, sjindel wrote:
> > On 2017/08/11 06:24:55, jensj wrote:
> > > I don't think this method is actually used.
> > 
> > It's not -- I found it useful for debugging, so I left it in for others.
> 
> While that's fine for developing, I don't think we want that in the landed
code.
> If everyone added unused but 'useful to me'-methods life probably wouldn't be
> better.

Done.

https://codereview.chromium.org/2998803002/diff/60001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:3021: positional_argument_count +
named_argument_count + !!type_param_count;
On 2017/08/14 06:47:13, jensj wrote:
> Something like
> 
> ```
> intptr_t argument_count = positional_argument_count + named_argument_count;
> if (type_param_count > 0) ++argument_count;
> ```
> would read better.

Done, although 'argument_count' can't be 'const' anymore.

[Dmitry Stefantsov, 2017-08-16 11:44:39]

LGTM if you respond to the comments.

https://codereview.chromium.org/2998803002/diff/80001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/captured_class_type_vars.dart (right):

https://codereview.chromium.org/2998803002/diff/80001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/captured_class_type_vars.dart:1: // Copyright (c)
2016, the Dart project authors. Please see the AUTHORS file
2016 ==> 2017

https://codereview.chromium.org/2998803002/diff/80001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/80001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:2994: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel): Since the frontend can't yet emit
How about creating an issue and using something like "TODO(<issue_number>)" here
and other places explaining that the frontend doesn't yet fully support generic
methods?  This way anyone can easily find all the places related to the issue. 
We use this technique for TODOs everywhere.

As I get it from my last conversation with Peter, he's ok with creating an issue
in the tracker for generic methods support in fasta.

[sjindel, 2017-08-16 12:57:12]

https://codereview.chromium.org/2998803002/diff/80001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/captured_class_type_vars.dart (right):

https://codereview.chromium.org/2998803002/diff/80001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/captured_class_type_vars.dart:1: // Copyright (c)
2016, the Dart project authors. Please see the AUTHORS file
On 2017/08/16 11:44:39, Dmitry Stefantsov wrote:
> 2016 ==> 2017

Done.

https://codereview.chromium.org/2998803002/diff/80001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2998803002/diff/80001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:2994: //
KERNEL_GENERIC_METHODS_UNDONE(sjindel): Since the frontend can't yet emit
On 2017/08/16 11:44:39, Dmitry Stefantsov wrote:
> How about creating an issue and using something like "TODO(<issue_number>)"
here
> and other places explaining that the frontend doesn't yet fully support
generic
> methods?  This way anyone can easily find all the places related to the issue.

> We use this technique for TODOs everywhere.
> 
> As I get it from my last conversation with Peter, he's ok with creating an
issue
> in the tracker for generic methods support in fasta.

Done.

[sjindel, 2017-08-16 13:24:31]

Description was changed from

==========
[kernel] Support for top-level generic functions.

Summary:

Previously, there was no support for generic methods in kernel. This prevented
us from being able to pass captured type arguments to the target top-level
function in converted closures, so these type arguments were always instantiated
to 'dynamic'.

Now, we save the type arguments to the closure creation operation in the
context, and read them out and forward them appropriately in closure wrapper
function. Since fasta doesn't currently support generic methods (their type
parameters are replaced by 'dynamic'), only top-level generic functions can
surface in kernel, as they are generated by closure conversion of closures that
capture type parameters of a class.

My focus here is enabling closure conversion to work in only these cases, and as
such, the code has some temporary "hacks" in the VM that may not work for
generic member functions or generic closures when they are enabled in fasta.

Test Plan:

I ran all the tests in closures/, and those which were previously expected to
crash due to missing VM support now pass and produce correct results.

Further testing is paused until we understand why the recent commit "[kernel]
Insert kernel bodies into VM heap" has broken all these tests.

Reviewers: regis@google.com, jensj@google.com, dmitryas@google.com

BUG=
==========

to

==========
[kernel] Support for top-level generic functions.

Summary:

Previously, there was no support for generic methods in kernel. This prevented
us from being able to pass captured type arguments to the target top-level
function in converted closures, so these type arguments were always instantiated
to 'dynamic'.

Now, we save the type arguments to the closure creation operation in the
context, and read them out and forward them appropriately in closure wrapper
function. Since fasta doesn't currently support generic methods (their type
parameters are replaced by 'dynamic'), only top-level generic functions can
surface in kernel, as they are generated by closure conversion of closures that
capture type parameters of a class.

My focus here is enabling closure conversion to work in only these cases, and as
such, the code has some temporary "hacks" in the VM that may not work for
generic member functions or generic closures when they are enabled in fasta.

Test Plan:

I ran all the tests in closures/, and those which were previously expected to
crash due to missing VM support now pass and produce correct results.

Further testing is paused until we understand why the recent commit "[kernel]
Insert kernel bodies into VM heap" has broken all these tests.

Reviewers: regis@google.com, jensj@google.com, dmitryas@google.com

BUG=
R=dmitryas@google.com, jensj@google.com

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

[sjindel, 2017-08-16 13:24:32]

Committed patchset #6 (id:100001) manually as
6c5d1fba95de0e903eccbf6230f7f637f93eaf51 (presubmit successful).