-
Notifications
You must be signed in to change notification settings - Fork 205
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
Should extension structs allow overrides in sub-classes? #2369
Comments
In general, allowing this concerns me less than the question of overriding things from interfaces (discussed in #2360), since we only allow extending other extension structs, and there should be no expectation of interface polymorphism around extension structs. In contrast, with interfaces, when implementing a class interface, there is a definite expectation of polymorphism. |
I see no problem with overriding and changing behavior based on static type. That's what extension structs do (or views, or extension types, or even normal extension methods). The moment someone chose to use "extension"-something, they opted in to static type being the determining factor. I'm more concerned with whether |
I think this is a bit too much of a footgun, because static dispatch contradicts the behavior of object-oriented dispatch and member overriding, and yet it looks so similar. extension struct BaseInteger(int x) {
String get foo => 'base';
String getFoo() => foo;
}
extension struct DerivedInteger(int x) extends BaseInteger {
String get foo => 'derived';
}
void test() {
var i = DerivedInteger(2);
print(i.foo); // 'derived'.
print(i.getFoo()); // 'base'.
} It's tempting to say that "static overriding" is so different from standard OO overriding that we just shouldn't have the former. If we disallow overriding then we'll have to rename |
Maybe this should be called "shadowing" instead of "overriding"? And maybe have a lint for adding an |
If the concern is around the call site looking too similar to OO overriding, can we imagine a lint telling users to explicitly cast when they're using a supertype that contains a method that is overridden in a subtype? For example: extension struct BaseInteger(int x) {
String get foo => 'base';
}
extension struct DerivedInteger(int x) extends BaseInteger {
String get foo => 'derived';
}
void test() {
var i = DerivedInteger(2);
print(i.foo); // OK
var b = i as BaseInteger;
print(i.foo); // LINT, `foo` is overridden in DerivedInteger - prefer a cast to avoid confusion.
print(BaseInteger(i).foo); // OK
} Perhaps this is excessively verbose. It might also be noisy, as the value may never have been used with class A {}
extension E1 on A {
String get name => 'A';
}
class B extends A {}
extension E2 on B {
String get name => 'B';
}
void main() {
var b = B();
var a = b as A;
print(a.name); // prints A, not B
} |
My main concern is that the direct invocation of an overridden method "works" (the "overriding declaration" is executed), but an indirect invocation may well invoke the "overridden declaration": In the example, we call I think that's too deceptively similar to OO overriding. Here's the example again, for easy reference: extension struct BaseInteger(int x) {
String get foo => 'base';
String getFoo() => foo;
}
extension struct DerivedInteger(int x) extends BaseInteger {
String get foo => 'derived';
}
void test() {
var i = DerivedInteger(2);
print(i.foo); // 'derived'.
print(i.getFoo()); // 'base'.
} |
Ah, I was wondering why you included You may have discussed this elsewhere, but would maybe changing the keyword |
If we don't have overriding (or shadowing, or whatever we want to call it) then the situation may actually be OK, because it is consistent with OO dispatch. So we don't need to remember that overriding works differently with extension structs, because there is no overriding, and the other things work the same: extension struct Base(Object? _) {
String get foo => 'base';
String getFoo() => foo;
}
extension struct Derived(Object? _) extends Base {
String get bar => 'derived'; // Can't override, use a different name.
}
void test() {
var i = Derived(2);
print(i.bar); // 'derived'.
print(i.foo); // 'base'.
print(i.getFoo()); // 'base'.
} If the static type is The story is quite simple: Overriding is an error with extension structs. If we need it, we should use a class or a struct. |
@ds84182 I would like to have some verbiage to distinguish between arbitrary shadowing (e.g. replacing a method returning an |
I don't really see why we would ship a feature that allows you to override
Extensions don't define new types, so I think they're a little different. But I agree that particularly for |
Right, I agree, but I feel like it's a little more intuitive to grasp this when they're tied to a feature that exists already (extensions). @rileyporter had some concrete examples that I believe are harder to work around without overriding that I don't remember but I hope she can share. If we can address some of these or at least have a consistent answer to them, then I feel less iffy about disallowing overriding. |
For the For example, returning a library 'dart:html';
class Node {}
extension NodeExtension on Node {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
} library 'my_list_library';
import 'dart:html' as html;
// Doesn't work, the `on` type needs to be Node and have NodeExtension.childNodes hidden
extension ListNodeExtension on html.NodeExtension {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} The behavior we'd hoped for was when With extension structs, I don't think that really makes sense, but it might look something like: // library 'dart:html'
extension struct Node() {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
}
// library 'my_list_library'
extension struct ListNode() extends html.Node {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} In order to get the behavior we want, I think we'd have to add casts to In that example, the return type of // library 'dart:html'
extension struct Node() {
set nodeValue(String? newValue) {
js_util.setProperty(this, 'nodeValue', newValue);
}
}
// library 'my_safe_html'
extension struct SafeNode() extends html.Node {
set nodeValue(String? newValue) {
js_util.setProperty(this, 'nodeValue', sanitize(newValue));
}
} The desired behavior being when |
@rileyporter, I think we can have the kind of features you want, with a twist. Here is a variant of your examples using views: // Library 'dart.html'.
class Node {}
view NodeView on Node {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
}
// Library 'my_list_library'.
import 'dart:html' as html;
view ListNodeView on html.NodeView {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} With views we will have the subtype relation I also tend to think that layering views on top of views is a subtle and complex approach, and it's much better to use a simpler relationship where each view in a family of related views are directly related to the underlying non-view-type ( The simplest possible fix is to keep the two views unrelated: // Library 'dart.html'.
class Node {}
view NodeView on Node {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
}
// Library 'my_list_library'.
import 'dart:html' as html;
view ListNodeView on Node {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} Now both views are connected with However, if // Library 'dart.html'.
class Node {}
view NodeView on Node {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
}
// Library 'my_list_library'.
import 'dart:html' as html;
view ListNodeView extends html.NodeView on html.Node {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes')); // ERROR!
} However, this is a compile-time error because there is a collision between the two members named import 'dart:html';
import 'my_list_library';
void main() {
Node node = ...;
NodeView nodeView = node; // OK.
var list1 = nodeView.childNodes;
ListNodeView listNodeView = nodeView; // OK.
var list2 = listNodeView.childNodes;
} We don't get any errors (compile-time or run-time), and we do get a If we had given those two methods the same return type then we're not very likely to see the problem immediately. As seen from a class point of view, we are calling We could use a modified // Library 'dart.html'.
class Node {}
view NodeView on Node {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
}
// Library 'my_list_library'.
import 'dart:html' as html;
view ListNodeView extends html.NodeView hide childNodes on html.Node {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes')); // OK.
} This means that there is no subtype relationship between import 'dart:html';
import 'my_list_library';
void main() {
Node node = ...;
NodeView nodeView = node; // OK.
var list1 = nodeView.childNodes;
ListNodeView listNodeView = nodeView; // ERROR! But we could do `= nodeView as node`.
var list2 = listNodeView.childNodes;
} We will use static resolution of member invocations (that's basically the core property of a zero-cost abstraction), but we won't silently switch from one set of implementations to a different one, so we have to have an explicit cast to "leave" the abstraction I expect that we would have the same trade-off with extension structs: // library 'dart:html'
extension struct NodeES(Node node) {
NodeList get childNodes => js_util.getProperty(node, 'childNodes');
}
// library 'my_list_library'
extension struct ListNodeES(Node node) extends html.NodeES {
List<html.Node> get childNodes => wrap(js_util.getProperty(node, 'childNodes')); // ERROR!
} Again, we could choose to say "No problem, go ahead and override what you want!", but I think that the semantics of doing so is error prone. |
Could the "connection" then be to cast the extension NodeView on Node {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
String get name => "NodeView";
}
extension ListNodeView on Node {
List<Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
void printName() => print(NodeView(this).name);
} |
I'd expect views to be used for a while with the same object. So the typical situation would be that we get hold of an object, and the static type is a specific view, and then we use that object according to that static type for a while. NodeView someFunction(...) {...}
void main() {
var nodeView = someFunction(...);
// Now do 100 things with `nodeView`.
} The point is that the view is a complete abstraction (like a class), so we should be satisfied with the affordances offered by that view, that is, we should be able to do what's typically done with such objects without ever considering which type is the on-type of the view. However, we need to get started, e.g., to obtain a reference of type If the abstraction isn't sufficient to handle the given task we may need to "exit" from the abstraction again, that is, we may need to get a reference to the underlying object using the on-type. If the view is So I wouldn't expect casting into and out of a view type (or an extension struct type) to be very common, it's basically a violation of the soft notion of encapsulation that the view / extension struct provides. If you are more happy about a perspective where the underlying object keeps its own type (so there's no encapsulation) and the extension/view members are just added to the existing members, then it's probably better to use an |
Thanks for the input! I do think views are what we want, but maybe extension structs would allow for enough of the same functionality. I think the closest to what we'd want is your example with the One tweak though, I think we'd want the For example: // Library 'dart.html'.
view Node on JSObject {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
}
// Library 'my_list_library'.
import 'dart:html' as html;
view ListNodeView extends html.Node hide childNodes on JSObject {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} And the usage would be: import 'dart:html';
import 'my_list_library';
void main() {
Node node = ...; // likely some other JSObject view API like getRootNode()
var list1 = node.childNodes; // OK, type of list1 is NodeList
ListNodeView listNodes1 = node; // ERROR?
ListNodeView listNodes2 = node as JSObject; // OK
var list2 = listNodes2.childNodes; // OK, type of list2 is List<Node>
ListNodeView listNodes3 = node as ListNodeView; // Could we make this be OK?
var list3 = listNodes3.childNodes; // OK, type of list3 is List<Node>
var list4 = (node as JSObject).childNodes; // Error? What would the type of list4 be?
} Also, I could see |
Yes, the views should actually be declared as you mention. That's also how it was done in discussions we've had about JS interop.
The cast from However, like all casts, it is a forced move for the compiler, because we can cast whatever we want to whatever we want (e.g., Given that the run-time representation of a view / extension struct type is the underlying representation type, this requirement will be enforced at run time, no matter which cast we're using. import 'dart:html';
import 'my_list_library';
void main() {
Node node = ...;
var list1 = node.childNodes; // OK, type is `NodeList`.
ListNodeView listNodes1 = node; // Compile-time error.
ListNodeView listNodes2 = node as JSObject; // OK.
var list2 = listNodes2.childNodes; // OK, type is `List<Node>`.
// This is probably the most direct and natural approach.
ListNodeView listNodes3 = node as ListNodeView; // OK.
var list3 = listNodes3.childNodes; // OK, type is `List<Node>`.
var list4 = (node as JSObject).childNodes; // Compile-time error.
} I think the cast from In contrast, the cast from In this particular case we do not have the subtype relationship However, if we are able to avoid the overriding conflict and just describe class SomeType {}
class SomeSubtype implements SomeType {}
view BaseView on SomeType {
void base() {...}
}
view DerivedView extends BaseView on SomeSubtype {
void derived() {...}
}
void f(BaseView baseView) => baseView.base();
void main() {
DerivedView derivedView = SomeSubtype();
derivedView.derived();
f(derivedView); // OK.
} |
True, that could yield a verbose declaration. The reason why I proposed that every non-inherited member should be mentioned separately is that this allows views to be optimized for code reuse. Classes are optimized for conceptual modelling and encapsulation, and it's crucial that a subclass is a subtype, and subtypes admit substitutability, such that we can provide an instance of a subtype So I aimed for optimal freedom in the management of sets of method implementations, i.e., code reuse. This is also the reason why views support multiple superviews in the We could come up with some mechanisms to make the syntax less verbose. E.g., we could use |
I'm a little confused. I do not believe that any of the proposals we have out there give you this behavior. Not |
1 similar comment
I'm a little confused. I do not believe that any of the proposals we have out there give you this behavior. Not |
@leafpetersen you're right, I don't think any of the proposals really provide the behavior to override specific members on a type. I think how we'd get close to this right now with extension methods is redefining the class we want to "override" behavior in: library 'dart:html_basic';
class Node {}
extension NodeExtension on Node {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
} library 'dart:html_full';
class Node {}
extension ListNodeExtension on Node {
List<Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} The user code can import one or the other library and refer to just import 'dart:html_basic';
Node n = ...;
n.childNodes; // returns a NodeList import 'dart:html_full';
Node n = ...;
n.childNodes; // returns a List<Node> The way we'd make this work for dart:html coverage is something like: library 'dart:html_full';
import 'dart:html_basic' as html_basic;
export 'dart:html_basic' hide Node; // provide the rest of the DOM API for dart:html
class Node {}
extension ListNodeExtension on Node {
List<Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
// re-implement all other html_basic.Node members, possibly with forwarding to the basic definition
void appendChild(Node n) => html_basic.Node.appendChild(n);
} That kind of gives us "overriding" behavior, since we're replacing the functionality of And if we don't want to override behavior, we can implement the class Foo implements html_basic.Foo {}
extension FooExt on Foo {
// get all of html_basic.Foo extension members for free
void someNewBehavior() => ...;
} |
I think I'll dive one step deeper on this one! 😄 @rileyporter wrote:
Let me try to understand the situation. I can see that I'm not 100% sure how to understand the semantics of these enhanced extensions, but I think we can simplify the situation by going back to the JS interop approach that I mentioned earlier (that is, we use // ----- Library 'dart.html'.
view Node on JSObject {
NodeList get childNodes => js_util.getProperty(this, 'childNodes');
Node? get firstChild => js_util.getProperty(this, 'firstChild');
}
// ----- Library 'my_list_library'.
import 'dart:html' as html;
export 'dart:html' hide Node;
view Node extends html.Node hide childNodes on JSObject {
List<html.Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} This looks promising. In particular, we have the following properties: An importer of 'my_list_library' receives a definition of the type We're getting close to this:
However, it is still fundamentally broken: We already get a hint in the signature of We could change the return type to be So we're really looking at a situation where we wish to change the meaning of one entity declared together with a rather large set of other entities (and there is a lot of mutual recursion). This requires all of them to be updated simultaneously. (Mumbo jumbo: If we consider a library to be similar to a class and introduce virtual classes as members of libraries, then we can "update" a set of mutually recursive classes by redefining just the one(s) we actually want to change, and then all the others will be re-wired to use the updated declarations. But we won't do that with Dart, because that's a completely radical change of the language.) The only realistic approach I can see for that is to keep the two sets of classes completely separate, and duplicate all the declarations that have any elements of mutual recursion (and that's probably every single declaration except things like As an alternative, we could consider changing class C { void foo() { print('C.foo!'); } }
extension E on C {
int get foo => 42;
}
void main() {
var c = C();
c.foo(); // OK, no name clash, prints 'C.foo!'.
var i = E(c).foo; // OK, we can force the use of the extension member.
} If we allow an extension declaration to have higher priority than an instance member with the same name (surely we won't do that, but let's just consider that possibility) then we could force all invocations of Then we'd have the same 'dart:html' as shown above, plus this 'my_list_library': // ----- Library 'my_list_library'.
import 'dart:html';
// Assume that we add support for `override` on extension members.
extension ChildNodesIsList on Node {
override List<Node> get childNodes => wrap(js_util.getProperty(this, 'childNodes'));
} However, that implementation (and signature) of |
If we were to do something like you hypothesized with
becomes a larger problem. If you have another library library third_party_library;
import 'my_list_library';
Node n = ...;
List<Node> getNodes => n.childNodes; import 'dart:html';
import 'third_party_library';
Node n = ...;
var list1 = n.childNodes; // type NodeList
var list2 = getNodes(); // type NodeList, but third_party_library expected to return `List<Node>` I think what we'll end up doing for library dart:html;
view Node on JSObject {
NodeList childNodes => ...;
String name => ...;
}
view Element on JSObject {...} library dart:html_extra;
import 'dart:html' as html;
view Node on JSObject {
List<Node> childNodes => ...;
String name => html.Node.name;
}
view Element extends html.Element on JSObject {
// get html.Element definitions through extension
void addExtraBehavior() => ...;
} User code will either get the library third_party_library;
import 'dart:html_extra';
Node n = ...; // html_extra.Node
List<Node> getNodes => n.childNodes; // List<html_extra.Node> import 'dart:html';
import 'third_party_library';
Node n = ...; // html.Node
var list1 = n.childNodes; // html.NodeList
var list2 = getChildNodes(); // List<html_extra.Node> Is that correct, or am I misunderstood on how |
Note that the recently updated view class proposal does allow for one view method to override another one. As I mentioned here, this relation is very different from an object-oriented method override, because the choice about which implementation to execute is made based on the static types. However, there is one safe step that will make the distinction: Control-click (or whatever it is that yields jump-to-declaration for a method call with the given tools) and check whether the enclosing declaration is a class or a mixin. If so (then it's a regular instance method) and you have just looked up the statically known declaration; the one that actually runs is the most specific override of that declaration in the run-time type of the receiver. If it's not (then it's an extension method or a view class method) and you have just found exactly the code which will be executed at run time. |
In the extension struct proposal (#2360) I initially propose to disallow extension structs to extend other extension structs. #2368 discusses whether to allow this. If we allow this, the question then arises of whether or not to allow overriding. This is discussed briefly in this addendum to the proposal. A concrete concern with allowing overriding, is that the semantics for extension structs members is non-virtual dispatch (think extension methods), and hence override behavior can be surprising:
While this is surprising, it also seems like a large restriction to forbid API authors from modifying the implementation when extending. This issue is for discussion of this design choice.
The text was updated successfully, but these errors were encountered: