First stab at mixins in VM compiler

First stab at mixins in VM compiler

Things not yet supported:
- Type parameters for super class and mixin class
- super call limitation check
Committed: https://code.google.com/p/dart/source/detail?r=18479

[kasperl, 2013-02-12 19:32:45]

If you have some (failing) tests for the field initialization across library
boundaries, it would be great if you check them in. 

Also, we should probably add some tests that make sure that we deal correctly
with private fields and methods that are mixed in across library boundaries.

(1) Mixing in a private method from another library shouldn't make it visible in
the library of the mixin application, but it should remain visible from the
other library.

(2) If you mixin methods that access private stuff on [this], such calls are
supposed to resolve in context of the mixed in method -- not the mixin
application.

There are probably more cases to test for than that :-)

[hausner, 2013-02-12 19:42:30]

Found a simple way to implement the super call limitation check.

[siva, 2013-02-13 01:34:37]

LGTM with one question about whether 'S with M,M' is an error?

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1259: ReportError(script, cls.token_pos(),
I am wondering if it would be more helpful if you also reported the script and
token position of the original mixin class here.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1273: ReportError(script, cls.token_pos(),
Ditto.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1283: cls.AddFunction(func);
Our AddFunction implementation is very inefficient, In this case it might make
sense to create a cloned_functions array (similar to cloned_fields below) and
add the implicit constructor which should probably be the only function in cls
to the list.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1298: fields = Array::MakeArray(cloned_fields);
Maybe we should assert here that
ASSERT(cls.fields() == Array::null());

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/object.h#newco...
runtime/vm/object.h:1203: RawClass* origin() const;
Our function naming style seems in consistent. I am wondering if origin and
script need to be Origin and Script (I am not suggesting you rename them in this
CL just noting that we are already inconsistent).

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3844: }
How do you handle 'S with M,N,M' or 'S with M,M' is it an error to repeat the
mixins?

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3851: String::Handle(String::New("&")));
Maybe add a Symbols::Ampersand() method similar to Symbols::Dot() etc. and use
that here instead of String::Handle(String::New("&"))

[hausner, 2013-02-13 19:58:14]

Slight change in the class finalizer after discussing the process with Regis.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1259: ReportError(script, cls.token_pos(),
On 2013/02/13 01:34:37, siva wrote:
> I am wondering if it would be more helpful if you also reported the script and
> token position of the original mixin class here.

I thought it makes more sense to report it where the class is used as a mixin
(i.e. at the keyword "with"). Ideally we should have a way to report two
locations. Since we can only report one location, the user may have to search
for one or the other related location.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1273: ReportError(script, cls.token_pos(),
On 2013/02/13 01:34:37, siva wrote:
> Ditto.
Ditto.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1283: cls.AddFunction(func);
I agree with what you suggest, but let me check it in this way so Mixins get
picked up by M3. I will make this more efficient with a follow-up changelist.

On 2013/02/13 01:34:37, siva wrote:
> Our AddFunction implementation is very inefficient, In this case it might make
> sense to create a cloned_functions array (similar to cloned_fields below) and
> add the implicit constructor which should probably be the only function in cls
> to the list.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1298: fields = Array::MakeArray(cloned_fields);
On 2013/02/13 01:34:37, siva wrote:
> Maybe we should assert here that
> ASSERT(cls.fields() == Array::null());

Done.

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/object.h#newco...
runtime/vm/object.h:1203: RawClass* origin() const;
I noticed that too. Technically, according to our coding standards, it would
have to be Origin() and Script() since the methods are more than simple
accessors. I never got that rule, since all the advantages of hiding the
implementation details of an accessor are lost if you have to change the
accessor name when the implementation changes. I prefer the lower case versions.

On 2013/02/13 01:34:37, siva wrote:
> Our function naming style seems in consistent. I am wondering if origin and
> script need to be Origin and Script (I am not suggesting you rename them in
this
> CL just noting that we are already inconsistent).

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3844: }
I discussed that with Gilad. He says it's perfectly legal to have a class in the
mixin chain more than once. The effect is that each time the class is mixed in
all its methods are overwritten again. It's basically a no-op that wastes
memory. OTOH, we explicitly forbid listing the same interface twice in the
implements clause... ;-)
On 2013/02/13 01:34:37, siva wrote:
> How do you handle 'S with M,N,M' or 'S with M,M' is it an error to repeat the
> mixins?

https://codereview.chromium.org/12210127/diff/11001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3851: String::Handle(String::New("&")));
I thought about this and didn't think this is used often enough where it makes
sense to pollute the symbol table. But .... Done. Looking at the symbol table
code I see that there is already a space for each one character string anyway.


On 2013/02/13 01:34:37, siva wrote:
> Maybe add a Symbols::Ampersand() method similar to Symbols::Dot() etc. and use
> that here instead of String::Handle(String::New("&"))