dart2js does not support using the same class as a mixin twice

[mkustermann]

The following test generates incorrect output with dart2js.

class Box {
  var value;
} 

class Upper extends Object with Box { }

class UpperAccessors extends Upper {
  get upperValue => super.value;
  set upperValue(v) => super.value = v;
} 

class Lower extends UpperAccessors with Box {}

class LowerAccessors extends Lower {
  get lowerValue  => super.value;
  set lowerValue(v) => super.value = v;
} 

main() {
  var o = new LowerAccessors();
  o.upperValue = 42;
  o.lowerValue = 84;
  print(o.upperValue);
  print(o.lowerValue);
}

The Dart VM correctly computes:

$ dart test.dart
42
84

Though for dart2js it is:

$ dart2js test.dart
$ third_party/d8/linux/d8 out.js
84
84

[rakudrama]

Box is mixed in twice. So there are two fields named value. We do not yet have a plan for how to implement this efficiently in JavaScript. We could clone Box for the second and subsequent mixin applications. This is a lot of machinery that simply does not exist at the moment. There is a similar problem with calling methods on the mixed-in class, and for super calls in the mixed-in class's methods. We are considering making it an error to have the same class mixed in twice (until we can make it work.)

[rakudrama]

I'm adding a priority (P2 for now), since it has affected a customer. We should give an error (or implement it fully) but not silently generate bad code.

[lrhn]

Just discovered this again while trying to test something else (equality of member-tear-offs of the same mixin applied more than once).

[eernstg]

Here is a version of the example program that works today (and demonstrates the bug in dart compile js, and not in dart, nor in dart compile exe):

mixin Box {
  late int value;
} 

class Upper with Box { }

class UpperAccessors extends Upper {
  get upperValue => super.value;
  set upperValue(v) => super.value = v;
} 

class Lower extends UpperAccessors with Box {}

class LowerAccessors extends Lower {
  get lowerValue  => super.value;
  set lowerValue(v) => super.value = v;
} 

main() {
  var o = new LowerAccessors();
  o.upperValue = 42;
  o.lowerValue = 84;
  print(o.upperValue);
  print(o.lowerValue);
}

[rakudrama]

The operation of multiple mixins is super hard to reason about :wink:. Erik's nice example would be much clearer if rewritten using delegation.

In https://github.com/dart-lang/sdk/issues/28794 (and https://github.com/dart-lang/sdk/issues/53682) we were mostly leaning towards banning using the same mixing twice because it is so hard to use correctly. Can we do that? Ban a bad feature that is hard to use and hard to implement?

The most viable path for dart2js is to expand out the second mixin application at the Kernel level. Other implementation strategies would be more expensive to implement and have too much impact on the quality of code for the vast majority of code that has single applications.

[lrhn]

The operation of multiple mixins is super hard to reason about 😉.

You can write incomprehensible code in any language.

What I like about mixins is that they're actually easy to reason about by themselves. Each mixin application adds another layer of class inheritance, with members that work as if you had written them yourself (in the library where the macro was written, for the lexical scope).

That's why I don't like adding exceptions about mixing in the same mixin twice, because there should be no real difference between C1 and C2 of:

class C1 with Box, SuperBox, Box {}
class C2 with Box, SuperBox, Box2 {}
mixin Box {
  Object? value;
}
mixin Box2 {
  Object? value;
}
mixin SuperBox on Box {
  Object? get superValue => super.value; 
  set superValue(Object? value) { super.value = value; }
}

The two Box mixins should behave the same, the only functional difference between C1 and C2 is that C2 implements one extra interface implemented, Box2. In Dart2JS they're not:

void main() {
  var c1 = C1();
  c1.value = 42;
  c1.superValue = 37;
  var c2 = C2();
  c2.value = 42;
  c2.superValue = 37;
  // In DartPad prints: (37,47) =?= (42,37)
  print('${(c1.value, c1.superValue)} =?= ${(c2.value, c2.superValue)}');
}

Adding exceptions to simple rules costs. Sometimes unpredictably, because someone will hit the exception at an inopportune time. If you need to know whether a class implements or extends/mixes in a superinterface, in order to know whether you can mix that mixin class in again, then the superclass abstraction is broken. (The only thing you need to know about your superclass is which interfaces it implements and wihch generative constructors it has, if it allows extending at all.)

When an abstraction is broken, it makes more things breaking changes, things that were equivalent to the abstraction.

If we disallow mixing in the same mixin twice, or mixing a mixin class on top of itself, then it would matter whether I do abstract class MyCollection extends Iterable or abstract class MyCollection implements Iterable, because one of them would prevent doing with Iterable in a subclass. But the subclass creator shouldn't need to know which one I used. Because that means that I, as an author, must now consider which one I use not only from my own needs, but from whether I predict anyone will ever want to mix in the same mixin again.

Using a mixin becomes a liability, it's better to copy the code. That's the exact opposite incentive of what I'd want for mixins.

So it's a perfectly reasonable feature, which may be slightly complicated to implement, if some optimziations assume that double-mixin cannot happen. But if with Box, Box is harder than with Box, Box2, then it sounds like an implementation choice more than a real issue. As you say, copying the mixin at the Kernel level for the second application might solve the problem. That's very likely since if I add:

mixin class Box3 = Object with Box;
class C3 with Box, SuperBox, Box3 {}

then C3 works just as well as C2. Doing a "copy" of a mixin does make things work. (That workaround, as written here, only works with mixins that can be applied to Object, but it works. If you can do it at the kernel level, it can probably work for any mixin.)