dart2js: Support mixins in the new emitter.

dart2js: Support mixins in the new emitter.

R=kasperl@google.com, sigurdm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42017

[floitsch, 2014-11-26 17:32:08]

floitsch@google.com changed reviewers:
+ kasperl@google.com, sigurdm@google.com

[floitsch, 2014-11-26 17:32:09]

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:463: //
$mixinFormatComment.
I'm using this "marker" as a way to easily find all references of the
mixin-format, and to find the format-specification from its uses.

[kasperl, 2014-11-27 09:05:14]

LGTM.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/model.dart:155: this._superclass = superclass;
Remove this.?

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/model.dart:178: this._mixinClass = mixinClass;
Remove this.?

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:265: assert(() {
Add a comment for the assert. What are you verifying? It might be easier to
diagnose assertion failures if you split these out as separate asserts, but I
understand that you prefer not to have the mixinApplication variable introduced
outside the asserts. Maybe using an 'as' cast in an assert would be okay?

[sigurdm, 2014-11-27 09:25:55]

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/model.dart:177: void setMixinClass(Class
mixinClass) {
Why not a setter?

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:342:
elements.addAll(allMethods.expand((e) => [ js.string(e.name), e.code ]));
Space inside the '['

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:347: "Mixins have
no constructor, but a reference to their mixin class.";
Empty line between members.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:349: List
elements = [ js.string(cls.superclassName),
Space inside the '['

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:463: //
$mixinFormatComment.
On 2014/11/26 17:32:09, floitsch wrote:
> I'm using this "marker" as a way to easily find all references of the
> mixin-format, and to find the format-specification from its uses.

Perhaps also explain that in a comment in the code.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:464: if (typeof
descriptor[2] !== 'function') {
Could there be a less fragile way of distinguishing mixins from other classes?

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/program_builder.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/program_builder.dart:267: if
(!element.isMixinApplication) {
What about elements that are not instantiated for other reasons than being a
mixin application?

[sigurdm, 2014-11-27 09:25:55]

lgtm

[floitsch, 2014-11-27 12:35:16]

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/model.dart:155: this._superclass = superclass;
On 2014/11/27 09:05:14, kasperl wrote:
> Remove this.?

Done.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/model.dart:177: void setMixinClass(Class
mixinClass) {
On 2014/11/27 09:25:54, sigurdm wrote:
> Why not a setter?

Because we don't want it to feel like a field.
There is a way to set the field, but it is a one-time operation in a specific
place. For all other purposes it is a final field.
If we made it a setter, the intent wouldn't be that clear.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/model.dart:178: this._mixinClass = mixinClass;
On 2014/11/27 09:05:13, kasperl wrote:
> Remove this.?

Done.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:265: assert(() {
On 2014/11/27 09:05:14, kasperl wrote:
> Add a comment for the assert. What are you verifying? It might be easier to
> diagnose assertion failures if you split these out as separate asserts, but I
> understand that you prefer not to have the mixinApplication variable
introduced
> outside the asserts. Maybe using an 'as' cast in an assert would be okay?

Reduced the assert.
It is now just testing that mixins don't introduce new fields.
The fields and methods are empty by construction (in the constructor), so it is
redundant to check it here. And I'm not too afraid that refactorings would
change that.

done.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:342:
elements.addAll(allMethods.expand((e) => [ js.string(e.name), e.code ]));
On 2014/11/27 09:25:54, sigurdm wrote:
> Space inside the '['

Done.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:347: "Mixins have
no constructor, but a reference to their mixin class.";
On 2014/11/27 09:25:54, sigurdm wrote:
> Empty line between members.

Done.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:349: List
elements = [ js.string(cls.superclassName),
On 2014/11/27 09:25:54, sigurdm wrote:
> Space inside the '['

Done.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:463: //
$mixinFormatComment.
On 2014/11/27 09:25:54, sigurdm wrote:
> On 2014/11/26 17:32:09, floitsch wrote:
> > I'm using this "marker" as a way to easily find all references of the
> > mixin-format, and to find the format-specification from its uses.
> 
> Perhaps also explain that in a comment in the code.

Done.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:464: if (typeof
descriptor[2] !== 'function') {
On 2014/11/27 09:25:54, sigurdm wrote:
> Could there be a less fragile way of distinguishing mixins from other classes?

I don't see any, that wouldn't require bigger changes, or that would waste
space.

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/program_builder.dart (right):

https://codereview.chromium.org/752553004/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/program_builder.dart:267: if
(!element.isMixinApplication) {
On 2014/11/27 09:25:54, sigurdm wrote:
> What about elements that are not instantiated for other reasons than being a
> mixin application?

The only ones that I can think of are onlyForRti and I will look at them when
doing RTI.

[floitsch, 2014-11-27 12:57:55]

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