[vm/aot] Ensure interface targets are re-resolved after deduping of mixin application classes

When the CFE compiles an application as a whole (reads in all source
code), the interface targets to members of mixin applications are the
original mixin classes.

When the CFE compiles an application modularly (dependencies are
supplied as kernel files), the interface targets to members of mixin
applications are the copied members in the mixin application classes.

This slight difference does not surface in any test failures. Yet if we
start running our AOT kernel pipeline, we will deduplicate the mixin
application classes. This leaves dangling references (with no target).

The C++ AOT compiler, `gen_snapshot`, will crash if it hits any of
those.

Issue https://github.com/dart-lang/sdk/issues/39375

Change-Id: I17a57370a87cfbdc174829c2e68ecdb7c4a9757e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/124993
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>

pkg/front_end/testcases/extensions/deferred_import_hidden.dart.strong.expect

@@ -1,6 +1,7 @@
 library;
 import self as self;
 import "deferred_explicit_access_lib.dart" as def;
+import "dart:core" as core;
 
 import "org-dartlang-testcase:///deferred_explicit_access_lib.dart" deferred as prefix;
 
@@ -15,7 +16,7 @@ static method main() → dynamic async {
   self::expect(87, let final dynamic #t7 = CheckLibraryIsLoaded(prefix) in def::topLevelMethod());
 }
 static method expect(dynamic expected, dynamic actual) → dynamic {
-  if(!expected.{dart.core::Object::==}(actual))
+  if(!expected.{core::Object::==}(actual))
     throw "Expected ${expected}, actual ${actual}";
 }
 

pkg/front_end/testcases/extensions/import_via_prefix.dart.strong.expect

@@ -1,6 +1,7 @@
 library;
 import self as self;
 import "import_via_prefix_lib.dart" as imp;
+import "dart:core" as core;
 
 import "org-dartlang-testcase:///import_via_prefix_lib.dart" as prefix;
 
@@ -8,7 +9,7 @@ static method main() → dynamic {
   self::expect(3, imp::Extension|method("foo"));
 }
 static method expect(dynamic expected, dynamic actual) → dynamic {
-  if(!expected.{dart.core::Object::==}(actual))
+  if(!expected.{core::Object::==}(actual))
     throw "Expected ${expected}, actual ${actual}";
 }
 

pkg/front_end/testcases/extensions/import_via_prefix.dart.strong.transformed.expect

@@ -1,6 +1,7 @@
 library;
 import self as self;
 import "import_via_prefix_lib.dart" as imp;
+import "dart:core" as core;
 
 import "org-dartlang-testcase:///import_via_prefix_lib.dart" as prefix;
 
@@ -8,7 +9,7 @@ static method main() → dynamic {
   self::expect(3, imp::Extension|method("foo"));
 }
 static method expect(dynamic expected, dynamic actual) → dynamic {
-  if(!expected.{dart.core::Object::==}(actual))
+  if(!expected.{core::Object::==}(actual))
     throw "Expected ${expected}, actual ${actual}";
 }
 

pkg/front_end/testcases/inference/downwards_inference_on_list_literals_infer_if_value_types_match_context.dart.outline.expect

@@ -3,7 +3,7 @@ import self as self;
 import "dart:core" as core;
 
 typedef Asserter<contravariant T extends core::Object* = dynamic> = (T*) →* void;
-typedef AsserterBuilder<contravariant S extends core::Object* = dynamic, unrelated T extends core::Object* = dynamic> = (S*) →* (T*) →* void;
+typedef AsserterBuilder<contravariant S extends core::Object* = dynamic, contravariant T extends core::Object* = dynamic> = (S*) →* (T*) →* void;
 class DartType extends core::Object {
   synthetic constructor •() → self::DartType*
     ;

pkg/front_end/testcases/inference/generic_methods_infer_generic_function_parameter_type2.dart.outline.expect

@@ -2,7 +2,7 @@ library test;
 import self as self;
 import "dart:core" as core;
 
-typedef G<unrelated V extends core::Object* = dynamic> = () →* core::List<V*>*;
+typedef G<V extends core::Object* = dynamic> = () →* core::List<V*>*;
 class C<T extends core::Object* = dynamic> extends self::D<self::C::T*> {
   synthetic constructor •() → self::C<self::C::T*>*
     ;

pkg/front_end/testcases/outline.status

@@ -2,12 +2,6 @@
 # for details. All rights reserved. Use of this source code is governed by a
 # BSD-style license that can be found in the LICENSE.md file.
 
-inference/downwards_inference_on_list_literals_infer_if_value_types_match_context: ExpectationFileMismatchSerialized
-inference/generic_methods_infer_generic_function_parameter_type2: ExpectationFileMismatchSerialized
-regress/issue_31181: ExpectationFileMismatchSerialized
-regress/issue_31213: ExpectationFileMismatchSerialized
-runtime_checks/contravariant_generic_return_tear_off: ExpectationFileMismatchSerialized
-
 general/abstract_members: TypeCheckError
 general/bug30695: TypeCheckError
 general/bug30695: TypeCheckError

pkg/front_end/testcases/rasta/issue_000031.dart.strong.expect

@@ -7,11 +7,12 @@ library;
 //   ^^^^
 //
 import self as self;
+import "dart:core" as core;
 
 import "dart:math" as math;
 
 static method main() → dynamic {
   let final dynamic #t1 = invalid-expression "pkg/front_end/testcases/rasta/issue_000031.dart:8:3: Error: A prefix can't be used as an expression.
   math..toString();
-  ^^^^" in let final void #t2 = #t1.{dart.core::Object::toString}() in #t1;
+  ^^^^" in let final void #t2 = #t1.{core::Object::toString}() in #t1;
 }

pkg/front_end/testcases/rasta/issue_000031.dart.strong.transformed.expect

@@ -7,11 +7,12 @@ library;
 //   ^^^^
 //
 import self as self;
+import "dart:core" as core;
 
 import "dart:math" as math;
 
 static method main() → dynamic {
   let final dynamic #t1 = invalid-expression "pkg/front_end/testcases/rasta/issue_000031.dart:8:3: Error: A prefix can't be used as an expression.
   math..toString();
-  ^^^^" in let final void #t2 = #t1.{dart.core::Object::toString}() in #t1;
+  ^^^^" in let final void #t2 = #t1.{core::Object::toString}() in #t1;
 }

pkg/front_end/testcases/regress/issue_31181.dart.outline.expect

@@ -2,7 +2,7 @@ library;
 import self as self;
 import "dart:core" as core;
 
-typedef Foo<invariant T extends core::Object* = dynamic> = <T extends core::Object* = dynamic>(T*) →* T*;
+typedef Foo<unrelated T extends core::Object* = dynamic> = <T extends core::Object* = dynamic>(T*) →* T*;
 static field <T extends core::Object* = dynamic>(T*) →* T* x;
 static method main() → dynamic
   ;

pkg/front_end/testcases/regress/issue_31213.dart.outline.expect

@@ -3,7 +3,7 @@ import self as self;
 import "dart:core" as core;
 
 typedef C<contravariant A extends core::Object* = dynamic, contravariant K extends core::Object* = dynamic> = <B extends core::Object* = dynamic>(A*, K*, B*) →* core::int*;
-typedef D<unrelated K extends core::Object* = dynamic> = <A extends core::Object* = dynamic>(core::int*) →* <B extends core::Object* = dynamic>(A*, K*, B*) →* core::int*;
+typedef D<contravariant K extends core::Object* = dynamic> = <A extends core::Object* = dynamic>(core::int*) →* <B extends core::Object* = dynamic>(A*, K*, B*) →* core::int*;
 static method producer<K extends core::Object* = dynamic>() → dynamic
   ;
 static method main() → dynamic

pkg/front_end/testcases/runtime_checks/contravariant_generic_return_tear_off.dart.outline.expect

@@ -3,7 +3,7 @@ import self as self;
 import "dart:core" as core;
 
 typedef F<contravariant T extends core::Object* = dynamic> = (T*) →* void;
-typedef G<unrelated T extends core::Object* = dynamic> = () →* (T*) →* void;
+typedef G<contravariant T extends core::Object* = dynamic> = () →* (T*) →* void;
 class C<T extends core::Object* = dynamic> extends core::Object {
   field (self::C::T*) →* void _x;
   constructor •((self::C::T*) →* void _x) → self::C<self::C::T*>*

pkg/kernel/lib/ast.dart

@@ -2742,6 +2742,7 @@ class PropertyGet extends Expression {
 
   visitChildren(Visitor v) {
     receiver?.accept(v);
+    interfaceTarget?.acceptReference(v);
     name?.accept(v);
   }
 
@@ -2790,6 +2791,7 @@ class PropertySet extends Expression {
 
   visitChildren(Visitor v) {
     receiver?.accept(v);
+    interfaceTarget?.acceptReference(v);
     name?.accept(v);
     value?.accept(v);
   }
@@ -2989,6 +2991,7 @@ class SuperPropertyGet extends Expression {
       v.visitSuperPropertyGet(this, arg);
 
   visitChildren(Visitor v) {
+    interfaceTarget?.acceptReference(v);
     name?.accept(v);
   }
 
@@ -3027,6 +3030,7 @@ class SuperPropertySet extends Expression {
       v.visitSuperPropertySet(this, arg);
 
   visitChildren(Visitor v) {
+    interfaceTarget?.acceptReference(v);
     name?.accept(v);
     value?.accept(v);
   }
@@ -3258,6 +3262,7 @@ class MethodInvocation extends InvocationExpression {
 
   visitChildren(Visitor v) {
     receiver?.accept(v);
+    interfaceTarget?.acceptReference(v);
     name?.accept(v);
     arguments?.accept(v);
   }
@@ -3314,6 +3319,7 @@ class SuperMethodInvocation extends InvocationExpression {
       v.visitSuperMethodInvocation(this, arg);
 
   visitChildren(Visitor v) {
+    interfaceTarget?.acceptReference(v);
     name?.accept(v);
     arguments?.accept(v);
   }

pkg/pkg.status

@@ -220,6 +220,9 @@ analyzer/test/src/task/strong/front_end_inference_test: Slow
 [ $arch != x64 || $compiler != none || $mode != release || $runtime != vm ]
 front_end/test/whole_program_test: SkipByDesign
 
+[ $mode == debug || $runtime != vm || $system == android ]
+vm/test/modular_kernel_plus_aot_test: SkipByDesign # This test should only run if binary is run from build dir
+
 [ $mode != release || $runtime != vm ]
 front_end/test/fasta/*: Skip
 front_end/tool/_fasta/*: Skip

pkg/vm/lib/transformations/mixin_deduplication.dart

@@ -9,7 +9,28 @@ import 'package:kernel/ast.dart';
 /// De-duplication of identical mixin applications.
 void transformComponent(Component component) {
   final deduplicateMixins = new DeduplicateMixinsTransformer();
+  final interfaceTargetResolver = InterfaceTargetResolver(deduplicateMixins);
+
+  // Deduplicate mixins and re-resolve super initializers.
+  // (this is a shallow transformation)
   component.libraries.forEach(deduplicateMixins.visitLibrary);
+
+  // Do a deep transformation to re-resolve all interface targets that point to
+  // members of removed mixin application classes.
+
+  // This is necessary iff the component was assembled from individual modular
+  // kernel compilations:
+  //
+  //   * if the CFE reads in the entire program as source, interface targets
+  //     will point to the original mixin class
+  //
+  //   * if the CFE reads in dependencies as kernel, interface targets will
+  //     point to the already existing mixin application classes.
+  //
+  // TODO(dartbug.com/39375): Remove this extra O(N) pass over the AST if the
+  // CFE decides to consistently let the interface target point to the mixin
+  // class (instead of mixin application).
+  component.libraries.forEach(interfaceTargetResolver.visitLibrary);
 }
 
 class _DeduplicateMixinKey {
@@ -96,7 +117,6 @@ class DeduplicateMixinsTransformer extends Transformer {
     if (canonical != c) {
       c.canonicalName?.unbind();
       _duplicatedMixins[c] = canonical;
-      // print('Replacing $c with $canonical');
       return null; // Remove class.
     }
 
@@ -128,6 +148,73 @@ class DeduplicateMixinsTransformer extends Transformer {
       throw 'Unexpected node ${node.runtimeType}: $node';
 }
 
+/// Rewrites interface targets to point to the deduplicated mixin application
+/// class.
+class InterfaceTargetResolver extends RecursiveVisitor<TreeNode> {
+  final DeduplicateMixinsTransformer transformer;
+
+  InterfaceTargetResolver(this.transformer);
+
+  defaultTreeNode(TreeNode node) {
+    node.visitChildren(this);
+    return node;
+  }
+
+  visitPropertyGet(PropertyGet node) {
+    node.interfaceTarget = _resolveNewInterfaceTarget(node.interfaceTarget);
+    return super.visitPropertyGet(node);
+  }
+
+  visitPropertySet(PropertySet node) {
+    node.interfaceTarget = _resolveNewInterfaceTarget(node.interfaceTarget);
+    return super.visitPropertySet(node);
+  }
+
+  visitMethodInvocation(MethodInvocation node) {
+    node.interfaceTarget = _resolveNewInterfaceTarget(node.interfaceTarget);
+    return super.visitMethodInvocation(node);
+  }
+
+  visitSuperPropertyGet(SuperPropertyGet node) {
+    node.interfaceTarget = _resolveNewInterfaceTarget(node.interfaceTarget);
+    return super.visitSuperPropertyGet(node);
+  }
+
+  visitSuperPropertySet(SuperPropertySet node) {
+    node.interfaceTarget = _resolveNewInterfaceTarget(node.interfaceTarget);
+    return super.visitSuperPropertySet(node);
+  }
+
+  visitSuperMethodInvocation(SuperMethodInvocation node) {
+    node.interfaceTarget = _resolveNewInterfaceTarget(node.interfaceTarget);
+    return super.visitSuperMethodInvocation(node);
+  }
+
+  Member _resolveNewInterfaceTarget(Member m) {
+    final Class c = m?.enclosingClass;
+    if (c != null && c.isAnonymousMixin) {
+      final Class replacement = transformer._duplicatedMixins[c];
+      if (replacement != null) {
+        // The class got removed, so we need to re-resolve the interface target.
+        return _findMember(replacement, m);
+      }
+    }
+    return m;
+  }
+
+  Member _findMember(Class klass, Member m) {
+    if (m is Field) {
+      return klass.members.where((other) => other.name == m.name).single;
+    } else if (m is Procedure) {
+      return klass.procedures
+          .where((other) => other.kind == m.kind && other.name == m.name)
+          .single;
+    } else {
+      throw 'Hit unexpected interface target which is not a Field/Procedure';
+    }
+  }
+}
+
 /// Corrects forwarding constructors inserted by mixin resolution after
 /// replacing superclass.
 void _correctForwardingConstructors(Class c, Class oldSuper, Class newSuper) {

pkg/vm/lib/transformations/type_flow/transformer.dart

@@ -653,7 +653,7 @@ class _TreeShakerPass1 extends Transformer {
         if (shaker.isFieldInitializerReachable(node)) {
           node.transformChildren(this);
         } else {
-          node.initializer = _makeUnreachableCall([]);
+          node.initializer = _makeUnreachableCall([])..parent = node;
         }
       }
     } else if (shaker.isMemberReferencedFromNativeCode(node)) {

pkg/vm/test/modular_kernel_plus_aot_test.dart

@@ -0,0 +1,163 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:async';
+import 'dart:io';
+import 'dart:typed_data';
+
+import 'package:front_end/src/api_unstable/bazel_worker.dart' as fe;
+import 'package:front_end/src/api_prototype/standard_file_system.dart';
+import 'package:kernel/target/targets.dart';
+import 'package:kernel/kernel.dart';
+import 'package:kernel/verifier.dart';
+import 'package:vm/target/vm.dart';
+import 'package:vm/kernel_front_end.dart';
+
+main() async {
+  Uri sdkSummary =
+      sdkRootFile(Platform.executable).resolve('vm_platform_strong.dill');
+  if (!await File.fromUri(sdkSummary).exists()) {
+    // If we run from the <build-dir>/dart-sdk/bin folder, we need to navigate two
+    // levels up.
+    sdkSummary = sdkRootFile(Platform.executable)
+        .resolve('../../vm_platform_strong.dill');
+  }
+
+  // Tests are run in the root directory of the sdk checkout.
+  final Uri packagesFile = sdkRootFile('.packages');
+  final Uri librariesFile = sdkRootFile('sdk/lib/libraries.json');
+
+  final vmTarget = VmTarget(TargetFlags());
+
+  await withTempDirectory((Uri uri) async {
+    final mixinFilename = uri.resolve('mixin.dart');
+    final mixinDillFilename = uri.resolve('mixin.dart.dill');
+    await File.fromUri(mixinFilename).writeAsStringSync(mixinFile);
+
+    await compileToKernel(vmTarget, librariesFile, sdkSummary, packagesFile,
+        mixinDillFilename, <Uri>[mixinFilename], <Uri>[], <Uri>[]);
+
+    final mainFilename = uri.resolve('main.dart');
+    final mainDillFilename = uri.resolve('main.dart.dill');
+    await File.fromUri(mainFilename).writeAsStringSync(mainFile);
+
+    await compileToKernel(
+        vmTarget,
+        librariesFile,
+        sdkSummary,
+        packagesFile,
+        mainDillFilename,
+        <Uri>[mainFilename],
+        <Uri>[mixinDillFilename],
+        <Uri>[]);
+
+    final bytes = concat(
+        await File.fromUri(sdkSummary).readAsBytes(),
+        concat(await File.fromUri(mixinDillFilename).readAsBytes(),
+            await File.fromUri(mainDillFilename).readAsBytes()));
+    final component = loadComponentFromBytes(bytes);
+
+    // Verify before running global transformations.
+    verifyComponent(component, isOutline: false, afterConst: true);
+
+    const useGlobalTypeFlowAnalysis = true;
+    const enableAsserts = false;
+    const useProtobufTreeShaker = false;
+    await runGlobalTransformations(
+        vmTarget,
+        component,
+        useGlobalTypeFlowAnalysis,
+        enableAsserts,
+        useProtobufTreeShaker,
+        ErrorDetector());
+
+    // Verify after running global transformations.
+    verifyComponent(component, isOutline: false, afterConst: true);
+  });
+}
+
+Future compileToKernel(
+    Target target,
+    Uri librariesFile,
+    Uri sdkSummary,
+    Uri packagesFile,
+    Uri outputFile,
+    List<Uri> sources,
+    List<Uri> linkedInputs,
+    List<Uri> summaryInputs) async {
+  final state = await fe.initializeCompiler(
+      null,
+      sdkSummary,
+      librariesFile,
+      packagesFile,
+      summaryInputs,
+      linkedInputs,
+      target,
+      StandardFileSystem.instance, const <String>[], const <String, String>{});
+
+  void onDiagnostic(fe.DiagnosticMessage message) {
+    print(message);
+  }
+
+  final Component component =
+      await fe.compileComponent(state, sources, onDiagnostic);
+  final Uint8List kernel = fe.serializeComponent(component,
+      filter: (library) => sources.contains(library.importUri));
+  await File(outputFile.toFilePath()).writeAsBytes(kernel);
+}
+
+Future withTempDirectory(Future func(Uri dirUri)) async {
+  final dir = await Directory.systemTemp.createTemp('modular-compile-test');
+  try {
+    await func(dir.uri);
+  } finally {
+    await dir.delete(recursive: true);
+  }
+}
+
+Uint8List concat(List<int> a, List<int> b) {
+  final bytes = Uint8List(a.length + b.length);
+  bytes.setRange(0, a.length, a);
+  bytes.setRange(a.length, bytes.length, b);
+  return bytes;
+}
+
+Uri sdkRootFile(name) => Directory.current.uri.resolve(name);
+
+const String mainFile = r'''
+import 'mixin.dart';
+class R extends A2 {
+  void bar() {
+    mixinProperty = '';
+    mixinProperty .foo();
+    mixinMethod('').foo();
+    super.mixinProperty= '';
+    super.mixinProperty.foo();
+    super.mixinMethod('').foo();
+  }
+}
+
+main() {
+  A1();
+  final a2 = A2();
+  // The mixin deduplication will remove the anonymous mixin application class
+  // from `A2 & Mixin` and instead use the one from `A1 & Mixin`.
+  a2.mixinProperty= '';
+  a2.mixinProperty.foo();
+  a2.mixinMethod('').foo();
+}
+''';
+
+const String mixinFile = r'''
+class Foo {
+  foo() {}
+}
+class Mixin {
+  void set mixinProperty(v) {}
+  Foo get mixinProperty{}
+  Foo mixinMethod(v) {}
+}
+class A1 extends Object with Mixin { }
+class A2 extends Object with Mixin { }
+''';