Bogus "[warning] Missing concrete implementation of 'RenderObject.applyPaintTransform'"

[Hixie]

If I apply the following patch to Flutter's repository (at aa80bd6fc94d2a5189a1170737c59de1df10b249), and then run the analyzer (flutter analyze --flutter-repo), then the code goes from having zero warnings to having lots of warnings to the effect of [warning] Missing concrete implementation of 'RenderObject.applyPaintTransform'.

$ git diff upstream/master --stat ; git diff upstream/master
 packages/flutter/lib/src/rendering/box.dart | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/packages/flutter/lib/src/rendering/box.dart b/packages/flutter/lib/src/rendering/box.dart
index 4c57d43..6f98b7c 100644
--- a/packages/flutter/lib/src/rendering/box.dart
+++ b/packages/flutter/lib/src/rendering/box.dart
@@ -624,7 +624,7 @@ abstract class RenderBox extends RenderObject {
   ///
   /// The RenderBox implementation takes care of adjusting the matrix for the
   /// position of the given child.
-  void applyPaintTransform(RenderObject child, Matrix4 transform) {
+  void applyPaintTransform(RenderBox child, Matrix4 transform) {
     assert(child.parent == this);
     BoxParentData childParentData = child.parentData;
     Point position = childParentData.position;

RenderBox is a subtype of RenderObject and the method is not abstract anywhere, it's concretely defined everywhere.

See: flutter/flutter#887
https://travis-ci.org/flutter/flutter/builds/96211427

[Hixie]

eukreign helped narrow this down. Here's a tiny test case that shows the same problem. You have to run dartanalyzer --strong on this:

abstract class A { void test(A arg) { } }
abstract class B extends A { void test(B arg) { } }
abstract class X implements A { }
class C extends B with X { }
void main() { }

Because B's test signature is different than A's, it gets an "Invalid override." strong-mode error. This seems to cause the analyzer to forget the implementation of test even exists, and then when it gets to C, it seems that it sees the "implements A" directive and looks for a test implementation and can't find one.

I guess then this is a request that when you get the "Invalid override." error, you still leave the implementation alone so that the "Missing concrete implementation" message doesn't also appear.

[eukreign]

This may not actually be a bug...

X is claiming to implement test(A arg) but the final subclass C implements test(B arg). These are not compatible because while test(A arg) can handle B instances the reverse is not true. test(B arg) only supports B and B's subclasses.

To illustrate the issue, here is some code to run through dartanalyzer:

class A { test(A arg) {} }
class B extends A { test(B arg) {} }
main() {
  new B().test(new A());
}

Which will produce:

Analyzing [foo.dart]...
[error] Invalid override. The type of B.test ((B) → dynamic) is not a subtype of A.test ((A) → dynamic). (foo.dart, line 2, col 21)
[error] Type check failed: new A() (A) is not of type B (foo.dart, line 4, col 16)
2 errors found.

So, while B is of type A, the reverse is not true, A cannot be passed to methods that are expecting B.

[Hixie]

If you change the argument types to be unrelated, you get the right set of warnings:

abstract class A { void test(int arg) { } }
abstract class B extends A { void test(String arg) { } }
abstract class X implements A { }
class C extends B with X { }
void main() { }

[error] Invalid override. The type of B.test ((String) → void) is not a subtype of A.test ((int) → void). (test.dart, line 2, col 30)
[warning] The parameter type 'String' is not assignable to 'int' as required by the method it is overriding from 'A' (test.dart, line 2, col 40)
[warning] 'test' is inherited by at least two interfaces inconsistently, from (String) → void, (int) → void (test.dart, line 4, col 7)

[jmesserly]

This still reproduces. Interestingly the error comes from existing Analyzer code in ErrorVerifier._checkForNonAbstractClassInheritsAbstractMember.

I think the problem is this line: if (_typeSystem.isSubtypeOf(foundConcreteFT, requiredMemberFT)) { ... that subtype check looks backwards. In classic Dart that makes almost no difference for function types, but in strong mode it's pretty important to get the direction of the check correct.

edit: may be wrong about that guess. I couldn't repro it without strong mode, by adding an optional parameter. Need to investigate further.

[jmesserly]

Ah so the issue is that abstract class B extends A { void test(B arg) { } } is not a legal override in strong mode:

[error] Invalid override. The type of B.test ((B) → void) is not a subtype of A.test ((A) → void). (test.dart, line 2, col 30)

that invalid override is the same reason that "C" does not satisfy the interface "A". Consider:

class A { void test(A arg) { } }
abstract class B extends A { void test(B arg) { } }
abstract class X implements A { }
class C extends B with X { }
main() {
  A a = new C();
  a.test(new A()); // boom! at runtime. A is not a B.
}

I'll dupe this with #24507 which is about covariant overrides (there's dart-archive/dev_compiler#289 too, possibly others).

[Hixie]

The bug here was really "I guess then this is a request that when you get the "Invalid override." error, you still leave the implementation alone so that the "Missing concrete implementation" message doesn't also appear."; that is, just a clarification rather than an error per se.

[jmesserly]

I think for cases like:

class A { void test(A arg) { } }
abstract class B extends A { void test(B arg) { } }
class C extends B implements A { }

We'd want to issue an error on the implements A right? (we currently do). The implements A claim might appear far away from class B extends A, they might even be in different packages.

My mental model is that "implements" and "with" should cause appropriate checking when they appear in a concrete class, to ensure the interface is implemented.

The error message wording should be improved though. Instead of "Missing concrete implementation" it should say something about the override not matching types. Would that work?

CC @leafpetersen @vsmenon for thoughts as well.

[Hixie]

Right now you get an Invalid Override error and a Missing concrete implementation error. I think you should only get the former, and not the latter.

[jmesserly]

consider a case like this:

class A { void test(A arg) { } }
abstract class B extends A { void test(B arg) { } }

class D { void test(A arg) { } }
class C extends B implements D { }

In that case, D has no relation to A or B. Shouldn't we issue an error, that C does not correctly implement D? C has a problem with its declaration. (B does too, but they're different problems, IMO.)

It's not easy to distinguish this case from the other one.

It's sort of like:

takesAString(String s);
main() {
  int x = '42'; // error: String is not an int
  String y = x; // error: int is not a String
  takesAString(x); // error: int is not a String
  takesAString(x); // error: int is not a String
}

They're caused by the same typo in int x declaration, but it still issues type errors at subsequent uses. If we fixed that to be "String x" all of them would go away.

[leafpetersen]

@Hixie I'm guessing the optimal solution for you guys would be for us to support covariant overriding so that neither would be an error?

Definitely agreed that the error message is non-optimal. I think @jmesserly's concern here is good though - it's hard to get rid of the double error message without skipping other useful error messages.

It's odd that we give that particular error message there though - especially since we give the correct error message in the case that the overridden types are unrelated. That seems like a clear bug.

[Hixie]

I don't understand @jmesserly's example. Why doesn't C implement D? Or do you mean in the DDC mode it doesn't implement D? Presumably in "normal" Dart it would be fine.

The analyzer and the VM already do the covariant overriding support we use in Flutter, and we manually filter out the "Invalid override" warning (which is why initially I was confused when filing this bug, hehe, notice how I didn't mention Invalid Override in the first comment) but yes, for using strong mode the ideal solution for flutter would be to just not have these extra limitations on overriding. (Extra over what Dart normally allows, I mean.)

[jmesserly]

Why doesn't C implement D? Or do you mean in the DDC mode it doesn't implement D? Presumably in "normal" Dart it would be fine.

Right, it is not a sound override, so the stronger type system doesn't accept it.

Here's an example that illustrates the behavior in classic Dart:

class A { void test(A arg, [foo]) { } }
abstract class B extends A { void test(B arg) { } }

class D { void test(A arg, [foo]) { } }
class C extends B implements D { }

output from dartanalyzer:

$ dartanalyzer test.dart
Analyzing [test.dart]...
[warning] Must have at least 2 parameters to match the overridden method 'test(A arg, [dynamic foo]) → void' from 'A' (test.dart, line 2, col 35)
[warning] Missing concrete implementation of 'D.test' (test.dart, line 5, col 7)
2 warnings found.

It's odd that we give that particular error message there though - especially since we give the correct error message in the case that the overridden types are unrelated. That seems like a clear bug.

I still don't understand the problem (other than the message text being misleading). What ErrorVerifier does (this is the existing Analyzer code -- no strong mode involved) is look for an override that is valid in the type system. If it doesn't find an override with matching type, so you get "Missing concrete implementation" even though there was an implementation, just one that has the wrong type signature. I agree the error message could be better.

Either way, it doesn't seem related to strong mode. If we don't like behavior above we should open a new bug for normal analyzer & close this one. edit: or change the subject line of this bug. Here's a variant of the original problem for normal dartanalyzer:

abstract class A { void test(A arg, [foo]) { } }
abstract class B extends A { void test(B arg) { } } // [warning] Must have at least 2 parameters to match the overridden method 'test(A arg, [dynamic foo]) → void' from 'A'
abstract class X implements A { }
class C extends B with X { } // [warning] Missing concrete implementation of 'A.test'
void main() { }

Does that make sense & seem right to y'all?

[jmesserly]

Suggestion for a new subject:

"Missing concrete implementation" warning even though implementation exists, but with a not-matching type

[pq]

@Hixie : where do we stand on this one?

Running flutter analyze with the '\\[warning\\] Missing concrete implementation of \'RenderObject\\.applyPaintTransform\'' regexp removed reports no warnings. Can we close this and remove the regexp?

[Hixie]

if there's no warnings, then yup

[Hixie]

(The original bug is still present in the analyzer, though; if the warnings are gone for flutter, that just means we've coincidentally tweaked our code so as to avoid it for now.)

[pq]

if the warnings are gone for flutter, that just means we've coincidentally tweaked our code so as to avoid it for now.

Or, more likely, it's because strong-mode is off! I'll sit tight until we settle that bit.

[Hixie]

FWIW, we're now explicitly working around this issue by having no-op methods in the Flutter code. See flutter/flutter#2965

[jmesserly]

I think if we can get y'all a solution for covariant overrides (which @leafpetersen may be working on?), then you'll be able to get rid of those, because the base class implementation will be valid, which means the "implements" in the derived class will also be valid.

[pq]

@jmesserly, @leafpetersen : any update on this one?

[jmesserly]

@pq -- are you looking for #25578 related to covariant parameter overrides? Last update to that bug was here: #25578 (comment) ... and @leafpetersen and I would be the right folks to implement that one, probably. I'm juggling a few other things but it's on my list. Ideally Leaf and/or Bob will come up with a syntax proposal first, that was the big reason I didn't jump in.

(this bug here is not strong mode. Analyzer will report incorrect overrides at every level of the inheritance chain where they occur. So we wouldn't be the right folks to fix it, and I could be wrong, but I'm guessing it's not what you're asking about :) )

[pq]

Thanks for the context @jmesserly!

[Hixie]

For the record, this is still causing problems for Flutter.

Specifically, there are methods where we have the wrong signatures because we're working around this issue. For example, handleEvent() on RenderBox has the wrong signature and we work around this by mentioning it in a dartdoc. If I fix the signature, we get 56 bogus "missing concrete implementation" errors even though the code is in fact fine, all triggered by the mixins with "implements" confusing strong mode (despite the fact that we have the covariant checks turned off).

[jmesserly]

@Hixie would covariant override support help? I am working on that in #25578

[Hixie]

We already have covariant overrides, the bug here is just that the analyzer gets confused when you use them with mixins if you have strong mode enabled even if you disable strong_mode_invalid_method_override. #25578 is about limiting covariant overrides to only parameters where you opt-in to them, which seems orthogonal.

[jmesserly]

Ahhhhh, yes, so what's going on here is described in my last comment:
#25578 (comment)

Analyzer has duplicate override checking so suppressing strong_mode_invalid_method_override is not enough to get covariant overrides.

[Hixie]

I just ran into this again. I wanted to provide a mixin for decendants of State<T>. State<T> has a didUpdateConfig(T) method. I don't override it in the mixin, but now suddenly anywhere where I use the mixin, I have "Missing concrete implementation of 'State.didUpdateConfig'" even though there is a concrete implementation and it works fine.

[jmesserly]

@Hixie is there a repro for that or something I can look at? It might be a different bug (not sure). Either way that sounds bad, I'd be happy to help track that down.

[Hixie]

The repro from the second comment above still works:

abstract class A { void test(A arg) { } }
abstract class B extends A { void test(B arg) { } }
abstract class X implements A { }
class C extends B with X { }
void main() { }

[jmesserly]

Ouch, it even fails with @checked:

import 'package:meta/meta.dart';
abstract class A { void test(A arg) { } }
// good news: no error here because we used @checked
abstract class B extends A { void test(@checked B arg) { } }
abstract class X implements A { }
// bad news: error here
// [error] Missing concrete implementation of 'A.test'
class C extends B with X { }
void main() { }

[jmesserly]

I don't know how to fix this for "Dart 1" mode but I have a fix on the way for strong mode.

[jmesserly]

https://codereview.chromium.org/2340173002/

[srawlins]

Fixed for strong mode.