Support mixins when loading from .dill

Support mixins when loading from .dill

R=sigmund@google.com

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

[Johnni Winther, 2017-05-16 14:45:27]

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

[Siggi Cherem (dart-lang), 2017-05-17 16:50:32]

lgtm, with questions comments - would it be simpler if we transform the IR
enough to avoid this special case for `getSuperConstructor`?

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

https://codereview.chromium.org/2884273002/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map.dart:57: /// in [constructor] targeting
[target].
nit: consider rephrasing this a bit and include below a bit more details on what
the result is (in addition to what the "target" is).

In particular, I was a bit confused by "targeting [target]": IIUC, the result
matches [target] when [target] corresponds to the immediate superclass, but in
dart2js we need the entity for the mixin application instead. Correct?

Here is a possible tweak/suggestion:
 
Returns the [ConstructorEntity] corresponding to a super initializer in
[constructor].

The IR resolves super initializers to a [target] up in the type hierarchy. Most
of the time, the result of this function will be the entity corresponding to
that target. In the presence of unnamed mixins, this function returns an entity
for an intermediate synthetic constructor that kernel doesn't explicitly
represent. 

For example:
     class M {}
     class C extends Object with M {}

Kernel will say that C()'s super initializer resolves to Object(), but this
function will return an entity representing the unnamed mixin application
"Object+M"'s constructor.

https://codereview.chromium.org/2884273002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_impact.dart (right):

https://codereview.chromium.org/2884273002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_impact.dart:306:
elementAdapter.getSuperConstructor(node.parent, node.target);
is node.target here the one that was read from the dill file before adding the
synthetic super constructor? Should we instead rewrite node.target to point to
the right target, then getSuperConstructor becomes getConstructor(target)?

[Johnni Winther, 2017-05-18 08:57:03]

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

https://codereview.chromium.org/2884273002/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map.dart:57: /// in [constructor] targeting
[target].
On 2017/05/17 16:50:32, Siggi Cherem (dart-lang) wrote:
> nit: consider rephrasing this a bit and include below a bit more details on
what
> the result is (in addition to what the "target" is).
> 
> In particular, I was a bit confused by "targeting [target]": IIUC, the result
> matches [target] when [target] corresponds to the immediate superclass, but in
> dart2js we need the entity for the mixin application instead. Correct?
> 
> Here is a possible tweak/suggestion:
>  
> Returns the [ConstructorEntity] corresponding to a super initializer in
> [constructor].
> 
> The IR resolves super initializers to a [target] up in the type hierarchy.
Most
> of the time, the result of this function will be the entity corresponding to
> that target. In the presence of unnamed mixins, this function returns an
entity
> for an intermediate synthetic constructor that kernel doesn't explicitly
> represent. 
> 
> For example:
>      class M {}
>      class C extends Object with M {}
> 
> Kernel will say that C()'s super initializer resolves to Object(), but this
> function will return an entity representing the unnamed mixin application
> "Object+M"'s constructor.

Done.

https://codereview.chromium.org/2884273002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_impact.dart (right):

https://codereview.chromium.org/2884273002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_impact.dart:306:
elementAdapter.getSuperConstructor(node.parent, node.target);
On 2017/05/17 16:50:32, Siggi Cherem (dart-lang) wrote:
> is node.target here the one that was read from the dill file before adding the
> synthetic super constructor? Should we instead rewrite node.target to point to
> the right target, then getSuperConstructor becomes getConstructor(target)?

Adding a TODO. I have so far been reluctant to put rewrite logic into
kernel_impact, but we might consider it the required pre-step for inference and
ssa-building.

[Johnni Winther, 2017-05-18 09:04:18]

Description was changed from

==========
Support mixins when loading from .dill
==========

to

==========
Support mixins when loading from .dill

R=sigmund@google.com

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

[Johnni Winther, 2017-05-18 09:04:20]

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