Fix #29003

Fix #29003

Expand out nested mixins to apply them separately.

[vsm, 2017-03-27 21:47:05]

Description was changed from

==========
Fix #29003

Expand out nested mixins to apply them separately.
==========

to

==========
Fix #29003

Expand out nested mixins to apply them separately.
==========

[vsm, 2017-03-27 21:47:05]

vsm@google.com changed reviewers:
+ jmesserly@google.com

[Jennifer Messerly, 2017-03-27 22:15:15]

Let me know if I can clarify comments ... or if I should take it, happy to do
so, I think it's a very quick fix to visitClassTypeAlias

https://codereview.chromium.org/2778083002/diff/10001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2778083002/diff/10001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1286: List<InterfaceType>
_flattenMixins(List<InterfaceType> mixins) {
I don't think this is correct in general?

it will expand out mixins arbitrarily, yes it happens to work with "class M2 =
Object with M1;" but it won't work with:

    class M2 extends Object with M1 { foo() => 123 }

... the method "foo" above would just get discarded?

For the moment, that may be disallowed by another check, but in general
flattening out seems really bad. For example:

library 1:
    // Private!
    class _M1 { int foo() => 42; }
    class M2 = Object with _M1;

library 2:

    class M3 extends M2 {} // does this expand to _M1 ??


... with this fix, wouldn't we try to generate a private reference to M1 from
library2  ... that might happen to work but it's very unsafe.

Finally ... this would support invalid things like:
    class M1 { int foo() => 42; }
    class M2 { int foo() => 42; }
    class M3 = Object with M1, M2;
    class M4 extends Object with M3 {} // not actually valid??

I described this in more detail here:
https://github.com/dart-lang/sdk/issues/29003#issuecomment-285471442

... what's going on here is that "class M2 = Object with M1;" is like a special
form of typedef.

I think the way to support this is handling the special case "type A = Object
with B" in visitClassTypeAlias. That has a lot less risk of accidentally causing
other breakage.

[vsm, 2017-03-27 22:22:24]

On 2017/03/27 22:15:15, Jennifer Messerly wrote:
> Let me know if I can clarify comments ... or if I should take it, happy to do
> so, I think it's a very quick fix to visitClassTypeAlias
> 
>
https://codereview.chromium.org/2778083002/diff/10001/pkg/dev_compiler/lib/sr...
> File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):
> 
>
https://codereview.chromium.org/2778083002/diff/10001/pkg/dev_compiler/lib/sr...
> pkg/dev_compiler/lib/src/compiler/code_generator.dart:1286:
List<InterfaceType>
> _flattenMixins(List<InterfaceType> mixins) {
> I don't think this is correct in general?
> 
> it will expand out mixins arbitrarily, yes it happens to work with "class M2 =
> Object with M1;" but it won't work with:
> 
>     class M2 extends Object with M1 { foo() => 123 }
> 
> ... the method "foo" above would just get discarded?
> 
> For the moment, that may be disallowed by another check, but in general
> flattening out seems really bad. For example:
> 
> library 1:
>     // Private!
>     class _M1 { int foo() => 42; }
>     class M2 = Object with _M1;
> 
> library 2:
> 
>     class M3 extends M2 {} // does this expand to _M1 ??

Ahh, good example!

> 
> 
> ... with this fix, wouldn't we try to generate a private reference to M1 from
> library2  ... that might happen to work but it's very unsafe.
> 
> Finally ... this would support invalid things like:
>     class M1 { int foo() => 42; }
>     class M2 { int foo() => 42; }
>     class M3 = Object with M1, M2;
>     class M4 extends Object with M3 {} // not actually valid??
> 
> I described this in more detail here:
> https://github.com/dart-lang/sdk/issues/29003#issuecomment-285471442
> 
> ... what's going on here is that "class M2 = Object with M1;" is like a
special
> form of typedef.
> 
> I think the way to support this is handling the special case "type A = Object
> with B" in visitClassTypeAlias. That has a lot less risk of accidentally
causing
> other breakage.

Yeah, I'd been relying on other static checks to prevent the "bad" cases of
flatten.  But, funny enough, I started with the special case approach.   Let me
go back to that one.

[vsm, 2017-03-27 22:47:51]

On 2017/03/27 22:22:24, vsm wrote:
> On 2017/03/27 22:15:15, Jennifer Messerly wrote:
> > Let me know if I can clarify comments ... or if I should take it, happy to
do
> > so, I think it's a very quick fix to visitClassTypeAlias
> > 
> >
>
https://codereview.chromium.org/2778083002/diff/10001/pkg/dev_compiler/lib/sr...
> > File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):
> > 
> >
>
https://codereview.chromium.org/2778083002/diff/10001/pkg/dev_compiler/lib/sr...
> > pkg/dev_compiler/lib/src/compiler/code_generator.dart:1286:
> List<InterfaceType>
> > _flattenMixins(List<InterfaceType> mixins) {
> > I don't think this is correct in general?
> > 
> > it will expand out mixins arbitrarily, yes it happens to work with "class M2
=
> > Object with M1;" but it won't work with:
> > 
> >     class M2 extends Object with M1 { foo() => 123 }
> > 
> > ... the method "foo" above would just get discarded?
> > 
> > For the moment, that may be disallowed by another check, but in general
> > flattening out seems really bad. For example:
> > 
> > library 1:
> >     // Private!
> >     class _M1 { int foo() => 42; }
> >     class M2 = Object with _M1;
> > 
> > library 2:
> > 
> >     class M3 extends M2 {} // does this expand to _M1 ??
> 
> Ahh, good example!
> 
> > 
> > 
> > ... with this fix, wouldn't we try to generate a private reference to M1
from
> > library2  ... that might happen to work but it's very unsafe.
> > 
> > Finally ... this would support invalid things like:
> >     class M1 { int foo() => 42; }
> >     class M2 { int foo() => 42; }
> >     class M3 = Object with M1, M2;
> >     class M4 extends Object with M3 {} // not actually valid??
> > 
> > I described this in more detail here:
> > https://github.com/dart-lang/sdk/issues/29003#issuecomment-285471442
> > 
> > ... what's going on here is that "class M2 = Object with M1;" is like a
> special
> > form of typedef.
> > 
> > I think the way to support this is handling the special case "type A =
Object
> > with B" in visitClassTypeAlias. That has a lot less risk of accidentally
> causing
> > other breakage.
> 
> Yeah, I'd been relying on other static checks to prevent the "bad" cases of
> flatten.  But, funny enough, I started with the special case approach.   Let
me
> go back to that one.

closing this CL in favor of https://codereview.chromium.org/2783443002/