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

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

ClassHierarchy.forEachOverridePair contains special logic for unusual
cases like this one:

class A {
  void foo() {}
}
class B extends A {
  void foo();
}
main() {
  B b = new B();
  b.foo();
}

In this case, A.foo is considered to override B.foo (contrary to the
usual situation where the derived class method overrides the
superclass method).  The reasoning is that calling foo on a concrete
instance of B will cause A.foo to be executed (as illustrated in
main); therefore A.foo is callable via the interface of B.foo, thus in
a sense A.foo "overrides" B.foo.

The code contained a questionable optimization, however; it only
executed this special logic if the derived class was concrete.
Presuambly the reasoning was that if B were abstract, then a concrete
instance of B could never be created, so this situation could never
arise.

However, there is nothing to stop a concrete class from being derived
from B, e.g.:

class A {
  void foo() {}
}
abstract class B extends A {
  void foo();
}
class C extends B {}
main() {
  B b = new C();
  b.foo();
}

Now, calling foo on a concrete instance of C will cause A.foo to be
executed (as illustrated in main); therefore A.foo is callable via the
interface of B.foo, as before.  So we still need to report this as an
override pair even though B is abstract.

R=ahe@google.com, scheglov@google.com

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

[Paul Berry, 2017-08-25 21:32:12]

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

[Paul Berry, 2017-08-25 21:32:13]

Konstantin, can you give me an initial review since you're in my time zone?

I'll wait for final approval from Kevin before landing.

[scheglov, 2017-08-25 21:53:52]

LGTM

[ahe, 2017-08-28 18:10:58]

ahe@google.com changed reviewers:
+ ahe@google.com

[ahe, 2017-08-28 18:10:58]

LGTM

I can't spot any flaws in your reasoning, and I understand why you're wary about
this. I'm going to add Lasse and Johnni to this CL, just to get their thoughts,
but I think you can submit this.

[Paul Berry, 2017-08-28 18:49:04]

Description was changed from

==========
Fix forEachOverridePair in the case where the class in question is abstract.

ClassHierarchy.forEachOverridePair contains special logic for unusual
cases like this one:

class A {
  void foo() {}
}
class B extends A {
  void foo();
}
main() {
  B b = new B();
  b.foo();
}

In this case, A.foo is considered to override B.foo (contrary to the
usual situation where the derived class method overrides the
superclass method).  The reasoning is that calling foo on a concrete
instance of B will cause A.foo to be executed (as illustrated in
main); therefore A.foo is callable via the interface of B.foo, thus in
a sense A.foo "overrides" B.foo.

The code contained a questionable optimization, however; it only
executed this special logic if the derived class was concrete.
Presuambly the reasoning was that if B were abstract, then a concrete
instance of B could never be created, so this situation could never
arise.

However, there is nothing to stop a concrete class from being derived
from B, e.g.:

class A {
  void foo() {}
}
abstract class B extends A {
  void foo();
}
class C extends B {}
main() {
  B b = new C();
  b.foo();
}

Now, calling foo on a concrete instance of C will cause A.foo to be
executed (as illustrated in main); therefore A.foo is callable via the
interface of B.foo, as before.  So we still need to report this as an
override pair even though B is abstract.
==========

to

==========
Fix forEachOverridePair in the case where the class in question is abstract.

ClassHierarchy.forEachOverridePair contains special logic for unusual
cases like this one:

class A {
  void foo() {}
}
class B extends A {
  void foo();
}
main() {
  B b = new B();
  b.foo();
}

In this case, A.foo is considered to override B.foo (contrary to the
usual situation where the derived class method overrides the
superclass method).  The reasoning is that calling foo on a concrete
instance of B will cause A.foo to be executed (as illustrated in
main); therefore A.foo is callable via the interface of B.foo, thus in
a sense A.foo "overrides" B.foo.

The code contained a questionable optimization, however; it only
executed this special logic if the derived class was concrete.
Presuambly the reasoning was that if B were abstract, then a concrete
instance of B could never be created, so this situation could never
arise.

However, there is nothing to stop a concrete class from being derived
from B, e.g.:

class A {
  void foo() {}
}
abstract class B extends A {
  void foo();
}
class C extends B {}
main() {
  B b = new C();
  b.foo();
}

Now, calling foo on a concrete instance of C will cause A.foo to be
executed (as illustrated in main); therefore A.foo is callable via the
interface of B.foo, as before.  So we still need to report this as an
override pair even though B is abstract.

R=ahe@google.com, scheglov@google.com

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

[Paul Berry, 2017-08-28 18:49:06]

Committed patchset #2 (id:20001) manually as
e81deebfd8ce007af285304174027261270f7b11 (presubmit successful).

