Forwarding stubs should not forward to other forwarding stubs

[stereotype441]

Consider the following code:

class A {}
class B extends A {}
class C {
  void f(B x, B y) {}
}
abstract class I1 {
  void f(covariant A x, B y);
}
class D extends C implements I1 {}
abstract class I2 {
  void f(B x, covariant A y);
}
class E extends D implements I2 {}

During compilation of class D, the front end discovers that it needs to insert a forwarding stub for f to ensure that x is type checked. This forwarding stub forwards to C::f. During compilation of class E, the front end discovers that it needs to insert a forwarding stub for f to ensure that y is also type checked. This forwarding stub should forward directly to C::f--it shouldn't forward to D::f.

Note that the front end currently does this correctly provided that D and E are being compiled at the same time. But in a modular compilation scenario where E is being compiled and D comes from a .dill file, the front end tries to forward E::f to D::f.

(Borrowing terminology from #31562 (comment), the target in question here is the "super target")

CC: @sjindel-google

[sjindel-google]

The reason for this is that the semantics of forwarding stubs are much more complicated if they can forward to other forwarding stubs.

[lrhn]

@leafpetersen @eernstg

I'm not sure I would expect this program to compile. I'd expect the class D to be rejected at compile-time because its implementation off has type void Function(B,B), which is not a valid override type for the member type of f in the interface of D, void Function(covariant A, B). (I use "valid override" instead of just "subtype" because the former will consider covariant as well as safe function typing).

The type of f in D is inherited from I1 because that member type is a valid override of the member type of f in the interface of C, and when inheriting a member from multiple superinterfaces, we pick the one that is a valid override of all the others (and fail if there isn't one).

When we check whether a class correctly implements its interface, we check whether the member implementation's type would be a valid override of the interface member type.
In this case void Function(B,B) is not a valid override type for void Function(covariant A,B) because the B in the former is not covariant.

You can't write an override like that, because covariance is inherited, but this example inherits a non-covariant implementation and tries to make it match a (covariant) type that it doesn't match.

So, instead of forwarding anything, I'd just reject the classes D and E, just as if A had not been covariant. Being covariant saves a class itself from satisfying some superclass constraints, but it doesn't help an implementation that was already written in a superclass from not satisfying the interface.

You could instead choose to provide a satisfying implementation:

class D extends C implements I1 {
  void f(covariant A x, B y) => super.f(x as B, y);  // and tell me *why* x is a B!
}

The fact that you have a surprising and unsafe-looking cast here suggests that the just implicitly calling super.f directly is not a good idea!

Are there any good reasons for allowing classes like D and E? That is, is anyone already writing code like this (please, tell me no!).

Come to think of it, I'm not we have fully considered the is-a-valid-implementation rules for covariant parameter methods.
Would the classes above be valid if x and y were covariant in C?
I'd even argue that they are not. You can change the interface in a subclass to something more restrictive, but if you make the subclass more permissive (by changing the parameter to a superclass), then an inherited implementation may be invalid (with covariant parameters, it might be valid, without it never will). We should use the actual parameter type of the inherited member, without considering covariance annotations, to decide whether it's a valid implementation of the interface or not. In this case, it is not.

[eernstg]

I think the strongest argument in favor of requiring an explicit declaration of f in D and E (rather than accepting the situation silently and generating a forwarder) is that it would be an observable change to add such a forwarder for f in D in order to get the right semantics: The reified type of f in an instance of C must be void Function(B, B) (so unless we make "inherited implementation" mean something more involved than you'd expect, it would be the same in D, with the same reified type, if we only say that "it is inherited") and the reified type of the forwarder in D must be void Function(Object, B).

So, based on the fact that the forwarder in D must be observably different from the inherited implementation, it makes sense to say "we won't generate such a forwarder implicitly".

I think the way to say this is that it is a compile-time error if the implementation available to a class (of a method blah-blah) has a non-covariant parameter, and the corresponding parameter (in blah-blah) in the interface is covariant.

Checking, it is clear (and requires no amendments to the notion of covariant parameters) that C.f has a non-covariant first positional argument.

[stereotype441]

@lrhn I think you make a very convincing case that class D should be rejected at compile time. And after some experimentation I see that the analyzer implementation of strong mode agrees with you.

@leafpetersen do you agree with this? If so, I'm happy to take that as gospel from the language team, and I'll adjust the front end behavior accordingly.

[stereotype441]

@lrhn regarding your followup comment, that the classes D and E would be invalid even if x and y were marked covariant in C::f, this would be a more significant change, and I'm not precisely sure how it would work.

As I understand it, the current analyzer behavior is: when checking whether an implementation method satisfies the interface of a class, if the implementation method contains any parameters that are marked covariant (or that inherit covariant from one of their respective interfaces), then the implementation method is checked against all transitively inherited interfaces. The override is considered valid provided that the overridden parameter type is either a subtype or a super type of each transitively inherited interface parameter type. So for instance, in the following code, classes A, B, and C are allowed, but D is not:

class A {
  f(covariant num x) {}
}
class B extends A {
  f(covariant Object x) {}
}
class C extends B {
  f(covariant int x) {}
}
class D extends B {
  f(covariant String x) {} // Incompatible with transitively inherited interface from A
}

What rule do you have in mind, and how would it work with this example?

[stereotype441]

@eernstg I think the rule you have in mind would reject a lot of programs that DDC currently accepts, for example:

class C {
  void f(String x) {}
}
abstract class I<T> {
  void f(T x);
}
class D extends C implements I<String> {}

This would be rejected because the parameter of I::f is covariant due to class covariance, but the inherited method C::f has a non-covariant parameter.

I'm not certain, but I suspect that the rule you have in mind would actually reject all programs in which DDC currently inserts forwarding stubs. I suspect that probably makes it a bigger breaking change than we can feasibly entertain right now.
@leafpetersen @lrhn your thoughts?

(Edit: changed I to I<String> in the example above)

[stereotype441]

Regarding the original subject of this bug (that the front end shouldn't create forwarding stubs that forward to other forwarding stubs), I think that issue is still of concern, if we consider a slightly different example:

class A {}
class B extends A {}
class C {
  void f(B x, B y) {}
}
abstract class I1<T> {
  void f(T x, B y);
}
class D extends C implements I1<B> {}
abstract class I2<T> {
  void f(B x, T y);
}
class E extends D implements I2<B> {}

In this example the covariance arises from covariant generics rather than the use of the covariant parameter, so it avoids @lrhn's objections.

[eernstg]

From Lasse:

classes D and E would be invalid even if x and y were marked covariant in C::f

I didn't see this, but it wouldn't be the case with the approach that I considered: The first positional parameter of f would then be covariant in the inherited implementation as seen from D and from E, and there would be no problem (conceptually, and since the arguments of C.f would have generated dynamic type checks at the beginning of the body of f it's also no problem with respect to the implementation).

From Paul:

reject a lot of programs that DDC currently accepts

class C {
  void f(String x) {}
}
abstract class I<T> {
  void f(T x);
}
class D extends C implements I<String> {}

That's true, the rule that I proposed would cause this to be rejected (and the developer could fix it by writing an explicit forwarder in D).

But if you ask your IDE (if that's possible, but it could certainly be possible) to take you to the implementation of f which is being used by an instance of D (that is, an object whose dynamic type is D, not just something with static type D), and it would take you to the declaration of f in C, then you could get really puzzled because you have just torn off one of these guys, but it has reified type void Function(Object), and there is nothing about the declaration that your IDE gave you which justifies that reified type.

(Ah, here's a natural scenario: You are actually running the torn-off method, so you can directly see during single-stepping that you're running the implementation declared in C, and you also happen to know that the reified type of the torn-off method is void Function(Object). "WOT?")

Does this really affect "lots of programs"? Isn't it reasonable to tell developers that something spooky is going on with f "when it is inherited by D"? ;-)

[lrhn]

@lrhn regarding your followup comment, that the classes D and E would be invalid even if x and y were marked covariant in C::f,

I think I was perhaps a bit too strict there. It's correct that the covariant method from C would be a valid override of the interface of D, so there shouldn't be a technical problem with that.
It will introduce a curious case where the implementation and the interface of a non-abstract class do not agree on the static signature. The more specific interface signature is inherited from I1, and the implementation from C. In all other cases, the interface and implementation has to match.
That's why I wanted to reject it. The implementation does not match the interface. The interface says that the class will accept A instances, the implementation won't. That's not a covariant implementation of the class' interface, it's an implementation of another signature, so it would be a valid override, but it is not a valid implementation.
So, I still want to reject it, but not for not being a valid override.

The generic-introduced case is harder to reject. I want to, but I can't really find a good argument (except a pseudo-rule of "a non-covariant method is not a valid implementation of a covariant signature", which isn't a rule we have introduced ... yet?)
It does suffer from the problem that we insert a synthetic forwarder, and that synthetic forwarder is visibly different than the inherited method. I we go this way, the specification will have to specify that forwarder insertion, something we have otherwise tried to avoid.
I'd actually prefer to have the pseudo-rule above. You ask for covariance - you better be prepared to implement it. We won't do it for you.

[leafpetersen]

@lrhn I think you make a very convincing case that class D should be rejected at compile time. And after some experimentation I see that the analyzer implementation of strong mode agrees with you.

@leafpetersen do you agree with this? If so, I'm happy to take that as gospel from the language team, and I'll adjust the front end behavior accordingly.

Yes, I'm happy with this.

For this example:

 class C {
   void f(String x) {}
 }
 abstract class I<T> {
   void f(T x);
 }
 class D extends C implements I {}

Did you mean I<String>? Otherwise this should be rejected (and is by the current analyzer implementation).

Assuming so, I'm hesitant to reject this. I see where @lrhn is coming from, but on the other hand, if I was invariant we wouldn't want to do this, and our poor users don't get to opt out of covariance. They didn't necessarily ask for I to be unsafely generic: they just got it, because that's all we got. So making them stub everything out by hand just seems excessive to me.

[stereotype441]

@leafpetersen Thanks! I'll fix the front end accordingly next week.

(And yes, you are correct that I meant I<String>. I've edited my comment to fix it.)