Pass type arguments as a list in generic methods invocations

Pass type arguments as a list in generic methods invocations

All generic methods are equipped with one extra named parameter for
passing type arguments as a list of type values.

Additinally, this change forces strong mode usage for 'dartk' in
'reified_dart'. Strong mode is required for loader to not strip away
type arguments from generic methods. In future, a command line argument
may be implemented for that (e.g. --generic-methods), if generic method
support will land before the strong mode.

R=karlklose@google.com

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

[Dmitry Stefantsov, 2017-02-24 11:48:06]

Description was changed from

==========
Pass type arguments as a list in generic methods invocations

All generic methods are equipped with one extra named parameter for
passing type arguments as a list of type values.

Additinally, this change forces strong mode usage for 'dartk' in
'reified_dart'. Strong mode is required for loader to not strip away
type arguments from generic methods. In future, a command line argument
may be implemented for that (e.g. --generic-methods), if generic method
support will land before the strong mode.
==========

to

==========
Pass type arguments as a list in generic methods invocations

All generic methods are equipped with one extra named parameter for
passing type arguments as a list of type values.

Additinally, this change forces strong mode usage for 'dartk' in
'reified_dart'. Strong mode is required for loader to not strip away
type arguments from generic methods. In future, a command line argument
may be implemented for that (e.g. --generic-methods), if generic method
support will land before the strong mode.
==========

[Dmitry Stefantsov, 2017-02-24 11:48:07]

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

[karlklose, 2017-02-24 12:07:24]

Could you add a 'golden' test?

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/target/vmrei...
File pkg/kernel/lib/target/vmreify.dart (right):

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/target/vmrei...
pkg/kernel/lib/target/vmreify.dart:44: // Here we disable Erasure pass of
VmTarget class. The information deleted by
If you write '[Erasure]' and '[VmTarget]' instead of 'Erase' and 'VmTarget',
tools treat them as names declared in the program.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/reify/transformation/remove_generics.dart
(right):

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/remove_generics.dart:48: // 
  Procedure target = node.interfaceTarget;
Why is this commented out?

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/reify/transformation/transformer.dart
(right):

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:106: String
get genericMethodTypeParametersName => "\$typeParameters";
This could also be a constant.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:591:
ListLiteral genericMethodTypeParameters = new ListLiteral(new List.from(
You could also call '.toList(growable: false)' on the result of map instead of
using the 'List.from' constructor.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:593:
.map((DartType type) => createRuntimeType(type))),
How about
  .map(createRuntimeType)
?

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:596:
genericMethodTypeParametersName, genericMethodTypeParameters)..parent =
expression.arguments);
Long line, break before '..'. You could also run dartmt (either from command
line or inside the IDE) to avoid having these problems.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:603: type:
new InterfaceType(
Not sure we need this type, but feel free to keep it.

[Dmitry Stefantsov, 2017-02-28 11:32:16]

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/target/vmrei...
File pkg/kernel/lib/target/vmreify.dart (right):

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/target/vmrei...
pkg/kernel/lib/target/vmreify.dart:44: // Here we disable Erasure pass of
VmTarget class. The information deleted by
On 2017/02/24 12:07:23, karlklose wrote:
> If you write '[Erasure]' and '[VmTarget]' instead of 'Erase' and 'VmTarget',
> tools treat them as names declared in the program.

Done.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/reify/transformation/remove_generics.dart
(right):

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/remove_generics.dart:48: // 
  Procedure target = node.interfaceTarget;
On 2017/02/24 12:07:23, karlklose wrote:
> Why is this commented out?

I should have removed that. The idea is actually to keep type parameters and
pass them to VM, so that it has some information about types.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/reify/transformation/transformer.dart
(right):

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:106: String
get genericMethodTypeParametersName => "\$typeParameters";
On 2017/02/24 12:07:23, karlklose wrote:
> This could also be a constant.

Done.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:591:
ListLiteral genericMethodTypeParameters = new ListLiteral(new List.from(
On 2017/02/24 12:07:23, karlklose wrote:
> You could also call '.toList(growable: false)' on the result of map instead of
> using the 'List.from' constructor.

Done.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:593:
.map((DartType type) => createRuntimeType(type))),
On 2017/02/24 12:07:23, karlklose wrote:
> How about
>   .map(createRuntimeType)
> ?

Sure! :)

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:596:
genericMethodTypeParametersName, genericMethodTypeParameters)..parent =
expression.arguments);
On 2017/02/24 12:07:23, karlklose wrote:
> Long line, break before '..'. You could also run dartmt (either from command
> line or inside the IDE) to avoid having these problems.

