-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
support covariant parameter overrides #25578
Comments
related: #24507 There was also a request for |
@vsmenon assuming you haven't looked at this? in which case I can do it. Started poking around, looks pretty easy, although I need to add some tracking so we can mark the parameter as needing a cast. |
Also related #25232 |
We may want to merge the various "covariant override support" bugs. Our most recent thoughts (from a chat with @leafpetersen and @munificent) are to implement an opt-in way to get full support for covariant overrides, rather than downgrade to warning. For example, perhaps something like: class A {
void foo(A a) { print('A $a'); }
}
class B extends A {
void foo(b as B) { print('B $b'); }
} (syntax is still heavily under discussion, though) |
if @leafpetersen and/or @munificent find a syntax they like, I can take a stab at implementing this in Analyzer/DDC side. |
So one thought that @munificent had -- maybe we should just allow this by default? And Analyzer Strong Mode can mark the AST appropriately for back ends like DDC to know where the casts are. If that's too permissive, we could add a flag to disable it, or perhaps a warning and then that warning can be suppressed by the usual techniques (per-use, or per-project). The benefit of this is we don't need new syntax -- we can just allow it. Thoughts @leafpetersen ? |
I vote for just allowing it. I don't think this was ever a problem so far (which would make me vote for explicit syntax). The cast is local and kind-of expected (no surprise). (That said: we could definitely optimize this case to avoid the check if we know the target). |
I think assignability, covariant argument types, and covariance of class type arguments go together: They all introduce implicit downcasts. It's no problem to detect and reject or warn about any of these features, the real issue is how this affects the flexibility that programmers enjoy in return for the unsoundness. Assignability is a local issue, and programmers could just add the required downcast in case assignability is strengthened to a subtype requirement. But the other two are non-local, so removing the ability to use covariance for argument types and class type arguments will remove the potential for checked-or-strong-mode downcast failures from a set of locations in the code which is much more complex (global analysis is needed in order to detect a superset of the locations where these failures may occur, and it's undecidable whether the "dangerous" types of objects will ever occur there). Furthermore, covariant argument types and covariance of class type arguments fit in with certain programming idioms where the underlying regularity ensures soundness in ways that the Dart type system cannot capture (e.g., family polymorphism, where classes work together in the same family but objects from different families are never used together). This means that these kinds of covariance are not just about being lazy and taking unnecessary risks, they are just as much about expressing regularities which actually exist, even though they are not enforced fully by the type checker. From this point of view, I think restrictions on covariance for argument types and class type arguments should be considered together, which also amounts to an argument in favor of allowing covariance for argument types. This means that we keep having defaults which are flexible and convenient, a few steps beyond that which soundness will justify. That said, it would be nice to have the ability to strengthen checking to the sound level, e.g., by being able to declare that an entire type hierarchy must use sound overriding (no covariant parameter types, no contravariant return types, etc), such that we can have useful knowledge about soundness that developers can use in their reasoning, and compilers may use to produce faster code. |
Thanks so much for the thoughts @floitschG @eernstg! I'm definitely persuaded by that. Excellent point too about the symmetry between covariant argument types and covariance of class type arguments. I'll go ahead and implement the "allow it" plan 👍 |
I'm quite strongly opposed to implementing this as the default. It is certainly the case that there are valid use cases for covariant overrides. But they are there to support a very specific pattern of programming which is used in specific limited cases, and should always be used with specific intention. Making them the default turns off useful static checking and refactoring tools for all users, for the benefit of the small subset of programs which use this pattern. If a programmer changes a method signature in a way that is incompatible with overriding methods, they should get feedback from the static checker that they have broken other code. This is one of the key benefits that users get from static checking: it helps make incremental maintenance of large code bases tractable. Note that empirically, of the large code bases that I've looked at (flutter, internal apps) covariant overrides are by far a corner case. |
I'm currently leaning towards agreeing with @leafpetersen. If you want to downcast, you can always do it in the method's body (or am I missing something?). That will keep the type hierarchy sound and make it clear to a reader that there is a downcast happening. All of the covariant overrides that I can think of from the existing code bases actually indicated design issues. Also, it seems the conversion to strong mode can be automated because they are detectable: BEFORE: @override
void foo(SubType v) {
...
} AFTER: @override
void foo(ParentType originalV) {
SubType v = originalV as SubType;
...
} |
To be clear, I'm completely on board with providing opt in syntax to make the compiler generate the boilerplate downcasts. I just don't think it should be the default. |
Yes, you can, but:
I agree with Leaf that I'd like this to be explicitly opt-in with some kind of syntax. There's an interesting question about whether the syntax should be in the superclass ("subclasses can tighten this") or in each subclass ("I am tightening this"). My hunch is the former hits the right usability goals. But getting new syntax in seems to take quite a while. So I wonder if in the short term we should allow covariant overrides with a runtime check and then work towards a notation to make them explicitly opt-in? |
Allowing it by default now and then later on opt-in is just asking for pain. When we make it opt-in, everything has to be fixed up. Code that is currently strong mode clean will drift, and code that is newly converted won't have been fixed up ever, and so we will have a huge mass of code to fix, and we will be sad... :( |
Leaf, I can understand your underlying preference for sound rather than For developers, the useful choice is: do I or do I not want to take the I think a promising approach for allowing developers to make this kind of The crucial point is that expressions admit subsumption (it's statically For instance, we could have modifiers or annotations specifying that a This would provide added safety in useful chunks, it's not hard to Obviously, the default could be the other way around (start out strictly I tend to think that starting out on the flexible side and building That's the reason why I prefer defaults where strictness is requested On Wed, Aug 3, 2016 at 3:34 AM, Yegor notifications@github.com wrote:
Erik Ernst - Google Danmark ApS |
Yes, in many cases you can solve it in a sound way using generics. The downside is that it in some cases, it adds a lot of complexity to the entire API. For example, if you change: abstract class Renderer { void render(Widget w); } To: abstract class Renderer<T extends Widget> { void render(T w); } Then:
So, for Flutter, what I'm planning to do is see where they get covariant override errors and try to make the surrounding code generic to see how much complexity it adds. If it's not too bad, I think it's a viable solution. But in some cases, I think it can end up being not worth the effort. |
I totally agree, though for me that's an argument to look into making generic variance type-safe too. :)
I like this, though I think the granularity should be at the member or possibly even parameter level, not the entire class. Covariant overrides are pretty rare, so even in a class that needs them, it probably only uses it on one or two members. The other members in the class shouldn't pay the cost in terms of runtime checking or lack of safety just to please the minority.
I look at choosing defaults as basically akin to Huffman encoding. You want to give the most common code the shortest representation. In this case, my experience is that intentional covariant overrides are quite rare. Probably less than 5% of members, maybe closer to 1%. (We can and should scrape a corpus and get real numbers here.) Given that, I think defaulting to not allowing covariant overrides is the right default. |
I don't buy an argument that says that we should do two things wrong instead of just one, because consistency... :)
This is the key disagreement here: my argument is that this is not the relevant risk. The relevant risk is that some future modification of the code will silently break other pieces of code.
I think you are hugely overestimating the frequency of which programmers actually use covariant overrides. There are 184 uses of co-variant overrides in the flutter repo. I don't have an exact count of the number of actual overrides, but a rough textual count gives 2701.
That's less than 10% of overrides, for one of the biggest advocates of this feature. Why are we optimizing for the rare case? More generally, I advocate for building systems which have "pits of success" to fall into. Most programmers who override a method won't change the signature. They are implementing an interface, and they expect to follow that interface, and they have no interest in thinking about co-variant or contra-variant overrides. The "pit of success" is that the static checking just works for them - they get errors if they use the wrong type, or if they change a superclass type and don't change the subclass type. Their code works, and is maintainable, and they are happy. |
@munificent I think you might have misunderstood the second part of my comment: "raw types could hoist the types", which seems to address your problems as follows: Problem # 1: No, you do not need to type Problem # 2: Similarly here, raw Problem # 3: I do not fully understand the issue here. I have a feeling that in order for class BatchRenderer {
// same as final List<Renderer<Widget>> _renderers;
final List<Renderer> _renderers;
void render<T extends Widget>(T widget) {
_renderers
.where((r) => r is Renderer<T>)
.forEach((r) => r.render(widget));
}
} |
On Wed, Aug 3, 2016 at 7:35 PM, Bob Nystrom notifications@github.com
An aside: It is actually possible to handle a void some_method(final Renderer renderer) The type Btw, it's unsafe to allow Dart isn't likely to get support for virtual types in this century, but
This was exactly the point I was making: Class type argument covariance |
On Thu, Aug 4, 2016 at 3:10 AM, Leaf Petersen notifications@github.com
Covariant argument types and covariance of class type arguments (when used (For a longer discussion about why it makes sense to tolerate potential
If we introduce support for quantified constraints (on the form "this
|
It looks like we want to support some form of covariance. I think it's worth splitting up the syntax aspect from the soundness aspect. For syntax, I don't have a whole lot to say beyond a preference for something explicit to indicate the runtime check to the user. I think most of our existing users prefer that and actually see our current type param checks as more a bug than a feature. For soundness, we already have arguable broken behavior (with generic covariance) where the runtime type of a variable is not a subtype of the static type. I hope we don't make it worse. E.g., class Foo<T extends Iterable> {
T bar(T x) { ... }
}
void main() {
Foo<Iterable> foo = new Foo<List>();
var bar = foo.bar; // Static type is Iterable -> Iterable
print(bar.runtimeType); // Prints List -> List
bar = (bar as dynamic); // Prints a runtime error in DDC
} One option is to view covariance as syntactic sugar on a sound pattern. E.g., the above could be lowered to: class Foo<T extends Iterable> {
T bar(Iterable _x) { T x = _x as Iterable; ... }
} In this case, the runtime type of the tearoff is Iterable (the type bound) -> List. |
Ah, sorry, you're right. I was looking at this in terms of how you'd solve it using generics without any other language changes. Your suggestion is pretty interesting in that it gives you the safety of more generics but alleviates some of the usability problems. My thirty second intuition is that the implicit hoisting probably has some weird failure modes. Like what happens if you have two overridden methods that provide different parameter types? Do you hoist their least upper bound? Will it cause problems when a class evolves over time if it does so in a way that changes the implicitly hoisted generics? But I haven't put much thought into it. It is a really neat idea. I'll ponder it more. |
Yeah, more generally what I'm implying is that there are no raw types any more, not statically nor at runtime. Now if there are no issues with eliminating raw types, then the interpretation of the syntax of raw types is up for grabs. I suggest that we use it for "reasonable defaults" for generic parameters that you do not wish to type. My gut feeling is that it's very doable. As an example, C# doesn't have raw types, and in fact doesn't use raw type syntax for anything (it's illegal). I think that's also the case in Swift. |
Actually, C#'s story is a little different. Types can be "overloaded" by type parameter arity. So you can define separate classes, It ends up being really handy. See, for example, the different forms of Tuple, Func, and Action. |
Yeah, if there's any chance that Dart will support this in the future, then there's more to think about. Perhaps arity can be hoisted too using structural matching if possible? abstract class Tuple<T1> { T1 get value1; }
abstract class Tuple<T1, T2> { T1 get value1; T2 get value2; }
// structurally only matches Tuple<int, int>
class IntTuple extends Tuple {
@override int get value1 => 1;
@override int get value2 => 1;
}
// error: ambiguous - could mean either Tuple<int> or Tuple<int, dynamic>
abstract class FunkyTuple extends Tuple {
@override int get value1 => 1;
} The compiler would then have to do structural matching, something akin to interfaces in Golang. Or the whole situation could be considered ambiguous and the developer is forced to type out generics exactly. The issue is that introducing new types with different arities could be a breaking change. |
Yeah, that level of structural typing feels out of place to me for a language like Dart. |
this is not a fix for this issue, but fixes another bug I found while investigating: https://codereview.chromium.org/2236763002/, so I'll probably want to land that first. We also appear to be checking overrides in two places (checker.dart and error_verifier.dart) but more disturbing was they were using different logic (which that CL should hopefully fix). |
fyi, I'm blocked for now on landing either this or something like it: https://codereview.chromium.org/2246293002/, basically we need to remove the duplicate override checking |
@leafpetersen -- it sounds like we might want this for callbacks as well? based on flutter/flutter#5689 (comment) <Type, GestureRecognizerFactory>{
// fn type is Object -> TapGestureRecognizer
TapGestureRecognizer: (@covariant TapGestureRecognizer recognizer) {
// implicit cast recognizer to TapGestureRecognizer
return (recognizer ??= new TapGestureRecognizer())
..onStart = _handleOnStart;
}
}, |
We may want to prototype this, but I need to chat with @Hixie a bit more first. He's expressed a preference for a mechanism that puts the burden on the API rather than on the client of the API. It's not exactly clear to me yet how that should be surfaced in code like this. |
R=pquitslund@google.com Review URL: https://codereview.chromium.org/2334413002 .
Note, after some discussion we chose to go with |
With this in place, in strong mode, you can now mark any given parameter type of a method with the class A {
void f(Object x) {};
}
class B extends A {
void f(@checked num x) {}; // Allowed, even though num <: Object
}
class C extends B {
void f(int x) {}; // Allowed again, since a superclass has opted in
} On DDC, and on other eventual strong mode platforms, there will be an implied runtime type check inserted in the header of the method to verify that the argument passed is of the correct type. Tearing off a method which has been marked with typedef void F(Object x);
void main() {
var c = new C();
assert(c.f is F); // Always true.
} Covariant overrides are checked against all overridden methods. This means that you cannot override with arbitrary types. The following is not allowed: class A {
void f(Object x) {};
}
class B extends A {
void f(@checked num x) {}; // covariant override
}
class C extends B {
void f(Object x) {}; // contravariant override, ok
}
class D extends B {
void f(String x) {}; // ERROR. A valid covariant override from Object, but not from num
} The |
This seems weird. What are the implications? Can I do this?: typedef void F(String s);
class A { void f(@checked num x) { } }
class X { F h; }
void main() {
A a = new A();
X x = new X();
x.h = a.f;
} |
The static type of the tearoff So, that example should still be a static type error on the assignment to |
We currently give a strong mode error on the override in
B
below:We could downgrade this to a warning (and in DDC add a corresponding runtime check in
B.foo
).@yjbanov @leafpetersen
The text was updated successfully, but these errors were encountered: