Mixins with Generics

Mixins with Generics

This change adds support for generics to mixins. It's not a particularly elegant
implementation, so I expect to change how it works internally in later checkins.

Also, there is still one aspect of the implementation that is incorrect. In the
case of typedef, the newly introduced name is not yet an alias for  the
mixin application. Instead, the MA is a superclass of the typedef name. That
will need to be fixed in a later checkin. I don't want to make this change
bigger.
 
Committed: https://code.google.com/p/dart/source/detail?r=20084

[hausner, 2013-03-14 21:51:45]

Can you please point out where I am adding too many 'dynamic' entries in the
type arguments? Thank you!

[regis, 2013-03-14 22:11:06]

On 2013/03/14 21:51:45, hausner wrote:
> Can you please point out where I am adding too many 'dynamic' entries in the
> type arguments? Thank you!

I replied to your separate email.
Please, let me know when you have uploaded the fix.

[hausner, 2013-03-14 22:49:31]

PTAL, thank you.

[regis, 2013-03-14 23:40:34]

LGTM for a first shot.

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

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1189: // refer to the respecive type parameters of
the mixin application
respective

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1195:
AbstractType::Handle(mixapp_class.super_type());
I would assert that the super type is not a MixinAppType, or the code of this
function will not do the right thing.
If it 'was' one (because of multiple mixins), it should have been resolved by
now and replaced by a Type.
You can simply assert that the super type is resolved, since a MixinAppType is
not resolved by definition.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1247: ASSERT(super_type.IsType());
This may fail if the super type is a BoundedType. But it depends on how you are
going to handle bounds.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1538: void ClassFinalizer::CopyArgs(const Class&
cls,
I would pick more meaningful names. What args are you copying? From where to
where? Actually, it is not just a copy operation. Maybe something like
ExtractTypeArguments or
void CollectTypeArguments(cls, type, collected_type_args)?

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1578: ASSERT(type.IsType());
Not necessarily. It could be a BoundedType. Add a TODO for now, and you will
need lots of tests.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1602: type ^= mixins.At(mixins.Length() - 1);
Can you explain in a comment that the last type in the array represents the raw
type of the mixin app class?
Would it be cleaner to add a separate field of type Class rather than stuffing a
raw Type in the mixins array for this?

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.h:103: 
No blank line elsewhere in this private section.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.h:106: 
... except here.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9898: bool MixinAppType::IsResolved() const {
I do not think this call is necessary. By definition, a MixinAppType is not
resolved.

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

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.h#newco...
runtime/vm/object.h:3755: virtual bool IsMalformed() const { return false; }
That is optimistic :-)
I would actually leave this call out, and rely on the AbstractType version to
bomb if this is called. We should not call it, since we will never encounter a
MixinAppType in a type test.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.h#newco...
runtime/vm/object.h:3756: virtual bool IsResolved() const;
I would unconditionally return false.

[hausner, 2013-03-15 00:36:32]

Thanks for the in-depth review. I will check this in tomorrow morning.

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

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1189: // refer to the respecive type parameters of
the mixin application
On 2013/03/14 23:40:34, regis wrote:
> respective

Done.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1195:
AbstractType::Handle(mixapp_class.super_type());
On 2013/03/14 23:40:34, regis wrote:
> I would assert that the super type is not a MixinAppType, or the code of this
> function will not do the right thing.
> If it 'was' one (because of multiple mixins), it should have been resolved by
> now and replaced by a Type.
> You can simply assert that the super type is resolved, since a MixinAppType is
> not resolved by definition.

Done.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1247: ASSERT(super_type.IsType());
On 2013/03/14 23:40:34, regis wrote:
> This may fail if the super type is a BoundedType. But it depends on how you
are
> going to handle bounds.
Added a todo that I need to handle BoundedType here. Need to discuss with you 
to understand when a super type can be bounded. In the super type chain the
parser creates and that we are dealing with here, there are no bounded types.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1538: void ClassFinalizer::CopyArgs(const Class&
cls,
On 2013/03/14 23:40:34, regis wrote:
> I would pick more meaningful names. What args are you copying? From where to
> where? Actually, it is not just a copy operation. Maybe something like
> ExtractTypeArguments or
> void CollectTypeArguments(cls, type, collected_type_args)?

Done.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1578: ASSERT(type.IsType());
On 2013/03/14 23:40:34, regis wrote:
> Not necessarily. It could be a BoundedType. Add a TODO for now, and you will
> need lots of tests.
Same reasoning as above: the parser does not put a bounded type here. Added
TODO.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1602: type ^= mixins.At(mixins.Length() - 1);
On 2013/03/14 23:40:34, regis wrote:
> Can you explain in a comment that the last type in the array represents the
raw
> type of the mixin app class?
> Would it be cleaner to add a separate field of type Class rather than stuffing
a
> raw Type in the mixins array for this? 

Actually, all elements in the mixins array are mixin application types, a type
formed by the generated mixin application class and the type arguments parsed
from the source. I definitely want to make this nicer down the road. I would be
most happy if I could eliminate the MixinAppType again if I have a better grasp
on how to resolve and finalize the mixin applications.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.h:103: 
On 2013/03/14 23:40:34, regis wrote:
> No blank line elsewhere in this private section.
Removed

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.h:106: 
On 2013/03/14 23:40:34, regis wrote:
> ... except here.
Also removed.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.h:106: 
On 2013/03/14 23:40:34, regis wrote:
> ... except here.
Also removed.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9898: bool MixinAppType::IsResolved() const {
On 2013/03/14 23:40:34, regis wrote:
> I do not think this call is necessary. By definition, a MixinAppType is not
> resolved.

Done.

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

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.h#newco...
runtime/vm/object.h:3755: virtual bool IsMalformed() const { return false; }
On 2013/03/14 23:40:34, regis wrote:
> That is optimistic :-)
> I would actually leave this call out, and rely on the AbstractType version to
> bomb if this is called. We should not call it, since we will never encounter a
> MixinAppType in a type test.
There is unfortunately code that calls this and bombs if I don't overwrite this
function. Eg, CollectFinalizedSuperClasses()  is a caller.
In a way, a type can't be malformed if it's not resolved yet, so returning false
seems acceptable to me.

https://codereview.chromium.org/12779008/diff/29001/runtime/vm/object.h#newco...
runtime/vm/object.h:3756: virtual bool IsResolved() const;
On 2013/03/14 23:40:34, regis wrote:
> I would unconditionally return false.

Done.

[hausner, 2013-03-15 15:59:36]

Committed patchset #6 manually as r20084 (presubmit successful).