Implement type variables in mixin applications.

Implement type variables in mixin applications.

R=karlklose@google.com

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

[ahe, 2017-02-14 17:38:16]

ahe@google.com changed reviewers:
+ sigmund@google.com

[ahe, 2017-02-14 17:44:45]

Hang on. Something is wrong with this CL.

I'll send a message when it's fixed.

Two problems:

1. I accidentally reverted code for checking duplicated definitions.

2. A number of constructors aren't found.

[ahe, 2017-02-14 18:12:36]

Thanks for waiting, this CL is ready for review now.

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/test/fast...
File pkg/front_end/test/fasta/mixin.dart.direct.expect (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/test/fast...
pkg/front_end/test/fasta/mixin.dart.direct.expect:7: : super
self::Object&M1&M2::•()
This is invalid-initializer if I don't set mixedInType, but C isn't affected.

[ahe, 2017-02-15 08:02:59]

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

[ahe, 2017-02-15 08:03:00]

Karl, could you take a look?

[karlklose, 2017-02-15 09:09:30]

LGTM

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_class_builder.dart (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_class_builder.dart:59: bool get
isMixinApplication => cls.isMixinApplication;
Should this be `isMixinApplicationBuilder`?

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_mixin_application_builder.dart
(right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_mixin_application_builder.dart:50:
String subclassName;
Could you move (or copy) this documentation to `MixinApplicationBuilder`? And
maybe add an example on how this name is contruscted here.

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_mixin_application_builder.dart:86:
String name = subclassName != null
Could you add a comment on when these cases happen?

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:372: if
(builder.isMixinApplication || !builder.constructors.isEmpty) return;
Old code, but how about .isNotEmpty?

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:252: if (existing
is ClassBuilder && other is ClassBuilder) {
Please add a comment about what invariance this has, that is, that multiple
mixin-application with the same 'super-chain' are allowed.

[ahe, 2017-02-15 09:36:51]

Thank you, Karl!

One open question below that we can potentially address in a follow-up CL.

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_class_builder.dart (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_class_builder.dart:59: bool get
isMixinApplication => cls.isMixinApplication;
On 2017/02/15 09:09:30, karlklose wrote:
> Should this be `isMixinApplicationBuilder`?

Generally, I haven't used the Builder prefix in bool properties, for example,
see Builder. But what's your thinking behind this question?

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_mixin_application_builder.dart
(right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_mixin_application_builder.dart:50:
String subclassName;
On 2017/02/15 09:09:30, karlklose wrote:
> Could you move (or copy) this documentation to `MixinApplicationBuilder`? And
> maybe add an example on how this name is contruscted here.

Done.

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_mixin_application_builder.dart:86:
String name = subclassName != null
On 2017/02/15 09:09:30, karlklose wrote:
> Could you add a comment on when these cases happen?

Done.

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:372: if
(builder.isMixinApplication || !builder.constructors.isEmpty) return;
On 2017/02/15 09:09:30, karlklose wrote:
> Old code, but how about .isNotEmpty?

Done.

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:252: if (existing
is ClassBuilder && other is ClassBuilder) {
On 2017/02/15 09:09:30, karlklose wrote:
> Please add a comment about what invariance this has, that is, that multiple
> mixin-application with the same 'super-chain' are allowed.

Done.

[ahe, 2017-02-15 09:38:54]

Description was changed from

==========
Implement type variables in mixin applications.
==========

to

==========
Implement type variables in mixin applications.

R=karlklose@google.com

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

[ahe, 2017-02-15 09:38:55]

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

[karlklose, 2017-02-15 09:43:57]

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_class_builder.dart (right):

https://codereview.chromium.org/2689303003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_class_builder.dart:59: bool get
isMixinApplication => cls.isMixinApplication;
On 2017/02/15 09:36:50, ahe wrote:
> On 2017/02/15 09:09:30, karlklose wrote:
> > Should this be `isMixinApplicationBuilder`?
> 
> Generally, I haven't used the Builder prefix in bool properties, for example,
> see Builder. But what's your thinking behind this question?

It made sense to me to have a prefix here to make it explicit that this is a
builder and not the thing it builds and I wasn't sure if there already is a
convention. If you did not use a prefix elsewhere, keep it as is.