language/covariant/subtyping_with_mixin_test is failing

[scheglov]

No description provided.

[scheglov]

Is this test valid, in language_2 and language?

class A {}

class B extends A {}

class Mixin<T> {
  void f(T arg) {}
}

abstract class Interface {
  void f(covariant A arg);
}

class C with Mixin<B> implements Interface {}

We report ERROR|COMPILE_TIME_ERROR|INVALID_OVERRIDE|/Users/scheglov/Source/Dart/sdk.git/sdk/tests/language_2/covariant/subtyping_with_mixin_test.dart|21|7|1|'Mixin.f' ('void Function(B)') isn't a valid override of 'Interface.f' ('void Function(A)').

The argument arg in Mixin.f is not covariant-by-declaration (because it is not marked covariant in M, and the superclass of C&M is Object that does not have f), so we check subtypes as is, and fail.

The fact that we later have a class C that implements Interface where the corresponding parameter is covariant does not matter, the parameter in M.f should be covariant. The concrete implementation of f in C is copied as is from M.f, without adding covariant.

@eernstg Where am I wrong here?

[eernstg]

@scheglov, I agree with the analysis.

We had some very long discussions about this topic area, concluding here that it is an error if a concrete class D inherits an implementation of a method f which has a parameter that is not covariant-by-declaration at its declaration, but it is covariant-by-declaration in the interface of D:

class C {
  void f(int x) {}
}
abstract class I {
  void f(covariant num x);
}
class D extends C implements I {} // Error.

I don't see a need to change this argument in order to work with null-safety. So your example here should mark C as a compile-time error, in language as well as language_2. That example uses a mixed-in method, but that is still a method in a superclass (Object with Mixin<B>) which is inherited by C.

I can see that 'tests/language/covariant/subtyping_with_mixin_test.dart' was added in CL
https://dart-review.googlesource.com/c/sdk/+/142067, and I was a reviewer.

@munificent, you migrated that test, do you have additional comments?

I didn't notice this problem when I reviewed the CL, and I guess I relied on the version in 'language' to have sorted out that kind of problem already. ;-)

But the conclusion I see here is that (1) any issues about changing the analyzer to accept these tests should be closed, and (2) if there is no issue about changing the tests to expect a compile-time error then such an issue should be created.

@scheglov, does that match your view on the matter?

[scheglov]

@eernstg Yes, looks good to me, thank you.

Should we also report the covariant-by-declaration mismatch error, even when types match? Here is a slightly modified version of the example that you used, notice that I.f uses type int.

class C {
  void f(int x) {}
}
abstract class I {
  void f(covariant int x);
}
class D extends C implements I {} // Error.

Currently analyzer does not report error for this example.
Should I change it? Is this error in the spec?

[munificent]

@munificent, you migrated that test, do you have additional comments?

None, if I recall right, I saw that the test was failing under language_2/ so I considered a failure in language/ to be a "correct" migration since it meant the test was doing the same thing as the unmigrated test.

My goal isn't necessarily to fix all the bugs in the tests, so to have a migrated NNBD corpus of tests at parity with the legacy test. Sometimes parity means inadvertent bug-for-bug compatibility. I can't always tell what's a bug in the test or the implementation so I assume that if it does the same thing as the legacy test, at least I haven't made the test worse. :)

[scheglov]

@eernstg There seems to be a contradiction between reporting an error for implementation / interface covariant-be-declaration mismatch, and the other issue with a very similar code. In this other issue the conclusion was that "the analyzer should stop emitting an error". I'm confused.

I was looking at a failing test language/closure/forwarding_stub_tearoff_test, started searching what is a forwarding stub (because the spec does not have it), and found this other issue.

[eernstg]

@scheglov, sorry about the delay, I'm catching up after the Easter days. ;)

There was the question about this example:

class C {
  void f(int x) {}
}
abstract class I {
  void f(covariant int x);
}
class D extends C implements I {} // Error.

D is an error.

The underlying issue is that a concrete declaration (like C.f) may be inherited by a subclass (like D), and in this case the implementation may need an extra type check on an actual argument in the new context.

In particular, D.f must check the actual type of x because that parameter is covariant-by-declaration. One case where this is required is the following:

void main() {
  Function f = D().f;
  void Function(Object) g = f; // Succeeds at run time.
  g(Object()); // Statically checked call, no check on argument at call site.
}

So even though there is no difference between the type annotation for x in C.f and in D.f, it makes a difference that the parameter of D.f is covariant-by-declaration: We must ensure that the function body performs the check.

So we can't just inherit C.f, we need to get the code generation step to implicitly induce a forwarding stub D.f, that is, a method declaration that just checks the run-time type of x and then calls super.f(x). Alternatively, we just make the situation an error.

We decided to make it an error when the reason for the dynamic check is that a parameter is covariant-by-declaration in the interface of the inheritor (D), but it isn't covariant-by-declaration in the inheritee (C), but to generate the forwarding stub when the reason is that the parameter is covariant-by-class.

So D is an error. But D1 is not:

class C1 {
  void f(int x) {}
}
abstract class I1<X> {
  void f(X x);
}
class D1 extends C1 implements I1<int> {} // OK.

With D1 it is still necessary to make sure that the dynamic check is performed, and we will need an implicitly induced forwarding stub for that, and in this case we are willing to generate that stub and accept the program.

The other issue was

"the analyzer should stop emitting an error"

for the following example:

class A {}
class B extends A {}

class Base {
  void f(B arg) {}
}

abstract class Interface {
  void f(covariant A arg);
}

class C extends Base implements Interface {}

That is inconsistent, and we should get the error: C cannot inherit Base.f, because C.f must check the argument type, because C.f has a parameter which is covariant-by-declaration. The parameter is not covariant-by-class, so we are clearly in the situation where it is an error and no forwarding stub is generated.

The following comment supports the claim that the language team decided to allow the implicitly induced forwarding stub for a parameter which is covariant-by-class, but not when it is covariant-by-declaration: #31580 (comment).

So that particular reference ('here') is out of sync with the comment ('the analyzer should stop emitting an error'). Sorry about that; I added an extra sentence there to make it explicit that it is an error.

This rule about inheritance is not specified in the language specification. I created dart-lang/language#925 to put that on the todo list.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/146120

[eernstg]

@scheglov, the language was changed in dart-lang/language#925, with implementation issue #47072, such that there should no more be an error for the situation where a class C has an interface with a member m with a parameter p such that p is covariant-by-declaration, but C inherits an implementation of m where the parameter corresponding to p exists and is not covariant-by-declaration.

The behavior for that case today should be to make sure that the required dynamic check on the type of the actual argument passed to p actually has the type required for that inherited method. (This would presumably be done by implicitly inducing a forwarding method which does the check and then invokes super.m...).

This might imply that https://dart-review.googlesource.com/c/sdk/+/146120 should be abandoned. Do you agree?

[scheglov]

SGTM