Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analyzer performs override check between superinterfaces and superclass #34392

Closed
askeksa-google opened this issue Sep 6, 2018 · 8 comments

Comments

Projects
None yet
7 participants
@askeksa-google
Copy link
Contributor

commented Sep 6, 2018

abstract class I {
  foo([a]);
}

abstract class A {
  foo() {}
}

abstract class B extends A implements I {}

class C extends B {
  foo([a]) {}
}

The analyzer complains (at the declaration of B) that the foo method in A is not a valid override of the foo method in I.

There is no override relationship between those two methods, so having them both in B is valid, because all of the following hold:

  • One of the methods has a signature which is a valid override of the other (so there is a most specific one for the interface of the class).
  • The class is abstract, so it doesn't have to have implementations for all methods in its interface.
  • Every concrete subclass has an implementation that is a valid override of both methods (so the method implementation satisfies the override rules and the class implements its interface properly).

This looks like it could be some mix-up between override checking and checking that every concrete class conforms to its interface.

@jmesserly

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

@leafpetersen - did we do this on purpose?

IIRC, strong mode initially did not allow abstract members to override concrete ones with a different signature. But that may have been for simplicity of the implementation. We could certainly relax this and accept this example. (Not sure if there's a Dart 2 spec for how this should work?)

As I was thinking about this, it occurred to me that abstract member overrides could probably break soundness in Analyzer, and sure enough:

class C {
  Object foo() => 42;
}
abstract class D extends C {
  String foo([int x]);
}
class E extends D {}

main() {
  String s = E().foo(123); // OOPS!
  print(s.trim());
}

So we've got to fix something here regardless. If we fix how concrete classes are checked against their interfaces, both of these examples will behave correctly (accepting the initial example, rejecting my example above).

@askeksa-google

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

I have filed the missing interface checks in the analyzer as #34507.

@eernstg

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

The current language specification specifies overriding in such a way that we must update it; e.g., it makes covariant parameters a compile-time error here (line 1479, 5dff98f):

It is a compile-time error if an instance method $m_1$ overrides an instance
member $m_2$ and the type of $m_1$ is not a subtype of the type of $m_2$.

So we can't just take everything from the language specification and be done with it.

However, the language specification does insist that the interface 'overrides' and 'inherits' relations treat all superinterfaces identically, so it would be a breaking change to start giving the superclass a different treatment than the classes that the current class implements.

Now, when we have several different member signatures for a given name and none is most specific, the language specification specifies that we will compute an interface whose type annotations are all dynamic. This is clearly not what we want to do in Dart 2. (The resulting method signature would of course also be a compile-time error today in many cases, and it was even an error in Dart 1 when it required a signature with both kinds of optional parameters at the same time ;-).

So we discussed how to update the specification on this topic. The following feature spec is in the pipeline (though not yet landed): interface-conflicts.md, introduced by this CL.

This feature spec specifies, exactly as stated by @askeksa-google, that all superinterfaces (i.e., the interface of the superclass as well as the interface of each class that it implements) are treated identically, and there is no requirement that any of them must override any others. (In particular, it is not required that a method signature in the interface of a class that it implements must override a signature with the same name from the interface of the superclass, nor the converse). So we just have the requirement that one of the method signatures must be more specific than all others, again exactly as @askeksa-google said. If no such most-specific signature is given then the developer must explicitly write one, we won't invent a solution.

@leafpetersen, @lrhn, do you agree that there is no doubt about the symmetric treatment of all superinterfaces? Do you foresee any changes to said feature spec with respect to the treatment of conflicts?

PS: The feature spec has a discussion about erasing certain top types to Object, but that's not part of the proposed specification, just a non-breaking extension that we can discuss later.

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

OK, given that the specification is not complete, I'm moving this out of Dart2.1 milestone for analyzer.

@scheglov scheglov added the p2-medium label Sep 19, 2018

@scheglov scheglov modified the milestones: Dart2.1, PostDart2.1 Sep 19, 2018

@lrhn

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

There is no doubt that we want the symmetric approach where the most specific member declaration among all super-interfaces is the interface signature inherited by the subclass (when it doesn't declare a signature itself). That has been the intended behavior for Dart 2.0 all along.

@eernstg

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Actually, dartLangSpec.tex does specify an error, as of a07b2a1, whenever there is no most-specific signature for a given member m (so the superinterfaces have multiple signatures for m, none of which is most specific, and the subclass itself does not declare m). I thought that change was part of a non-landed CL. But this means that there is no doubt that such a conflict is an error, and there is certainly no reason to consider this issue blocked on any spec updates.

@scheglov scheglov modified the milestones: PostDart2.1, Dart2.1 Sep 20, 2018

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

This would be good to fix for 2.1.

@scheglov scheglov self-assigned this Sep 22, 2018

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

https://dart-review.googlesource.com/c/sdk/+/76061 (re)implements handling inheritance according to the updated spec, and how it was explained to me in mail. This fixes Analyzer for the original code, so it does not report any error.

@scheglov scheglov closed this Sep 24, 2018

dart-bot pushed a commit that referenced this issue Sep 24, 2018

Implement inheritance/override checks from the spec.
This CL starts moving checks from strong-mode specific checker,
and old InheritanceManager into an implementation that is based
on the current spec, and avoids old baggage. It also fixes the issue
we were asked to fix for Dart 2.1.

Bug: #34392
Change-Id: Id5a23c5db7704b2b530bb894ae92628a08eaa70f
Reviewed-on: https://dart-review.googlesource.com/76061
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

dart-bot pushed a commit that referenced this issue Sep 24, 2018

Add regression test for issue 34392.
Bug: #34392
Change-Id: Ia86264e8e752d7563b97657321f92eaa53f257bb
Reviewed-on: https://dart-review.googlesource.com/76180
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

dart-bot pushed a commit that referenced this issue Sep 25, 2018

Reland: Implement inheritance/override checks from the spec.
Relands https://dart-review.googlesource.com/c/sdk/+/76061
Was reverted in https://dart-review.googlesource.com/c/sdk/+/76301

The difference with the original CL is that we don't look into
superclass and mixins of mixed-in for concrete members. This reduces
the number of errors in Flutter codebase to 1.


R=brianwilkerson@google.com

Bug: #34392
Change-Id: I86256b598d116439194cbaf4d09b4f72013d6563
Reviewed-on: https://dart-review.googlesource.com/76340
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.