Done.

https://codereview.chromium.org/2713163002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:603: type:
new InterfaceType(
On 2017/02/24 12:07:23, karlklose wrote:
> Not sure we need this type, but feel free to keep it.

OK

[Dmitry Stefantsov, 2017-03-15 13:41:15]

dmitryas@google.com changed reviewers:
- kmillikin@google.com

[Dmitry Stefantsov, 2017-03-15 13:41:16]

PTAL. Currently, 'reify' transformation works with latest updates from 'master',
and the golden tests, that were inherited from 'reify' repository, pass.

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

https://codereview.chromium.org/2713163002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:742: returnType:
substitute(function.returnType, cloner.typeSubstitution),
This line fixes a bug in closure conversion. Previously, if a generic method was
torn off from a non-generic class instance, the new type information retrieved
from the method was lost and wasn't used to adjust the return type of the
method.

[karlklose, 2017-03-15 15:20:22]

LGTM

https://codereview.chromium.org/2713163002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/reify/transformation/transformer.dart
(right):

https://codereview.chromium.org/2713163002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:590: if
(node.parent is Constructor) {
If you stored `node.parent` in a local variable, type promotion would make the
cast (`as`) unnecessary.

Maybe:

if (node.parent is Member) {
  Member member = node.parent;
  if (member is Constructor ||
      member is Procedure && member.kind == ProcedureKind.Factory) {
    return member.enclosingClass.typeParameters <
         node.typeParameters.length;
  }
} else {
  return node.typeParameters.isNotEmtpy;
}

[Dmitry Stefantsov, 2017-03-16 09:30:42]

https://codereview.chromium.org/2713163002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/reify/transformation/transformer.dart
(right):

https://codereview.chromium.org/2713163002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/reify/transformation/transformer.dart:590: if
(node.parent is Constructor) {
On 2017/03/15 15:20:22, karlklose wrote:
> If you stored `node.parent` in a local variable, type promotion would make the
> cast (`as`) unnecessary.
> 
> Maybe:
> 
> if (node.parent is Member) {
>   Member member = node.parent;
>   if (member is Constructor ||
>       member is Procedure && member.kind == ProcedureKind.Factory) {
>     return member.enclosingClass.typeParameters <
>          node.typeParameters.length;
>   }
> } else {
>   return node.typeParameters.isNotEmtpy;
> }

Yes, I agree with a small remark. The if-else operator should be transformed
into single-branched if-operator, and the return statement from the 'else'
branch should be put on its own line after the single-branched if-operator.
Otherwise, the case of a non-constructor and non-factory member is not covered.

[Dmitry Stefantsov, 2017-03-16 09:47:18]

Description was changed from

==========
Pass type arguments as a list in generic methods invocations

All generic methods are equipped with one extra named parameter for
passing type arguments as a list of type values.

Additinally, this change forces strong mode usage for 'dartk' in
'reified_dart'. Strong mode is required for loader to not strip away
type arguments from generic methods. In future, a command line argument
may be implemented for that (e.g. --generic-methods), if generic method
support will land before the strong mode.
==========

to

==========
Pass type arguments as a list in generic methods invocations

All generic methods are equipped with one extra named parameter for
passing type arguments as a list of type values.

Additinally, this change forces strong mode usage for 'dartk' in
'reified_dart'. Strong mode is required for loader to not strip away
type arguments from generic methods. In future, a command line argument
may be implemented for that (e.g. --generic-methods), if generic method
support will land before the strong mode.

R=karlklose@google.com

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

[Dmitry Stefantsov, 2017-03-16 09:47:20]

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