Handle forwarding constructors to unnamed mixin applications

Handle forwarding constructors to unnamed mixin applications

+ handle instance fields from mixins
+ change class sorting to put unnamed mixin applications last

R=sigmund@google.com

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

[Johnni Winther, 2017-08-30 07:36:48]

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

[Siggi Cherem (dart-lang), 2017-08-30 19:33:53]

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart (right):

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:247:
elementMap.elementEnvironment.forEachMixin(cls, (ClassEntity c) {
maybe it is worth having a method to just fetch the first mixin?

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:248: if (c != null)
return;
seems like c/mixin are swapped here and below:

  if (mixin != null) return;
  mixin = c;

Or maybe just do:
  mixin ??= c

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:269: return
compareClasses(getMixin(a), getMixin(b));
should this be recursive instead?

e.g: compare A&B&C vs A&B&D ?

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (left):

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:631:
fieldValues.addAll(_collectFieldValues(cls));
Is this not needed anymore?

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:479: // Gotta update the
resolvedAst when we're looking at field values
nit: resolvedAst => current entity? (I know it was from before)

[Johnni Winther, 2017-09-01 15:17:45]

ptal

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart (right):

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:269: return
compareClasses(getMixin(a), getMixin(b));
On 2017/08/30 19:33:52, Siggi Cherem (dart-lang) wrote:
> should this be recursive instead?
> 
> e.g: compare A&B&C vs A&B&D ?

Remove the need for this. If we sort first by library their names are unique.

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (left):

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:631:
fieldValues.addAll(_collectFieldValues(cls));
On 2017/08/30 19:33:53, Siggi Cherem (dart-lang) wrote:
> Is this not needed anymore?

[_collectFieldValues] handles fields in all superclasses, so this is no longer
needed.

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/3005823002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:479: // Gotta update the
resolvedAst when we're looking at field values
On 2017/08/30 19:33:53, Siggi Cherem (dart-lang) wrote:
> nit: resolvedAst => current entity? (I know it was from before)

Done.

[Siggi Cherem (dart-lang), 2017-09-01 15:41:49]

lgtm

https://codereview.chromium.org/3005823002/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart (right):

https://codereview.chromium.org/3005823002/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:262: failedAt(a,
"Multiple mixins named ${a.name}: $a vs $b."));
do we ensure today that if you do A extends Object with B and C extends Object
with B in the same file, then there is only one Object&B?

[Johnni Winther, 2017-09-01 16:13:48]

https://codereview.chromium.org/3005823002/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart (right):

https://codereview.chromium.org/3005823002/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:262: failedAt(a,
"Multiple mixins named ${a.name}: $a vs $b."));
On 2017/09/01 15:41:49, Siggi Cherem (dart-lang) wrote:
> do we ensure today that if you do A extends Object with B and C extends Object
> with B in the same file, then there is only one Object&B?

We do in the optimized mixin algorithm (ported from fasta), otherwise not.

[Johnni Winther, 2017-09-01 17:07:10]

Description was changed from

==========
Handle forwarding constructors to unnamed mixin applications

+ handle instance fields from mixins
+ change class sorting to put unnamed mixin applications last
==========

to

==========
Handle forwarding constructors to unnamed mixin applications

+ handle instance fields from mixins
+ change class sorting to put unnamed mixin applications last

R=sigmund@google.com

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

[Johnni Winther, 2017-09-01 17:07:14]

Committed patchset #2 (id:20001) manually as
374df045e70a1b473d861c09f6c8fa78a9cd0e39 (presubmit successful).