[fasta] Don't generate noSuchMethod forwarders in abstract classes

Bug: http://dartbug.com/33482
Change-Id: I4c9b2e9a43fdcd37238ede5814b2ad471f7b2c4d
Reviewed-on: https://dart-review.googlesource.com/60847
Commit-Queue: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>

pkg/front_end/lib/src/fasta/kernel/kernel_class_builder.dart

@@ -385,13 +385,10 @@ abstract class KernelClassBuilder
     }
   }
 
-  // TODO(dmitryas): Find a better place for this routine.
-  static bool hasUserDefinedNoSuchMethod(
-      Class klass, ClassHierarchy hierarchy) {
+  bool hasUserDefinedNoSuchMethod(
+      Class klass, ClassHierarchy hierarchy, Class objectClass) {
     Member noSuchMethod = hierarchy.getDispatchTarget(klass, noSuchMethodName);
-    // `Object` doesn't have a superclass reference.
-    return noSuchMethod != null &&
-        noSuchMethod.enclosingClass.superclass != null;
+    return noSuchMethod != null && noSuchMethod.enclosingClass != objectClass;
   }
 
   void transformProcedureToNoSuchMethodForwarder(
@@ -439,57 +436,47 @@ abstract class KernelClassBuilder
   /// class was modified.
   bool addNoSuchMethodForwarders(
       KernelTarget target, ClassHierarchy hierarchy) {
-    if (!hasUserDefinedNoSuchMethod(cls, hierarchy)) {
+    if (cls.isAbstract ||
+        !hasUserDefinedNoSuchMethod(cls, hierarchy, target.objectClass)) {
       return false;
     }
 
     Set<Name> existingForwardersNames = new Set<Name>();
     Set<Name> existingSetterForwardersNames = new Set<Name>();
-    if (cls.superclass != null &&
-        hasUserDefinedNoSuchMethod(cls.superclass, hierarchy)) {
-      List<Member> concrete = hierarchy.getDispatchTargets(cls.superclass);
-      for (Member member in hierarchy.getInterfaceMembers(cls.superclass)) {
+    Class leastConcreteSuperclass = cls.superclass;
+    while (
+        leastConcreteSuperclass != null && leastConcreteSuperclass.isAbstract) {
+      leastConcreteSuperclass = leastConcreteSuperclass.superclass;
+    }
+    if (leastConcreteSuperclass != null &&
+        hasUserDefinedNoSuchMethod(
+            leastConcreteSuperclass, hierarchy, target.objectClass)) {
+      List<Member> concrete =
+          hierarchy.getDispatchTargets(leastConcreteSuperclass);
+      for (Member member
+          in hierarchy.getInterfaceMembers(leastConcreteSuperclass)) {
         if (ClassHierarchy.findMemberByName(concrete, member.name) == null) {
           existingForwardersNames.add(member.name);
         }
       }
 
       List<Member> concreteSetters =
-          hierarchy.getDispatchTargets(cls.superclass, setters: true);
-      for (Member member
-          in hierarchy.getInterfaceMembers(cls.superclass, setters: true)) {
+          hierarchy.getDispatchTargets(leastConcreteSuperclass, setters: true);
+      for (Member member in hierarchy
+          .getInterfaceMembers(leastConcreteSuperclass, setters: true)) {
         if (ClassHierarchy.findMemberByName(concreteSetters, member.name) ==
             null) {
           existingSetterForwardersNames.add(member.name);
         }
       }
     }
-    if (cls.mixedInClass != null &&
-        hasUserDefinedNoSuchMethod(cls.mixedInClass, hierarchy)) {
-      List<Member> concrete = hierarchy.getDispatchTargets(cls.mixedInClass);
-      for (Member member in hierarchy.getInterfaceMembers(cls.mixedInClass)) {
-        if (ClassHierarchy.findMemberByName(concrete, member.name) == null) {
-          existingForwardersNames.add(member.name);
-        }
-      }
-
-      List<Member> concreteSetters =
-          hierarchy.getDispatchTargets(cls.superclass, setters: true);
-      for (Member member
-          in hierarchy.getInterfaceMembers(cls.superclass, setters: true)) {
-        if (ClassHierarchy.findMemberByName(concreteSetters, member.name) ==
-            null) {
-          existingSetterForwardersNames.add(member.name);
-        }
-      }
-    }
-
-    List<Member> concrete = hierarchy.getDispatchTargets(cls);
-    List<Member> declared = hierarchy.getDeclaredMembers(cls);
 
     Member noSuchMethod = ClassHierarchy.findMemberByName(
         hierarchy.getInterfaceMembers(cls), noSuchMethodName);
 
+    List<Member> concrete = hierarchy.getDispatchTargets(cls);
+    List<Member> declared = hierarchy.getDeclaredMembers(cls);
+
     bool changed = false;
     for (Member member in hierarchy.getInterfaceMembers(cls)) {
       if (member is Procedure &&

pkg/front_end/testcases/compile.status

@@ -125,3 +125,8 @@ incomplete_field_formal_parameter: Fail # Fasta doesn't recover well
 
 co19_language_metadata_syntax_t04: RuntimeError # Fasta doesn't recover well
 external_import: RuntimeError # Expected -- test uses import which doesn't exist.
+
+no_such_method_forwarders/forwarders_not_assumed_from_mixin: Fail
+no_such_method_forwarders/no_forwarders_for_abstract_classes: Fail
+no_such_method_forwarders/no_forwarders_for_abstract_classes_chain: Fail
+no_such_method_forwarders/concrete_method_over_forwarder_in_mixin_application: Fail

pkg/front_end/testcases/no_such_method_forwarders/concrete_method_over_forwarder_in_mixin_application.dart

@@ -0,0 +1,23 @@
+// Copyright (c) 2018, 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.
+
+// This test checks that when both LHS and RHS in a mixin application have a
+// concrete implementation of a method and a noSuchMethod forwarder for it, the
+// concrete implementation stays.
+
+abstract class I {
+  foo();
+}
+
+class A {
+  foo() {}
+}
+
+class B implements I {
+  noSuchMethod(_) => null;
+}
+
+class C extends A with B {}
+
+main() {}

pkg/front_end/testcases/no_such_method_forwarders/forwarders_not_assumed_from_mixin.dart

@@ -2,8 +2,9 @@
 // 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.
 
-// This test checks that the noSuchMethod forwarder is not duplicated in classes
-// that mix in the classes that already have the forwarder.
+// This test checks that the noSuchMethod forwarder is not assumed to be mixed
+// in and is generated for classes that mix in a user-defined noSuchMethod and
+// have an abstract method.
 
 abstract class I {
   void foo();

pkg/front_end/testcases/no_such_method_forwarders/no_dup_from_mixin.dart.direct.expect

@@ -1,27 +0,0 @@
-library;
-import self as self;
-import "dart:core" as core;
-
-abstract class I extends core::Object {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  abstract method foo() → void;
-}
-class A extends core::Object implements self::I {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  method noSuchMethod(core::Invocation i) → dynamic
-    return null;
-  no-such-method-forwarder method foo() → void
-    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("foo", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
-}
-abstract class _B&Object&A = core::Object with self::A {
-}
-class B extends self::_B&Object&A {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-}
-static method main() → dynamic {}

pkg/front_end/testcases/no_such_method_forwarders/no_dup_from_mixin.dart.direct.transformed.expect

@@ -1,34 +0,0 @@
-library;
-import self as self;
-import "dart:core" as core;
-
-abstract class I extends core::Object {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  abstract method foo() → void;
-}
-class A extends core::Object implements self::I {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  method noSuchMethod(core::Invocation i) → dynamic
-    return null;
-  no-such-method-forwarder method foo() → void
-    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("foo", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
-}
-abstract class _B&Object&A extends core::Object implements self::A {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  method noSuchMethod(core::Invocation i) → dynamic
-    return null;
-  no-such-method-forwarder method foo() → void
-    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("foo", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
-}
-class B extends self::_B&Object&A {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-}
-static method main() → dynamic {}

pkg/front_end/testcases/no_such_method_forwarders/no_dup_from_mixin.dart.outline.expect

@@ -1,25 +0,0 @@
-library;
-import self as self;
-import "dart:core" as core;
-
-abstract class I extends core::Object {
-  synthetic constructor •() → void
-    ;
-  abstract method foo() → void;
-}
-class A extends core::Object implements self::I {
-  synthetic constructor •() → void
-    ;
-  method noSuchMethod(core::Invocation i) → dynamic
-    ;
-  no-such-method-forwarder method foo() → void
-    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("foo", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
-}
-abstract class _B&Object&A = core::Object with self::A {
-}
-class B extends self::_B&Object&A {
-  synthetic constructor •() → void
-    ;
-}
-static method main() → dynamic
-  ;

pkg/front_end/testcases/no_such_method_forwarders/no_dup_from_mixin.dart.strong.expect

@@ -1,27 +0,0 @@
-library;
-import self as self;
-import "dart:core" as core;
-
-abstract class I extends core::Object {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  abstract method foo() → void;
-}
-class A extends core::Object implements self::I {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  method noSuchMethod(core::Invocation i) → dynamic
-    return null;
-  no-such-method-forwarder method foo() → void
-    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("foo", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
-}
-abstract class _B&Object&A = core::Object with self::A {
-}
-class B extends self::_B&Object&A {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-}
-static method main() → dynamic {}

pkg/front_end/testcases/no_such_method_forwarders/no_dup_from_mixin.dart.strong.transformed.expect

@@ -1,34 +0,0 @@
-library;
-import self as self;
-import "dart:core" as core;
-
-abstract class I extends core::Object {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  abstract method foo() → void;
-}
-class A extends core::Object implements self::I {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  method noSuchMethod(core::Invocation i) → dynamic
-    return null;
-  no-such-method-forwarder method foo() → void
-    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("foo", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
-}
-abstract class _B&Object&A extends core::Object implements self::A {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-  method noSuchMethod(core::Invocation i) → dynamic
-    return null;
-  no-such-method-forwarder method foo() → void
-    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("foo", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
-}
-class B extends self::_B&Object&A {
-  synthetic constructor •() → void
-    : super core::Object::•()
-    ;
-}
-static method main() → dynamic {}

pkg/front_end/testcases/no_such_method_forwarders/no_forwarders_for_abstract_classes.dart

@@ -0,0 +1,20 @@
+// Copyright (c) 2018, 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.
+
+// This test checks that the noSuchMethod forwarders aren't generated for the
+// abstract classes that have user-defined noSuchMethod, but rather in their
+// concrete descendants.
+
+abstract class A {
+  noSuchMethod(i) => null;
+
+  // The forwarder for [foo] shouldn't be generated here.
+  void foo();
+}
+
+class B extends A {
+  // The forwarder for [foo] should be generated here.
+}
+
+main() {}

pkg/front_end/testcases/no_such_method_forwarders/no_forwarders_for_abstract_classes_chain.dart

@@ -0,0 +1,29 @@
+// Copyright (c) 2018, 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.
+
+// This test checks that the noSuchMethod forwarders aren't generated for the
+// abstract classes that have user-defined noSuchMethod, but rather in their
+// top-most concrete descendants.  The immediate abstract children should not
+// receive the forwarders.
+
+abstract class A {
+  noSuchMethod(i) => null;
+
+  // The forwarder for [foo] shouldn't be generated here.
+  void foo();
+}
+
+abstract class B extends A {
+  // [B] shouldn't receive a forwarder.
+}
+
+class C extends B {
+  // The forwarder for [foo] should be generated here.
+}
+
+class D extends C {
+  // [D] shouldn't receiver a forwarder.
+}
+
+main() {}

pkg/front_end/testcases/outline.status

@@ -252,3 +252,8 @@ rasta/issue_000046: Fail
 rasta/issue_000047: Fail
 rasta/native_is_illegal: Pass # Issue 29763
 rasta/type_with_parse_error: Fail
+
+no_such_method_forwarders/forwarders_not_assumed_from_mixin: Fail
+no_such_method_forwarders/no_forwarders_for_abstract_classes: Fail
+no_such_method_forwarders/no_forwarders_for_abstract_classes_chain: Fail
+no_such_method_forwarders/concrete_method_over_forwarder_in_mixin_application: Fail

pkg/front_end/testcases/strong.status

@@ -221,3 +221,8 @@ incomplete_field_formal_parameter: Fail # Fasta doesn't recover well
 co19_language_metadata_syntax_t04: RuntimeError # Fasta doesn't recover well
 
 external_import: RuntimeError # The native extension to import doesn't exist. This is ok.
+
+no_such_method_forwarders/forwarders_not_assumed_from_mixin: Fail
+no_such_method_forwarders/no_forwarders_for_abstract_classes: Fail
+no_such_method_forwarders/no_forwarders_for_abstract_classes_chain: Fail
+no_such_method_forwarders/concrete_method_over_forwarder_in_mixin_application: Fail

tests/language_2/language_2_dartdevc.status

@@ -553,6 +553,9 @@ string_split_test: CompileTimeError # Issue 31616
 string_supertype_checked_test: CompileTimeError # Issue 31616
 super_bound_closure_test/none: CompileTimeError # Issue 31533
 super_call4_test/01: MissingCompileTimeError
+super_operator_index6_test: CompileTimeError # Issue 33498
+super_operator_index7_test: CompileTimeError # Issue 33498
+super_operator_index8_test: CompileTimeError # Issue 33498
 switch_bad_case_test/01: MissingCompileTimeError
 switch_bad_case_test/02: MissingCompileTimeError
 switch_case_test/00: MissingCompileTimeError

tests/language_2/language_2_kernel.status

@@ -655,6 +655,9 @@ vm/type_vm_test/35: MissingRuntimeError
 vm/type_vm_test/36: MissingRuntimeError
 
 [ $compiler == dartk && $runtime == vm ]
+super_operator_index6_test: CompileTimeError # Issue 33498
+super_operator_index7_test: CompileTimeError # Issue 33498
+super_operator_index8_test: CompileTimeError # Issue 33498
 type_alias_equality_test/02: RuntimeError # Issue 31359
 type_alias_equality_test/03: RuntimeError # Issue 31359
 type_alias_equality_test/04: RuntimeError # Issue 31359
@@ -1387,6 +1390,9 @@ setter_override_test/03: MissingCompileTimeError
 string_split_test: CompileTimeError
 string_supertype_checked_test: CompileTimeError
 super_bound_closure_test/none: CompileTimeError
+super_operator_index6_test: CompileTimeError # Issue 33498
+super_operator_index7_test: CompileTimeError # Issue 33498
+super_operator_index8_test: CompileTimeError # Issue 33498
 try_catch_test/01: MissingCompileTimeError
 type_promotion_functions_test/02: CompileTimeError
 type_promotion_functions_test/03: CompileTimeError