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

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

Committed: https://github.com/dart-lang/sdk/commit/017a1f4effa90474443ce06f659ca76d55ac05b2

[Paul Berry, 2017-08-29 20:44:32]

paulberry@google.com changed reviewers:
+ lrn@google.com, scheglov@google.com

[scheglov, 2017-08-29 20:50:44]

LGTM

[Paul Berry, 2017-08-29 21:46:19]

Description was changed from

==========
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.
==========

to

==========
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

Committed:
https://github.com/dart-lang/sdk/commit/017a1f4effa90474443ce06f659ca76d55ac05b2
==========

[Paul Berry, 2017-08-29 21:46:20]

Committed patchset #1 (id:1) manually as
017a1f4effa90474443ce06f659ca76d55ac05b2 (presubmit successful).