commit 017a1f4effa90474443ce06f659ca76d55ac05b2
author Paul Berry <paulberry@google.com> Tue Aug 29 14:46:12 2017 -0700
committer Paul Berry <paulberry@google.com> Tue Aug 29 14:46:15 2017 -0700
tree 7b206d3a4f80c7f6b8135170523f58b5c4a7b11e
parent 39f2dce9d501b193b6dd8e0e892e2897951f6429

Revert "Fix forEachOverridePair in the case where the class in question is abstract."

This reverts commit e81deebfd8ce007af285304174027261270f7b11.

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 e81deebfd8ce007af285304174027261270f7b11,
forEachOverridePair(B) would not yield this pair, but
forEachOverridePair(C) would.  After
e81deebfd8ce007af285304174027261270f7b11, both calls yield this pair.

When I made e81deebfd8ce007af285304174027261270f7b11, 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 e81deebfd8ce007af285304174027261270f7b11, it modifies it to
demonstrate why the old behavior was correct.

R=scheglov@google.com

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