DDK fails to represent some mixin super hierarchies.

[Markzipan]

Consider:

import "expect.dart";

class B {
  toString() => 'B(' + super.toString() + ')';
}

class R {
  toString() => 'R[' + super.toString() + ']';
}

class D extends R with B {
  toString() => 'D<' + super.toString() + '>';
}

main() {
  check(object, String expected) {
    Expect.equals(expected, object.toString());
  }

  check(D(), "D<B(R[Instance of '$D'])>");
}

DDK fails with: D<B([Instance of '$D'])>

This test succeeds if B is explicitly declared as a mixin. Looking at the generated code, the only difference seems to be whether or not _usesMixinNew(c) returns true or false. We'd like to do whatever that function does for any class used as a mixin - not just those declared with 'mixin'.

[eernstg]

A fresh analyzer (34219c802d98c392c9f21abcdb7b2b26fe5648d4) complains because B is used as a mixin, and it references super.

So that would essentially eliminate the program from consideration, because we generally assume that programs rejected by the analyzer may be accepted by the vm and other execution platforms (so they don't have to be entirely as strict as the analyzer, at least not immediately).

This looks like a constraint which would follow naturally from going back to a rather restrictive level of support for using classes as mixins (that's how it used to be, a long time ago, and then some restrictions were lifted, but today we have the explicit mixin declarations which are intended to handle all those more fancy forms of mixing in stuff, so the class-as-mixin residual can again be simpler and more strict).

The error message seems to arise here, but there's no reference to a spec update/location in the code or in the commit where it was introduced, and I still haven't found a location in the spec where it is mandated to emit this error...

@stereotype441, @bwilkerson, @scheglov: Can you help me find this requirement in the spec? If it is actually not required then the analyzer should be less strict, and then we have an analyzer issue here (though it might be appropriate to create a new issue for it). Note that this particular access to super is not an error in itself, because it targets a method in Object.

So we have two possible situations:

It is an error

If we can find justification in the spec that it is an error for B to be used as a mixin because it has a method that references super then this issue goes away: It does not matter whether class B and mixin B have the same behavior when used as a mixin, because class B cannot be used as a mixin, period.

It is not an error

In this case we conclude that class B can be used as a mixin even though it has a method that references super, because the spec really doesn't make it an error (that's how I currently see it).

It then does matter whether B behaves differently when declared as a class and as a mixin (and based on the notion of deriving a mixin from a class I believe that we can conclude that they should behave the same).

[eernstg]

Trying to run the example program with DDK, I can see that the superinvocation in B.toString invokes the Object.toString, with the following configuration:

custom configuration(architecture: x64, compiler: dartdevk, mode: release, runtime: chrome, system: linux, vm-options: [], dart2js-options: [], timeout: 960, host-checked, preview-dart-2)

This means that DDK generates code for B.toString when B is used as a mixin in the same way as it does when B is used as a class (so it "forgets" to rebind the target of the superinvocation to the actual superclass when it's used as a mixin).

This is definitely a bug.

It may still be the case that the program should be rejected in the first place, but in that case DDK should also reject the program rather than generating wrong code for it.

So here's the program used to test this (stored in 'tests/language_2/scratch_test.dart'):

// Cf. SDK issue #36585.

import "package:expect/expect.dart";

class B {
  toString() => 'B(' + super.toString() + ')';
}

class R {
  toString() => 'R[' + super.toString() + ']';
}

class D extends R with B {
  toString() => 'D<' + super.toString() + '>';
}

main() {
  check(object, String expected) {
    Expect.equals(expected, object.toString());
  }

  check(D(), "D<B(R[Instance of '$D'])>");
}

Here's the command: tools/test.py --host-checked -mrelease -cdartdevk language_2/scratch.

Here's an excerpt of the output:

Runtime error:
Error: Expect.equals(at index 4: Expected <...D<B(R[Instance of 'D'])>...>, Found: <...D<B(Instance of 'D')>>) fails.
at Object.dart.throw (http://127.0.0.1:45333/root_build/gen/utils/dartdevc/kernel/amd/dart_sdk.js:3870:11)
    at Function._fail (http://127.0.0.1:45333/root_build/gen/utils/dartdevc/pkg_kernel/expect.js:478:17)
    at Function.equals (http://127.0.0.1:45333/root_build/gen/utils/dartdevc/pkg_kernel/expect.js:109:25)

[stereotype441]

The requirement was removed from the spec a very long time ago, in 2c7234bd610bd5a99dd501da4ee59e78a33f1839, however only the VM implementation was changed to reflect the spec change. Dart2js continued flagging an error if a class that referenced "super" was mixed into another class. The analyzer behavior continued to match dart2js, except that a "superMixin" flag was added to allow users to request that it instead match the VM behavior (and the spec).

My understanding is that ahead-of-time compilers like dart2js have trouble generating efficient code if classes that reference "super" are allowed to be used as mixins, because it means that references to super can't be statically resolved to a method in the class's superclass. It's especially a problem for modular ahead-of-time compilers like DDC, which can't use whole program analysis to determine that a class isn't actually ever used as a mixin. I believe a large part of the motivation for introducing the "mixin" syntax in the first place was that we only wanted to pay the efficiency expense for classes that were truly intended to be used as mixins. So the intent was that if an ordinary class was used as a mixin, it would be an error for it to reference "super". But if a class declared using the "mixin" syntax was used as a mixin, it would be allowed for it to reference "super". AFAIK, this is what was implemented in the analyzer, which is why the analyzer rejects the code above.

So as I understand things, we have two bugs: (1) we forgot to re-add the restriction on referencing "super" to the spec, and (2) we forgot to implement that restriction in the front end.

Perhaps someone on the language team (@leafpetersen maybe) can confirm whether I'm understanding things correctly.

[leafpetersen]

So as I understand things, we have two bugs: (1) we forgot to re-add the restriction on referencing "super" to the spec, and (2) we forgot to implement that restriction in the front end.

Yes. It is illegal to mix in a class which has a super call in it. This was essentially the entire point of the super mixin migration.

[eernstg]

@stereotype441 wrote:

Perhaps someone on the language team (@leafpetersen maybe) can confirm

I'm on the language team, maintaining the spec. ;-)

But what I saw was an error that I did not expect, and I thought the code for this error was introduced recently, maybe here, maybe at this time, because of the introduction of syntactic mixin declarations.

That's the reason why I essentially asked "Why did you introduce this error in the analyzer?", hoping to get a reference to a part of the spec that I hadn't found, saying that we must have this error.

But your explanation shows that it's been part of the "supermixin" feature that we "sort of had" for a long time. I just didn't have that discrepancy between the spec and the reality in mind when I saw this issue.

So as I understand things, we have two bugs: (1) we forgot to re-add the restriction on referencing "super" to the spec, and (2) we forgot to implement that restriction in the front end.

That makes sense, especially because we can restrict the mixin-from-class feature now that we have an explicit mixin declaration.

I've created a spec update issue to get that done.

This means that the remaining work in this issue is for DDK/CFE to report that error.