mixin on a ClassMirror of a class which is the result of a mixin application of the form S with M doesn't work

[a-siva]

The ClassMirror of a class which is the result of mixing application of the form S with M returns a class mirror of the reflectee and not the mixin.

The code below -

import 'dart:mirrors';
class Super {}
class Mixin {}
class Mixin2 {}
class Mixin3 {}
class MixinApplication = Super with Mixin;
class Class extends Super with Mixin {}
class MultipleMixins extends Super with Mixin, Mixin2, Mixin3 {}
main() {
  print(reflectClass(MixinApplication).mixin);
}

prints
"ClassMirror on 'Mixin'" when run with Dart 1 and
"ClassMirror on 'MixinApplication'" with Dart 2.

Cause - The mixin_ field in the RawClass object in the VM is not being correctly populated,
The kernel file does not have this information to be able to populate this field correctly when loading the kernel file in the VM.

[alexmarkov]

The mixed-in type of a mixin application is not available as it is removed while mixins are de-sugared by kernel mixin transformation. This transformation erases mixed-in type and adds it into the list of interfaces.

In order to correctly implement dart:mirrors, CFE should leave some information about original mixed-in type and original list of interfaces in Dart sources, or at least inform VM about performed transformation. While keeping original mixed-in type and original list of interfaces is a cleaner solution, it will increase size of kernel binaries.

Instead of adding those fields, we can introduce 'isTransformedMixinApplication' flag and propagate it all the way to the dart:mirrors implementation. Using this flag, dart:mirrrors implementation can 'undo' the changes in mixed-in type and superinterfaces.

This solution is implemented here: https://dart-review.googlesource.com/c/sdk/+/56721.

@kmillikin please take a look. Can you suggest a better way of representing such information in kernel?

[kmillikin]

Just a quick comment because it's late.

That was exactly the intent of isSyntheticMixinApplication (https://codereview.chromium.org/2982373002) but it turns out that the implementation was incomplete. The flag was not documented in binary.md and was not set in all cases.

For classes marked as synthetic mixins, you should be able to rely on the superclass being the superclass and the mixed-in class being the only implemented interface.

Is there any reason that the VM needs to distinguish between "synthetic mixins" and "transformed mixins"? If not, I think a change like this should be sufficient on the Kernel side:

diff --git a/pkg/kernel/binary.md b/pkg/kernel/binary.md
index 13b28bf961..b3b35e54e4 100644
--- a/pkg/kernel/binary.md
+++ b/pkg/kernel/binary.md
@@ -286,7 +286,8 @@ type Class extends Node {
   UriReference fileUri;
   FileOffset fileOffset;
   FileOffset fileEndOffset;
-  Byte flags (isAbstract, isEnum, xx); // Where xx is index into ClassLevel
+  Byte flags (isAbstract, isEnum, isSyntheticMixinApplication,
+              xx); // Where xx is index into ClassLevel
   StringReference name;
   List<Expression> annotations;
   List<TypeParameter> typeParameters;
diff --git a/pkg/kernel/lib/transformations/mixin_full_resolution.dart b/pkg/kernel/lib/transformations/mixin_full_resolution.dart
index a2f3926209..1b01660a1e 100644
--- a/pkg/kernel/lib/transformations/mixin_full_resolution.dart
+++ b/pkg/kernel/lib/transformations/mixin_full_resolution.dart
@@ -237,6 +237,7 @@ class MixinFullResolution {
 
     // This class is now a normal class.
     class_.mixedInType = null;
+    class_.isSyntheticMixinImplementation = true;
   }
 
   Constructor buildForwardingConstructor(

(We should probably consider updating the text format but that requires rebasing tests.)

[alexmarkov]

Thank you for quick feedback!

We need a flag which covers mixin applications which were transformed to normal classes, including anonymous mixin applications and named mixin applications.

Synthetic mixin flag was not set for named mixin applications (mixin aliases):

sdk/pkg/front_end/lib/src/fasta/kernel/kernel_library_builder.dart

Lines 452 to 453 in 57d2563

	application.cls.isSyntheticMixinImplementation =
	!isNamedMixinApplication;

I was afraid to change the semantics of the isSyntheticMixinImplementation flag as other backends might not expect that:

sdk/pkg/compiler/lib/src/kernel/env.dart

Line 300 in 57d2563

bool get isUnnamedMixinApplication => cls.isSyntheticMixinImplementation;

Do you think it's a good idea to set 'isSyntheticMixinImplementation' for named mixin applications even though they are present in sources and not really synthetic?

AFAICT, currently VM doesn't use synthetic mixin application flag, but it could be need in future. For example, dart:mirrors implementation distinguishes between named and anonymous mixin applications for Dart 1 mixins, and eventually we might need this for desugared Dart 2 mixins as well.

Also, named mixin applications may have implemented interfaces, so mixed-in type is not the only type in their lists of interfaces.

Please advise how to proceed.

[kmillikin]

The VM is the only backend that uses the mixin elimination transformation. So the question is: does the VM need to distinguish between classes that were originally named or anonymous mixin applications. If not, we don't need a separate flag.

Dart2js only sees named (never anonymous) mixin applications. If it sees the flag on a named mixin application it knows that it was originally anonymous.

The VM only sees normal classes (never mixin applications). If it sees the flag on a normal class it knows it was originally a mixin application.

Is that sufficient? (The name is not great, but I can't think of anything that's much better so I'm inclined to just leave it for now.)

You're right about named mixing being able to implement interfaces. That means you can rely on there being at least one implementedType and implementedTypes.last is the original mixed-in type.