[Lasse Reichstein Nielsen, 2017-08-28 19:17:32]

I would never say that `A.foo` overrides `B.foo`. I'd rather say that `B.foo`
*doesn't* override `A.foo` wrt. implementation, only wrt. interface.

That is, I think of a class declaration as defining (at least) two different
entities - the "class" which declares or inherits concrete instance members, and
the "interface" which declares or inherits abstract member signatures. 
A concrete declaration in a class updates both. An abstract declaration only
affects the interface.
Concrete implementation can only be inherited from the superclass, interface
members is inherited from the superclass' interface and from any interfaces in
the implements clause.

So, in this case, *B.foo overrides A.foo* in the interface. *A.foo must
implements B.foo* (because A.foo is the inherited concrete member of B and B is
not abstract).

The implementation must match the interface, at least for concrete classes.
So we test, for any non-abstract class, that the implementation of a member
declared or inherited by a class matches the signature declared or inherited of
that member in the class' interface.

Here it works - the signature of `B.foo` is `void Function()` and the
implementation has type `void Function()`.

It's more interesting when that's not true any more:

class A {
  void foo() {}
}
abstract class I {
  void foo([x]);
}
abstract class B extends A implements I {
}
class C extends B {
  void foo([x]) => super.foo(x);  // Ooops?
}
main() {
  B b = new C();
  b.foo(42);
}

Here B has a problem because it doesn't satisfy its interface. It's abstract, so
in Dart 1 it gets away with that.
(I haven't decided yet whether it can safely get away with it in Dart 2). 

The `super.foo(x)` call is bad. It must be checked against the actual super
implementation of `foo`, not just the static interface of B to see that, but it
must be rejected.
(That's slightly annoying because it means that the relation between a subclass
and an abstract super-class is tighter than just the interface contract, but
avoiding that would mean that we require B to actually override A.foo with a
concrete method, which is an option I'm considering).

All in all, I don't know what forEachOverridePair does, so I'm just waxing
philosophically about the area in general.

[Paul Berry, 2017-08-28 21:01:48]

On 2017/08/28 19:17:32, Lasse Reichstein Nielsen wrote:
> I would never say that `A.foo` overrides `B.foo`. I'd rather say that `B.foo`
> *doesn't* override `A.foo` wrt. implementation, only wrt. interface.
> 
> That is, I think of a class declaration as defining (at least) two different
> entities - the "class" which declares or inherits concrete instance members,
and
> the "interface" which declares or inherits abstract member signatures. 
> A concrete declaration in a class updates both. An abstract declaration only
> affects the interface.
> Concrete implementation can only be inherited from the superclass, interface
> members is inherited from the superclass' interface and from any interfaces in
> the implements clause.
> 
> So, in this case, *B.foo overrides A.foo* in the interface. *A.foo must
> implements B.foo* (because A.foo is the inherited concrete member of B and B
is
> not abstract).
> 
> The implementation must match the interface, at least for concrete classes.
> So we test, for any non-abstract class, that the implementation of a member
> declared or inherited by a class matches the signature declared or inherited
of
> that member in the class' interface.
> 
> Here it works - the signature of `B.foo` is `void Function()` and the
> implementation has type `void Function()`.
> 
> It's more interesting when that's not true any more:
> 
> class A {
>   void foo() {}
> }
> abstract class I {
>   void foo([x]);
> }
> abstract class B extends A implements I {
> }
> class C extends B {
>   void foo([x]) => super.foo(x);  // Ooops?
> }
> main() {
>   B b = new C();
>   b.foo(42);
> }
> 
> Here B has a problem because it doesn't satisfy its interface. It's abstract,
so
> in Dart 1 it gets away with that.
> (I haven't decided yet whether it can safely get away with it in Dart 2). 
> 
> The `super.foo(x)` call is bad. It must be checked against the actual super
> implementation of `foo`, not just the static interface of B to see that, but
it
> must be rejected.
> (That's slightly annoying because it means that the relation between a
subclass
> and an abstract super-class is tighter than just the interface contract, but
> avoiding that would mean that we require B to actually override A.foo with a
> concrete method, which is an option I'm considering).
> 
> All in all, I don't know what forEachOverridePair does, so I'm just waxing
> philosophically about the area in general.

Thanks for the info, Lasse.  This gives me a better mental framework for
thinking about the problem.  I think forEachOverridePair is a clumsy interface
because it conflates the idea of "overrides" with the idea of "must implement",
and the implementation is confusing because it commingles the two types of
checks.

I think I understand what forEachOverridePair *does* but I still don't really
understand its use cases, so I'm not really sure if this CL implements correct
semantics or not.  I'll try to understand better and follow up either with some
documentation clarification or with a fix to the semantics.