-
Notifications
You must be signed in to change notification settings - Fork 214
Mixin declaration allowing super-invocations. #7
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
Comments
This seems like a reasonable proposal. A possible slightly change would be to use the word "class" instead of the word "mixin" to declare mixins, and have that automatically define both a mixin and a class, and then to enforce the rules for mixins only when the identifier is used as a mixin. It seems like that would get all the benefits of the proposal while simultaneously avoiding having a backwards-incompatible change in syntax. Is there a reason I'm missing why that would not be possible? |
I'm worried the
In practice, most classes that we see today used as mixins are intended to only be used as mixins. Many even put "Mixin" in the class name. So it doesn't seem like having dual-use types is a highly valuable affordance. By prohibiting using the mixin as a class, it means the author of the mixin doesn't have to worry about accidentally breaking users when they change the mixin in a way that makes it no longer a usable class (like adding an abstract method, etc.). It's roughly analogous to sealed classes, final methods etc. It lets the author narrow the scope of what future changes may break users. Also, I think it gives users a clearer expectation of how to use the type. It lets the API author give users a more "on rails" experience. It might be worth adding a way to explicitly opt in to defining a "mixin and class" type, but my feeling is that it's not a good default behavior. |
It's definitely possible, with various caveats. It has some downsides though. One (mentioned in the proposal) is simply that allowing classes to be used as mixins makes it harder for library writers to control what is a breaking change. So having a separate construct for the separate intended use provides some benefits independent of anything else. Another downside (or advantage of the new syntax, if you prefer) is that the class syntax doesn't tell the client which interfaces should be satisfied by the superclass, and which can be satisfied in other ways (e.g by the mixin itself, or by the class that the mixin is being mixed into). Compare: class A extends B implements C, D {...} versus mixin M on B, C implements D { ...} the latter makes it clear that There are also compilation benefits to knowing whether something is intended to be used as a mixin or a class, particularly in a modular setting. If a compiler needs to be prepared for the possibility that any suitable class might be used as a mixin, then it is more restricted in the set of optimizations that it can do (such as inlining super calls). |
This was considered. I don't have strong feelings either way. The @lrhn might have more thoughts. It's something I'd definitely be interested in hearing from other folks about. |
Just to clarify, I definitely like the "mixin" keyword, and I think it's a good idea, for all the reasons given above. I'm greatly concerned about yet another breaking change, however. One option would be to support both but have a lint that discourages the old style. |
(To handle the compiler issue, one option would be to have a compiler mode that just disables the legacy syntax for codebases that know they don't use it.) |
My impression from talking about and explaining mixins to lots of users is that almost none of them actually internalize the Bracha-ian notion that a mixin is a function that is applied to a superclass, so "on" probably doesn't illuminate much. Also "on" to me reads like a thing it does not a thing it needs.
The doc does state:
My expectation is that we would add the new syntax, only allow super calls in mixins that use the new syntax but also support the old syntax and semantics for regular non-super-calling mixins. The set of mixins that use super calls is vanishingly small. I wouldn't be entirely surprised if you were literally the author of a majority of them. :) |
For the For the backwards-compatibility issue: Could we instrument the analyzer to catch such cases, and anonymously report them? Flutter in particular has an analytics mechanism we could reuse for this. Obviously if all the uses are in the Flutter framework then there's no breakage concern. |
Just to be sure that we're on the same page, we're not intending to remove support for using classes as non super-mixins , at least yet. We'd like to do so in the future, but that would depend on driving usage low enough, and even then... well, we'll see. For super-mixins, there is no existing web code that uses them (they're not implemented), and the feature is not available even on the VM without turning on the experimental flag. So it's not breaking for direct consumers of the Dart SDK. Unfortunately for us, of course, the flutter tools ship with the experimental flag turned on. So this will be a breaking change for flutter. This is actually true whether we introduce new syntax or not: see below for an example of two bad uses of super-mixins that I really, really, really want to disallow, whether we use new syntax or old. But it's certainly true that keeping the old syntax would make this less breaking. This change is primarily breaking for library writers though. If you wrote the super-mixin correctly, then most of the time uses won't need to change. It's only the definition of super-mixins that will need to change. So most flutter users shouldn't even know this has happened. If their code worked before, it should continue working. The cases that they will be broken are:
My hope is that if we get out ahead of this with communication, this can be a minimally disruptive change to flutter users (and to the flutter framework). WDYT? // This program passes the static analysis, and runs in the dart VM (throwing an error on the missing super call)
class A {
void bar() {}
void foo() {}
}
class B {
void bar() {}
void foo() {}
}
abstract class M extends A implements B {
void baz() {
super.bar();
super.foo();
}
}
// Doesn't provide an `A` to `M`.
class C extends B with M {
}
abstract class AbsA implements A {
void bar();
void foo();
}
// Doesn't give `M` the required super methods
abstract class D extends AbsA with M {
}
class E extends D {
void bar() {}
void foo() {}
}
void main() {
C().baz();
E().baz(); // Runtime error
} |
I'd be very happy to see data on this, with the caveat that for lots of reasons we need to move fast on this (not least of which is that the longer we wait, the bigger the chance that we break people). For small syntax changes like this, I think it would be reasonable for us to implement the feature simultaneously with doing surveys, and then just change the keyword if based on data before shipping to users. We'd need to get going on data gathering ASAP though.
I would love to start getting analytics data about language and core library feature usage. Even if it's not feasible in time for this feature, it would be awesome if we could start getting this set up for future use. Not sure what would be involved - I don't think it would be technically hard in the analyzer, but there are lots of tricky issues. cc @devoncarew @bwilkerson |
I split off a new issue for discussion of the syntax choice for superclass constraints: #9 . |
I would also love to have that data available. There are two issues I can think of:
|
Which experimental flag in the VM are you referring to? |
You could probably get pretty far just analyzing the top n Dart repos on github.
For the analyzer at least, this is enabled via:
We can get data on how often this is enabled, but I expect it's a very small number of users. |
Hmm, is this on by default in the VM? In any case, it's not on by default in the analyzer, and not supported by any web implementation. |
I split off an issue here for discussion of the breaking change implications. |
Just to be precise: The |
We discussed the superclass constraints a number of times. Just for the record, here are some reasons why I think the current proposal offers a bit too much red tape and a bit too little actual help: Refactoring from classes may failThe issue here is that the superclass requirement (the For example, assume that a class class A {
int foo() => 42;
}
abstract class I {
int foo();
}
class B extends A implements I {
int bar() => foo();
int foo() => super.foo() + 1;
} If we make an attempt to express the same thing using a mixin and a mixin application, we hit the superclass constraint: class A {
int foo() => 42;
}
abstract class I {
int foo();
}
mixin M on I {
int bar() => foo();
int foo() => super.foo() + 1;
}
class B = A with M; // Error, `A` does not implement `I`. So the problem is that we cannot create mixins by refactoring existing classes, because a normal class will allow an inherited member to contribute to an implementation requirement, but a mixin application will not. We could try to use This is a problem because mixins are intended to help abstracting away multiple identical class bodies, and this means that they should be similarly powerful. Here, 'powerful' just means 'non-stubborn' because there is no technical problem in using a structural rather than nominal superclass constraint. A subtype check is not sufficient to ensure correct overridingIt would be useful if checking the superclass constraint were sufficient to ensure that a mixin application succeeds: That is, check that the superclass (We already know that the superclass constraint does not include information about which methods are implemented and which ones are not implemented. That is, we may make an attempt to apply a given mixin Consider the following example: abstract class I {
int foo();
}
class A implements I {
int foo([int x]) => 42;
}
class M on I {
int foo() => 43;
}
class B = A with M; // Error, `M.foo` cannot override `A.foo`. So the problem is that The sound "direction" for the constraint in the case of overriding is the reverse direction of the one that is used to satisfy an So for overriding, the What can we do?One possible approach is to say that we specify both an overriding interface ( That's really heavy, though. We could also give up on checking superclass overridings as such and only require that superinvocations actually hit an implementation with a suitable signature. This means that we would specify only the methods Finally, we could just drop the |
The immediate refactoring of mixin M on A implements I {
int bar() => foo();
int foo() => super.foo() + 1;
}
class B = A with M; This works and behaves exactly like the original declaration of It's true that a sub-type check is not sufficient to statically guarantee that a super-class implementation matches its interface because classes can be abstract and not implement their own interface. The current approach is a trade-off between lightness and static guarantees. We don't want something heavier. That includes specifying the actual methods being super-invoked. Dropping the requirements clause also removes a very important part of documentation. The class is intended to mix-in on a specific super-class, but if you can't specify that, then you don't get any local errors, only remote errors at the application points, and people can start using it on classes it's not intended for. All in all, I think the current design is a good trade-off. |
The immediate refactoring of
My point is that if we want to specify a superclass interface then the purpose of that superclass interface should be that it creates an encapsulated abstraction: We check a given actual superclass However, if we insist that actual superclasses must be subtypes of a specified set of types then we are not helping developers: The overrides will succeed if the actual superclass has exactly the specified method signatures; if the actual superclass has more specific method signatures then it is likely to fail (as in my example); and if the actual superclass has less specific method signatures (that is, we have the inverse override relation from the In other words: For overriding, we prevent a lot of cases that would work, we allow one case that will work, and we allow a lot of cases that won't work. To me, that sounds more like red tape than it sounds like a helpful mechanism. I still think it would be more pragmatically useful to specify an |
It's certainly annoying that we don't get encapsulation of the override errors, but as @lrhn points out, we've already lost that in Dart. When you override, you have to check against all implemented interfaces, and for a mixin the override "happens" at the application point. We could drop the This design seems to match up pretty well with the few uses we have in practice. |
With the current proposal we allow super calls that are statically known to fail. Consider this abstract class I {
foo([a]);
}
class A {
foo() => 42;
}
abstract class B extends A implements I {}
mixin M on I {
// This will fail because B doesn't have a concrete implementation of I.foo.
foo([a]) => super.foo(a);
}
class C = B with M; // No compile-time error since B (promises to) implements I.
main() => new C().foo(0); // Results in a super-noSuchMethod error at runtime. |
I agree that mixin declarations have no checks for concreteness on the methods invoked in superinvocations (so the static checks on But the proposal says that
so I believe that we would actually catch that one at compile-time with the current proposal: |
Good. I must have missed the mentioning of 'concrete implementation'. |
The tests are not exhaustive, but they can hopefully be a good start when implementing the feature. The test are not *checked* since there is currently no implementation of the feature, or even the syntax. They should be expected to contain some errors. Bug: dart-lang/language#7 Change-Id: I4a5364566aeba9de036e56ae188205337575154c Reviewed-on: https://dart-review.googlesource.com/70509 Commit-Queue: Lasse R.H. Nielsen <[email protected]> Reviewed-by: Leaf Petersen <[email protected]>
@rmacnak-google - are the |
Yes, the language team is responsible for making corresponding changes to the mirror API for every language change. |
Are there any details on the |
The old super-mixins are going away. Old non-super mixins will continue to be supported. |
This is news to the language team. :) I think it does make sense for dart:mirrors to be on the list of implementations against which we should file issues when we make language changes though. I'll add something to the tracking issue and the template. |
Closing, this was included in Dart 2.1 |
Uh oh!
There was an error while loading. Please reload this page.
Solution for issue #6.
See the feature specification document.
Implementation issue: #12
Introduce a new syntax for declaring mixins:
This declaration introduces a mixin, like one derived from a class, except that the mixin can only be applied to a super-class which implements
SuperType1
andSuperType2
. In return, it can then do super-invocations targeting members of theSuperType1
orSuperType2
interfaces.Each time this mixin is applied to a super-class, it must check that all the members that are invoked by super-invocations actually have implementations in the super-class which satisfies the most specific requirement of
SuperType1
andSuperType2
for that member.The text was updated successfully, but these errors were encountered: