Preserve type variables in closure conversion.

Preserve type variables in closure conversion.

Summary:

Previously, we filled in all occurrences of captured type variables with either
"dynamic" or their bound, if they had one.

Now, we add extra type parameters to the top-level function corresponding to the
closure, and pass in the corresponding arguments as type arguments to the
"MakeClosure" operation.

Test Plan:

Updated [type_variables.dart] and added a new test case to it.

R=dmitryas@google.com

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

[sjindel, 2017-07-24 14:44:57]

sjindel@google.com changed reviewers:
+ dmitryas@google.com

[Dmitry Stefantsov, 2017-07-25 08:30:34]

Description was changed from

==========
Preserve type variables in closure conversion.

Summary:

Previously, we filled in all occurrences of captured type variables with either
"dynamic" or their bound, if they had one.

Now, we add extra type parameters to the top-level function corresponding to the
closure, and pass in the corresponding arguments as type arguments to the
"MakeClosure" operation.

Test Plan:

Updated [type_variables.dart] and added a new test case to it.
==========

to

==========
Preserve type variables in closure conversion.

Summary:

Previously, we filled in all occurrences of captured type variables with either
"dynamic" or their bound, if they had one.

Now, we add extra type parameters to the top-level function corresponding to the
closure, and pass in the corresponding arguments as type arguments to the
"MakeClosure" operation.

Test Plan:

Updated [type_variables.dart] and added a new test case to it.
==========

[Dmitry Stefantsov, 2017-07-25 08:30:34]

dmitryas@google.com changed reviewers:
+ karlklose@google.com

[Dmitry Stefantsov, 2017-07-25 08:32:24]

+Karl

Karl, PTAL.  I'll also leave my comments.

[Dmitry Stefantsov, 2017-07-25 09:13:34]

I have some comments.  Please, find them below.

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

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:267: /****
Please, remove the code if it's no longer needed.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:675: List<TypeParameter>
typeParams_out,
As I understand it, we don't use underscores in variable names.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:675: List<TypeParameter>
typeParams_out,
I think it's better to write a separate procedure to extract type parameters and
type arguments from a substitution map than to have output type parameters.  As
an alternative, you may just extract them at the place they're needed.  I think
there's only one such place.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures_type_vars/suite.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures_type_vars/suite.dart:11:
Future<ClosureConversionContext> createContext(
Why do we need a new test suite?

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/instance_tear_off.dart.expect (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/instance_tear_off.dart.expect:53: return
MakeClosure<<T extends core::Object>(self::closure#E#g#function::T) → dynamic,
self::E::T>(self::closure#E#g#function, null);
The first type parameter of MakeClosure (i.e. the function type of the closure)
here doesn't look right.  The type of the function parameter should refer to the
type parameter defined to the left of it, not to the type parameter of some
(although, related) top-level function.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures_type_vars/type_variables.dart.expect (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures_type_vars/type_variables.dart.expect:26: <A
extends core::Object>(self::fn::A) → core::List<self::fn::A> fn2 =
MakeClosure<<A extends core::Object>(self::closure#fn#function::A) →
core::List<self::closure#fn#function::A>,
self::fn::A>(self::closure#fn#function, null);
Same comment as at instance_tear_off.dart.expect applies here.

[karlklose, 2017-07-25 09:38:23]

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:744: List<DartType> typeArgs;
Please avoid abbreviations in variable names, rename to 'typeArguments' here and
below.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3101: contextVector?.accept(v);
I think this method should call visitList(typeArguments) (and also
topLevelFunctionReference.acceptReference(v) and functionType.accept(v)).

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

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:264: var newParams =
<TypeParameter>[], newArgs = <TypeParameterType>[];
Use one variable declaration per variable.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:326: TreeNode
visitFunctionExpression(FunctionExpression node) =>
I prefer using {} and a return statement instead of '=>' if the expression does
not fit on the same line.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:669: /// [original] with
their copies. Additionally returns a list of new type
If you keep the "output parameters", please change this comment to say that the
method will append the new type parameters and type arguments to the lists given
as arguments.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:34: ///
Remove empty comment line.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:172:
capturedTypeVariables.where((t) => t.parent != currentFunction));
Consider adding a comment here to explain when 't.parent == currentFunction'
(also below).

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures_type_vars/closures_type_vars.status (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures_type_vars/closures_type_vars.status:1: # Copyright (c)
2016, the Dart project authors. Please see the AUTHORS file
2016 -> 2017

[sjindel, 2017-07-25 13:11:48]

All prior comment threads should be resolved, except for the one started by Karl
about visitChildren.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:744: List<DartType> typeArgs;
On 2017/07/25 09:38:23, karlklose wrote:
> Please avoid abbreviations in variable names, rename to 'typeArguments' here
and
> below.

Done.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3101: contextVector?.accept(v);
On 2017/07/25 09:38:23, karlklose wrote:
> I think this method should call visitList(typeArguments) (and also
> topLevelFunctionReference.acceptReference(v) and functionType.accept(v)).

I added visitList and functionType ones. But why the acceptReference one? It's
unclear to me where we should visit weak references and where we should not. For
example, the 'PropertyGet' class doesn't visit it's reference to an interface
target.

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

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:264: var newParams =
<TypeParameter>[], newArgs = <TypeParameterType>[];
On 2017/07/25 09:38:23, karlklose wrote:
> Use one variable declaration per variable.

Code removed.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:267: /****
On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> Please, remove the code if it's no longer needed.

Done.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:326: TreeNode
visitFunctionExpression(FunctionExpression node) =>
On 2017/07/25 09:38:23, karlklose wrote:
> I prefer using {} and a return statement instead of '=>' if the expression
does
> not fit on the same line.

Done.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:669: /// [original] with
their copies. Additionally returns a list of new type
On 2017/07/25 09:38:23, karlklose wrote:
> If you keep the "output parameters", please change this comment to say that
the
> method will append the new type parameters and type arguments to the lists
given
> as arguments.

Removed code.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:675: List<TypeParameter>
typeParams_out,
On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> As I understand it, we don't use underscores in variable names.

Done.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:675: List<TypeParameter>
typeParams_out,
On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> I think it's better to write a separate procedure to extract type parameters
and
> type arguments from a substitution map than to have output type parameters. 
As
> an alternative, you may just extract them at the place they're needed.  I
think
> there's only one such place.

Done.
There needed in two places, and the issue with this is it requires downcasting
the values in the map.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:34: ///
On 2017/07/25 09:38:23, karlklose wrote:
> Remove empty comment line.

Done.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:172:
capturedTypeVariables.where((t) => t.parent != currentFunction));
On 2017/07/25 09:38:23, karlklose wrote:
> Consider adding a comment here to explain when 't.parent == currentFunction'
> (also below).

Done.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures_type_vars/closures_type_vars.status (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures_type_vars/closures_type_vars.status:1: # Copyright (c)
2016, the Dart project authors. Please see the AUTHORS file
On 2017/07/25 09:38:23, karlklose wrote:
> 2016 -> 2017

Done.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures_type_vars/suite.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures_type_vars/suite.dart:11:
Future<ClosureConversionContext> createContext(
On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> Why do we need a new test suite?

This one doesn't run the erasure transform. Not running it on the other tests
would cause a lot of expected file updates.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/instance_tear_off.dart.expect (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/instance_tear_off.dart.expect:53: return
MakeClosure<<T extends core::Object>(self::closure#E#g#function::T) → dynamic,
self::E::T>(self::closure#E#g#function, null);
On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> The first type parameter of MakeClosure (i.e. the function type of the
closure)
> here doesn't look right.  The type of the function parameter should refer to
the
> type parameter defined to the left of it, not to the type parameter of some
> (although, related) top-level function.

It does actually refer to the parameter on the left, because they are the same
pointer. It's just the textual printing that's confusing here, because the same
type parameter pointer is reused below for the top-level function definition.
This is problematic because it violates the tree property though, so I've fixed
it by creating new parameters for the closure's type.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures_type_vars/type_variables.dart.expect (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures_type_vars/type_variables.dart.expect:26: <A
extends core::Object>(self::fn::A) → core::List<self::fn::A> fn2 =
MakeClosure<<A extends core::Object>(self::closure#fn#function::A) →
core::List<self::closure#fn#function::A>,
self::fn::A>(self::closure#fn#function, null);
On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> Same comment as at instance_tear_off.dart.expect applies here.

Done.

[Dmitry Stefantsov, 2017-07-26 09:12:10]

LGTM if the question with visiting the reference is resolved.

I also have one question in the comments below.

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

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:675: List<TypeParameter>
typeParams_out,
On 2017/07/25 13:11:48, sjindel wrote:
> On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> > I think it's better to write a separate procedure to extract type parameters
> and
> > type arguments from a substitution map than to have output type parameters. 
> As
> > an alternative, you may just extract them at the place they're needed.  I
> think
> > there's only one such place.
> 
> Done.
> There needed in two places, and the issue with this is it requires downcasting
> the values in the map.

Thanks for the explanation.  Tuples would look nice in this situation, I guess
:)

Originally, I was confused that some values are returned, and some are put into
empty data structs passed as arguments.  It also brought those "_out" suffixes
into existence.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:172:
capturedTypeVariables.where((t) => t.parent != currentFunction));
On 2017/07/25 13:11:48, sjindel wrote:
> On 2017/07/25 09:38:23, karlklose wrote:
> > Consider adding a comment here to explain when 't.parent == currentFunction'
> > (also below).
> 
> Done.

Karl may correct me, but I think using square brackets in comments in situations
like this won't produce the intended result.  I think, only identifiers can be
put into square brackets to be later turned into hyperlinks in the extracted
documentation.  I use backticks for fragments of code like this one.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures_type_vars/suite.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures_type_vars/suite.dart:11:
Future<ClosureConversionContext> createContext(
On 2017/07/25 13:11:48, sjindel wrote:
> On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> > Why do we need a new test suite?
> 
> This one doesn't run the erasure transform. Not running it on the other tests
> would cause a lot of expected file updates.

My understanding is that in future Erasure will be erased from [VmFastaTarget].

Do you think it is a good idea to turn [strongMode] on for all tests?  In this
case, we may reduce the number of test suites :)  It is ok to do that in a
separate CL.

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/instance_tear_off.dart.expect (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/instance_tear_off.dart.expect:53: return
MakeClosure<<T extends core::Object>(self::closure#E#g#function::T) → dynamic,
self::E::T>(self::closure#E#g#function, null);
On 2017/07/25 13:11:48, sjindel wrote:
> On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> > The first type parameter of MakeClosure (i.e. the function type of the
> closure)
> > here doesn't look right.  The type of the function parameter should refer to
> the
> > type parameter defined to the left of it, not to the type parameter of some
> > (although, related) top-level function.
> 
> It does actually refer to the parameter on the left, because they are the same
> pointer. It's just the textual printing that's confusing here, because the
same
> type parameter pointer is reused below for the top-level function definition.
> This is problematic because it violates the tree property though, so I've
fixed
> it by creating new parameters for the closure's type.

Thank you for the explanation!

[karlklose, 2017-07-26 10:36:30]

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:172:
capturedTypeVariables.where((t) => t.parent != currentFunction));
On 2017/07/26 09:12:09, Dmitry Stefantsov wrote:
> On 2017/07/25 13:11:48, sjindel wrote:
> > On 2017/07/25 09:38:23, karlklose wrote:
> > > Consider adding a comment here to explain when 't.parent ==
currentFunction'
> > > (also below).
> > 
> > Done.
> 
> Karl may correct me, but I think using square brackets in comments in
situations
> like this won't produce the intended result.  I think, only identifiers can be
> put into square brackets to be later turned into hyperlinks in the extracted
> documentation.  I use backticks for fragments of code like this one.

Yes, you should use `` for code fragments. Note that they will not be analyzed
by tools when they appear in normal (//) comments, but we still use them for
uniformity.

[sjindel, 2017-07-26 10:37:45]

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures_type_vars/suite.dart (right):

https://codereview.chromium.org/2989563002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures_type_vars/suite.dart:11:
Future<ClosureConversionContext> createContext(
On 2017/07/26 09:12:09, Dmitry Stefantsov wrote:
> On 2017/07/25 13:11:48, sjindel wrote:
> > On 2017/07/25 09:13:34, Dmitry Stefantsov wrote:
> > > Why do we need a new test suite?
> > 
> > This one doesn't run the erasure transform. Not running it on the other
tests
> > would cause a lot of expected file updates.
> 
> My understanding is that in future Erasure will be erased from
[VmFastaTarget].
> 
> Do you think it is a good idea to turn [strongMode] on for all tests?  In this
> case, we may reduce the number of test suites :)  It is ok to do that in a
> separate CL.

I'm indifferent.

[sjindel, 2017-07-26 10:43:09]

Description was changed from

==========
Preserve type variables in closure conversion.

Summary:

Previously, we filled in all occurrences of captured type variables with either
"dynamic" or their bound, if they had one.

Now, we add extra type parameters to the top-level function corresponding to the
closure, and pass in the corresponding arguments as type arguments to the
"MakeClosure" operation.

Test Plan:

Updated [type_variables.dart] and added a new test case to it.
==========

to

==========
Preserve type variables in closure conversion.

Summary:

Previously, we filled in all occurrences of captured type variables with either
"dynamic" or their bound, if they had one.

Now, we add extra type parameters to the top-level function corresponding to the
closure, and pass in the corresponding arguments as type arguments to the
"MakeClosure" operation.

Test Plan:

Updated [type_variables.dart] and added a new test case to it.

R=dmitryas@google.com

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

[sjindel, 2017-07-26 10:43:11]

Committed patchset #4 (id:60001) manually as
4d7490c60959036b28ddd2b151a8247795261f97 (presubmit successful).

[sjindel, 2017-07-26 11:28:58]

I had to add several RuntimeError lines to the status files.