Revert "Fix forEachOverridePair in the case where the class in questi…

…on is abstract."

This reverts commit e81deeb.

My reasoning in the above commit was wrong.  Consider the following code:

class A {
  void foo() {}
}
abstract class B extends A {
  void foo([x]);
}
class C extends B {}
main() {
  B b = new C();
  b.foo(42); // BAD: A.foo can't accept arguments.
}

To ensure soundness, this code needs to be disallowed, and the current
mechanism for doing that is to use forEachOverridePair.  Note that
forEachOverridePair is a bit of a misnomer; in addition to yielding
all pairs of methods (M1, M2) for which M1 overrides M2, it also
yields pairs of methods (M1, M2) for which the target class inherits
the concrete implementation M1, and M2 is part of the interface.
Technically this latter case is not an "override" but rather an
"implementation" (thanks to Lasse for pointing out this distinction).
However in both cases we need to do the same compile-time check to
ensure soundness: we need to check that the type of M1 is a subtype of
M2 (unless the check is suppressed by a "covariant" keyword).  Hence
it makes sense for forEachOverridePair to cover both cases.

To ensure that the above example is properly rejected, it is crucial
that some invocation of forEachOverridePair yield the pair (A.foo,
B.foo).  Prior to e81deeb,
forEachOverridePair(B) would not yield this pair, but
forEachOverridePair(C) would.  After
e81deeb, both calls yield this pair.

When I made e81deeb, I failed to
notice that forEachOverridePair(C) would yield the pair, so I thought
there was a problem.  So my "fix" was unnecessary.  And it created a
fresh problem: it meant that the following code would be disallowed:

class A {
  void foo() {}
}
abstract class B extends A {
  void foo([x]);
}
class C extends B {
  void foo([x]) {}
}
main() {
  B b = new C();
  b.foo(42); // OK: C.foo can accept an argument.
}

There is no a priori soundness reason for rejecting this code, and
according to Lasse, it has not yet been decided whether Dart 2.0 will
allow it.

This CL restores the old behavior.  Rather than remove the test case
in e81deeb, it modifies it to
demonstrate why the old behavior was correct.

R=scheglov@google.com

Review-Url: https://codereview.chromium.org/3004023002 .

pkg/kernel/lib/class_hierarchy.dart

@@ -417,18 +417,20 @@ class ClosedWorldClassHierarchy implements ClassHierarchy {
       _reportOverrides(info.declaredSetters, superSetters, callback,
           isSetter: true, onlyAbstract: true);
     }
-    // If a class declares an abstract method M whose
-    // implementation M' is inherited from the superclass, then the inherited
-    // method M' overrides the declared method M.
-    // This flies in the face of conventional override logic, but is necessary
-    // because an instance of the class will contain the method M' which can
-    // be invoked through the interface of M.
-    // Note that [_reportOverrides] does not report self-overrides, so in
-    // most cases these calls will just scan both lists and report nothing.
-    _reportOverrides(info.implementedGettersAndCalls,
-        info.declaredGettersAndCalls, callback);
-    _reportOverrides(info.implementedSetters, info.declaredSetters, callback,
-        isSetter: true);
+    if (!class_.isAbstract) {
+      // If a non-abstract class declares an abstract method M whose
+      // implementation M' is inherited from the superclass, then the inherited
+      // method M' overrides the declared method M.
+      // This flies in the face of conventional override logic, but is necessary
+      // because an instance of the class will contain the method M' which can
+      // be invoked through the interface of M.
+      // Note that [_reportOverrides] does not report self-overrides, so in
+      // most cases these calls will just scan both lists and report nothing.
+      _reportOverrides(info.implementedGettersAndCalls,
+          info.declaredGettersAndCalls, callback);
+      _reportOverrides(info.implementedSetters, info.declaredSetters, callback,
+          isSetter: true);
+    }
   }
 
   @override

pkg/kernel/lib/src/incremental_class_hierarchy.dart

@@ -104,18 +104,20 @@ class IncrementalClassHierarchy implements ClassHierarchy {
       _reportOverrides(info.declaredSetters, superSetters, callback,
           isSetter: true, onlyAbstract: true);
     }
-    // If a class declares an abstract method M whose
-    // implementation M' is inherited from the superclass, then the inherited
-    // method M' overrides the declared method M.
-    // This flies in the face of conventional override logic, but is necessary
-    // because an instance of the class will contain the method M' which can
-    // be invoked through the interface of M.
-    // Note that [_reportOverrides] does not report self-overrides, so in
-    // most cases these calls will just scan both lists and report nothing.
-    _reportOverrides(info.implementedGettersAndCalls,
-        info.declaredGettersAndCalls, callback);
-    _reportOverrides(info.implementedSetters, info.declaredSetters, callback,
-        isSetter: true);
+    if (!node.isAbstract) {
+      // If a non-abstract class declares an abstract method M whose
+      // implementation M' is inherited from the superclass, then the inherited
+      // method M' overrides the declared method M.
+      // This flies in the face of conventional override logic, but is necessary
+      // because an instance of the class will contain the method M' which can
+      // be invoked through the interface of M.
+      // Note that [_reportOverrides] does not report self-overrides, so in
+      // most cases these calls will just scan both lists and report nothing.
+      _reportOverrides(info.implementedGettersAndCalls,
+          info.declaredGettersAndCalls, callback);
+      _reportOverrides(info.implementedSetters, info.declaredSetters, callback,
+          isSetter: true);
+    }
   }
 
   @override

pkg/kernel/test/class_hierarchy_test.dart

@@ -304,6 +304,8 @@ class E = self::D with self::A implements self::B {}
         supertype: a.asThisSupertype,
         procedures: [newEmptyMethod('foo', isAbstract: true)],
         isAbstract: true));
+    var d = addClass(new Class(name: 'D', supertype: b.asThisSupertype));
+    var e = addClass(new Class(name: 'E', supertype: c.asThisSupertype));
 
     _assertTestLibraryText('''
 class A {
@@ -315,16 +317,17 @@ class B extends self::A {
 abstract class C extends self::A {
   abstract method foo() → void;
 }
+class D extends self::B {}
+class E extends self::C {}
 ''');
 
     _assertOverridePairs(b, [
       'test::A::foo overrides test::B::foo',
       'test::B::foo overrides test::A::foo'
     ]);
-    _assertOverridePairs(c, [
-      'test::A::foo overrides test::C::foo',
-      'test::C::foo overrides test::A::foo'
-    ]);
+    _assertOverridePairs(c, ['test::C::foo overrides test::A::foo']);
+    _assertOverridePairs(d, ['test::A::foo overrides test::B::foo']);
+    _assertOverridePairs(e, ['test::A::foo overrides test::C::foo']);
   }
 
   /// 3. A non-abstract member is inherited from a superclass, and it overrides