Disallow mixing in classes that use 'super'.

Disallow mixing in classes that use 'super'.


R=johnniwinther@google.com
BUG=

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

[ahe, 2013-01-23 11:36:58]

I'd like to understand what a mixin use is. If it is what I think it is, I don't
think it is safe to record it in the element. See World.subtypes.

[kasperl, 2013-01-23 11:40:18]

On 2013/01/23 11:36:58, ahe wrote:
> I'd like to understand what a mixin use is. If it is what I think it is, I
don't
> think it is safe to record it in the element. See World.subtypes.

A mixin application uses a class as a mixin. Every mixin application uses
exactly one class -- but a single class may be used as a mixin by multiple mixin
applications. Please elaborate on what you think is unsafe.

[ahe, 2013-01-23 11:55:14]

On 2013/01/23 11:40:18, kasperl wrote:
> A mixin application uses a class as a mixin. Every mixin application uses
> exactly one class -- but a single class may be used as a mixin by multiple
mixin
> applications. Please elaborate on what you think is unsafe.

So it sounds like a class that is used by a mixin applications has a list of
these mixin applications. I think this is analogous to having a list of
subclasses associated with a class.

If you implement incremental compilation, and do not take care to flush this
information, it can lead to a memory leak where the (unchanged) superclass is
keeping old versions of its subclasses (mixin uses) alive. Also, a class element
is no longer immutable after it has been resolved.

So instead we store such information on the side, see World.subtypes.

[ahe, 2013-01-23 11:58:03]

On 2013/01/23 11:55:14, ahe wrote:
> On 2013/01/23 11:40:18, kasperl wrote:
> > A mixin application uses a class as a mixin. Every mixin application uses
> > exactly one class -- but a single class may be used as a mixin by multiple
> mixin
> > applications. Please elaborate on what you think is unsafe.
> 
> So it sounds like a class that is used by a mixin applications has a list of
> these mixin applications. I think this is analogous to having a list of
> subclasses associated with a class.
> 
> If you implement incremental compilation, and do not take care to flush this
> information, it can lead to a memory leak where the (unchanged) superclass is
> keeping old versions of its subclasses (mixin uses) alive. Also, a class
element
> is no longer immutable after it has been resolved.
> 
> So instead we store such information on the side, see World.subtypes.

To be very concrete: eventually, I'd like to store the platform libraries as
compile-time constants in the dart2js snapshot.

[kasperl, 2013-01-23 12:04:04]

On 2013/01/23 11:58:03, ahe wrote:
> On 2013/01/23 11:55:14, ahe wrote:
> > On 2013/01/23 11:40:18, kasperl wrote:
> > > A mixin application uses a class as a mixin. Every mixin application uses
> > > exactly one class -- but a single class may be used as a mixin by multiple
> > mixin
> > > applications. Please elaborate on what you think is unsafe.
> > 
> > So it sounds like a class that is used by a mixin applications has a list of
> > these mixin applications. I think this is analogous to having a list of
> > subclasses associated with a class.
> > 
> > If you implement incremental compilation, and do not take care to flush this
> > information, it can lead to a memory leak where the (unchanged) superclass
is
> > keeping old versions of its subclasses (mixin uses) alive. Also, a class
> element
> > is no longer immutable after it has been resolved.
> > 
> > So instead we store such information on the side, see World.subtypes.
> 
> To be very concrete: eventually, I'd like to store the platform libraries as
> compile-time constants in the dart2js snapshot.

SGTM. We're still very far from having immutable elements but it's certainly the
direction we should be heading in.

[kasperl, 2013-01-23 12:19:26]

Uploaded new version with the mixin uses stored in compiler.world. PTAL.

[ahe, 2013-01-23 12:32:09]

Took a quick look (my brain is used up for today) and I didn't spot anything
scary. However, I do have a few comments.

https://codereview.chromium.org/12033049/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12033049/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:568:
compiler.reportMessage(compiler.spanFromNode(use),
I'm not sure this gives the best error message possible. I think that you get a
lot of messages about where you're using super (which is nice) but no
information about which mixin application that is using a class that cannot be
mixed in.

My suggestion is:

report an error on the mixin application if resolutionTree.superUses is
non-empty.

Change these messages to be of kind INFO.

https://codereview.chromium.org/12033049/diff/11001/tests/language/mixin_ille...
File tests/language/mixin_illegal_super_use_test.dart (right):

https://codereview.chromium.org/12033049/diff/11001/tests/language/mixin_ille...
tests/language/mixin_illegal_super_use_test.dart:10: super.toString();    ///
01: compile-time error
I'd write this as:

super.    /// 01: compile-time error
    toString();

Applies to most compile-time errors below.

https://codereview.chromium.org/12033049/diff/11001/tests/language/mixin_ille...
tests/language/mixin_illegal_super_use_test.dart:61: typedef F = Object with P2,
M;
I'm not sure you're instantiating classes C-F.

[kasperl, 2013-01-23 12:41:54]

https://codereview.chromium.org/12033049/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12033049/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:568:
compiler.reportMessage(compiler.spanFromNode(use),
On 2013/01/23 12:32:10, ahe wrote:
> I'm not sure this gives the best error message possible. I think that you get
a
> lot of messages about where you're using super (which is nice) but no
> information about which mixin application that is using a class that cannot be
> mixed in.
> 
> My suggestion is:
> 
> report an error on the mixin application if resolutionTree.superUses is
> non-empty.
> 
> Change these messages to be of kind INFO.

I thought about that too (which is why I'm passing the mixinApplication to this
method in the first place) so I'll try that out. Good idea with using INFO for
the nodes that actually use super.

[kasperl, 2013-01-23 12:42:31]

https://codereview.chromium.org/12033049/diff/11001/tests/language/mixin_ille...
File tests/language/mixin_illegal_super_use_test.dart (right):

https://codereview.chromium.org/12033049/diff/11001/tests/language/mixin_ille...
tests/language/mixin_illegal_super_use_test.dart:61: typedef F = Object with P2,
M;
On 2013/01/23 12:32:10, ahe wrote:
> I'm not sure you're instantiating classes C-F.

I am. See P1.test.

[kasperl, 2013-01-23 13:41:25]

PTAL.

https://codereview.chromium.org/12033049/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12033049/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:568:
compiler.reportMessage(compiler.spanFromNode(use),
On 2013/01/23 12:32:10, ahe wrote:
> I'm not sure this gives the best error message possible. I think that you get
a
> lot of messages about where you're using super (which is nice) but no
> information about which mixin application that is using a class that cannot be
> mixed in.
> 
> My suggestion is:
> 
> report an error on the mixin application if resolutionTree.superUses is
> non-empty.
> 
> Change these messages to be of kind INFO.

Done.

[Johnni Winther, 2013-01-24 08:19:09]

lgtm

https://codereview.chromium.org/12033049/diff/13002/tests/language/mixin_ille...
File tests/language/mixin_illegal_super_use_test.dart (right):

https://codereview.chromium.org/12033049/diff/13002/tests/language/mixin_ille...
tests/language/mixin_illegal_super_use_test.dart:38: // P1 class and its bar
method to be resolved before resolve the
resolve => resolving