-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fasta strong mode inference confused same class extending Base<C> and mixing class extending Base<B> #31656
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
@stereotype441 if there is something quick we could patch into the Fasta inference to make this case work that would also be appreciated. |
Uh oh. It's a fundamental limitation of both kernel and fasta, as currently implemented, that no class can implement any other class multiple times with different type arguments. IIRC, this limitation was inherited from dart2js, and at some point in the last few months I believe @leafpetersen and @lrhn discussed the idea of making the limitation explicit in the Dart 2.0 spec. @leafpetersen can you confirm? Fully relaxing this limitation would be nontrivial, because it underlies the design of the kernel ClassHierarchy API, which is used pervasively. Specificially, Unfortunately, I can't think of something quick we could patch into Fasta inference to make this work. The best workaround I can think of is as follows: partially relax the limitation, changing the rule to say "a class can implement another class multiple times with different type arguments, provided that one of the implemented types is a subtype of all the others". Then we could modify So in the example above, @kmillikin the would all have to be made to I'm happy to discuss this in more detail over VC if that would be helpful. |
Here's an idea for a quicker (but more fragile) workaround: change the superInfo.genericSuperTypes?.forEach((Class key, Supertype type) {
subInfo.genericSuperTypes[key] = substitution.substituteSupertype(type);
}); to this: superInfo.genericSuperTypes?.forEach((Class key, Supertype type) {
if (!subInfo.genericSuperTypes.contains(key)) {
subInfo.genericSuperTypes[key] = substitution.substituteSupertype(type);
}
}); When determining which interfaces a given class implements, the supertype is visited first, then mixins, then classes in the "implements" clause. This will ensure that the interface seen first dominates, so in the example above, the You'll also need to make the same change to |
I can confirm that we sanctioned the "no implementing two different instantiations of the same interface" rule for Dart 2. So no, it's not supposed to compile in Dart 2. This is one of the reasons we can even add the "extractTypeParameter" function - otherwise its behavior would be underdefined, and it makes the "flatten" spec function well-defined too. It's a good thing. If being this restrictive turns out to be a problem in practice, we might be able to loosen that in the future, by allowing multiple implementations of the same interface, as long as one of them is the most specific one, and that is the one that is used whenever it matters. |
Could we relax it now rather then in the future? It seems absolutely reasonable that application should be able to satisfy mixin requirement with a more specific class. It already works when generics are not involved, so it's not clear why it should not work when generics are involved, especially when these generics are fully instantiated like here. This seems to be a pretty common pattern in Flutter. To give you a real example: abstract class State<T extends StatefulWidget> extends Diagnosticable {
T get widget;
}
abstract class AutomaticKeepAliveClientMixin extends State<StatefulWidget> {
}
class _DismissibleState extends State<Dismissible> with TickerProviderStateMixin, AutomaticKeepAliveClientMixin {
bool get _directionIsXAxis {
// here widget is expected to be Dismissable
return widget.direction == DismissDirection.horizontal
|| widget.direction == DismissDirection.endToStart
|| widget.direction == DismissDirection.startToEnd;
}
} As a workaround I guess I could rewrite this code as: class _DismissibleState extends State<StatefulWidget> with TickerProviderStateMixin, AutomaticKeepAliveClientMixin {
@override
Dismissible get widget => super.widget;
bool get _directionIsXAxis {
return widget.direction == DismissDirection.horizontal
|| widget.direction == DismissDirection.endToStart
|| widget.direction == DismissDirection.startToEnd;
}
} but that also affects few other methods in |
I try not to understand mixins, (with a fair bit of success I think it's fair to say), but while I had the same initial thought (that this produced something with two different interfaces at the same type), is that really the case (or rather, should it be)? My understanding of abstract class M extends Base<B> {
} when thought of as a mixin would be that If they are usable as interfaces, then perhaps there's a problem, I'm not sure. It doesn't seem like you need to keep both around on the instance, but you do presumably need to know that Ahh mixins. |
What you are seeing is a consequence of using class syntax to introduce mixins (something it's not very well suited for, and the reason we want to introduce dedicated mixin declarations). abstract class M extends Base<B> {} This is a class declaration. It might be intended as a mixin, but here it is just a class declaration. The class When you do a mixin application of the mixin derived from I don't remember if we ever decided whether the new mixin syntax would introduce an interface. I'd be willing to say no, because then it wouldn't automatically implement the super-constraints. Now, I'm not sure mixins is the only problem here. If we have family polymorphism, like we do in Flutter in some cases, then it's likely we'll see code like: abstract class Widget<S extends WidgetState> { ... }
class TextWidget implements Widget<TextWidgetState> { ... }
class UnderlinedTextWidget extends TextWidget implements Widget<UnderlinedTextWidgetState> { ... } (I don't know Flutter well enough to say that it happens, but I wouldn't be surprised). The question is whether we want to support it, or require it to be rewritten into: abstract class Widget<S extends WidgetState> { ... }
class TextWidgetBase implements Widget<T extends TextWidgetState> { ... }
class TextWidget extends TextWidgetBase<TextWidgetState> { }
class UnderlinedTextWidget extends TextWidgetBase<UnderlinedTextWidgetState> { ... } (which is an option). |
My preferred state would be that mixins have their own syntax, and are thought of as functions from classes to classes rather than classes in an of themselves, and hence have no interface of their own (or alternatively, have a higher order interface : Obviously, we're not there yet, and we need to get flutter up and running, so I don't see any good alternatives other than allowing multiple interfaces. It seems like the proposal to allow classes to implement the same interface multiple times so long as there was one most precise version of the interface would be reasonable: it seems like at least for runtime subtyping purposes implementations could only record the most precise interface. For static checking the implementations would need to record all of them. Does this seem right? Can dart2js still use its existing approach and be compliant with this, or are there corner cases where it needs to know all of the super-interfaces? This is subject to the restriction we discussed, that all concrete method implementations must satsify all of the super-interfaces, not just the most specific one. |
I don't see why static checking would be different, as long as it is just subtype checks.
The one corner-case I can think if is covariant parameters. We have so far said that an overriding covariant parameter must be a sub- or super-type of every super-interface parameter that it overrides. The other corner-case is someone wanting to implement interfaces that implement Apart from that, @johnniwinther WDYT? |
The dart2js implementation won't work out-of-the-box. The current implementation assumes that the type-hierarchy of the the superclass can be augmented to create the type-hierarchy for the class itself. With the proposed change this will not be possible, since for instance Having done this through-out dart2js, though, the dart2js runtime has no problems with the new rules. |
I will try to fix this in Kernel's type hierarchy by always choosing the more specific instantiation. |
The fix for this will be easier when we fix the related issues #30674 and #31118. The code we generate for the named mixin application abstract class Base&M<U> = Base<U> with M;
class X extends Base&M<C> ... The mixin application class |
I think this is a critical issue to fix so we can make progress on getting flutter running in strong mode. Also I don't understand why issue #30674 which is related per comment above is not marked at severity P1 |
If you want reproduction instructions compiling the flutter code, you would have to check out flutter/engine and flutter/flutter and then execute - <flutter_engine_checkout>/engine/src/out/host_release/dart-sdk/bin/dart <flutter_engine_checkout>/engine/src/out/host_release/gen/frontend_server.dart.snapshot --sdk-root <flutter_engine_checkout>/engine/src/out/android_release/flutter_patched_sdk/ --aot --strong This will produce a slew of errors -
|
Moving to P0, this is blocking #31789 |
The problem doesn't reproduce in this example that should be equivalent. This is mostly a note to myself, I'll pick this up tomorrow.
|
@ahe I think this is specific to having the same generic class as multiple super-interfaces, instantiated at multiple different types. |
@leafpetersen that is correct. However, as I discussed this with @sjindel-google, he pointed out that these situations are similar. Since we add a forwarding stub in my example above, that's probably also how we should solve it for the case reported in this bug. |
I focused on implicit new for constructor invocations today. Given how much progress I made on that, I'm comfortable dropping everything else and attempt to fix this tomorrow. However, if @stereotype441 can extend pkg/front_end/lib/src/fasta/type_inference/interface_resolver.dart to handle this case as well as it handles BoxCase above, that would be most welcome! |
For the sake of completeness: I have tried workaround from the comment #31656 (comment) above and it did not fix the issues we are seeing with Flutter. Now we see even more issues that all have this issue as root cause. Now the earliest assertion you hit when you try to run Flutter in strong mode is this:
I think that's the very same issue because class DrawerController extends StatefulWidget {
}
class DrawerControllerState extends State<DrawerController> with SingleTickerProviderStateMixin {
}
abstract class SingleTickerProviderStateMixin extends State<dynamic> implements TickerProvider { // ignore: TYPE_ARGUMENT_NOT_MATCHING_BOUNDS, https://github.com/dart-lang/sdk/issues/25232
} |
@jensjoha came up with a temporary workaround. @mraleph could you try it?
|
That workaround only seems to work for simpler cases. Nothing is overwritten in Slava's original example. |
We're making progress: ClassHierarchy now is able to identify the problematic interfaces instead of picking a random. But we're not there yet: a quick hack similar to Jens' solution above doesn't remove all compile-time errors. |
This works around the issue by reporting an error that can be ignored and recovers in a similar fashion to what @stereotype441 suggested (prioritize the extends clause). |
Has the work around landed? |
@a-siva we are working on it. I would say it'll still take few days to arrive to the completely working solution. |
I understand a fully working solution would take days but landing the work around will help us make short term progress verifying changes being made for #31869 |
The work around isn't completed yet, and as @mraleph said, it will still take a few days to get it completely working. I'm marking this as P0 again. @mraleph and I agreed to downgrade to P1 as we had agreed on a workaround by rewriting Flutter code. However, that turned out to be infeasible, and we decided to invest our time last night in making progress on brainstorming and implementing a fix in Fasta instead of providing detailed information in this bug report. We naively assumed that would suffice given that we had also provided a link to the CL and stated clearly that "we're working on it". |
Unfortunately, my implementation isn't compatible with pkg/front_end/lib/src/incremental_kernel_generator_impl.dart, and So I'll have to remove these first. Fortunately, I have replacement ready for them in pkg/front_end/lib/src/fasta/incremental_compiler.dart. This implementation is already faster than minimal, but not as fast when it comes to body-only changes as the original IKG. However, we already have a prototype implementation that optimizes for body-only changes and then there are no performance regressions at all: |
We've removed the old implementations of IKG, and should be able to land change 33884 shortly. |
Fixed in 3fcede3. |
The code below is extracted from Flutter codebase with classes renamed for brevity.
Here class
X
is extendingBase<C>
and also mixing classM
which extendsBase<B>
whereC extends B
.Base<T>
has a getterf
returningT
. In classX
whichextends Base<C>
therefore it is expected thatf
returnsC
. However Fasta in strong mode reports:This means it thinks
f
returnsB
and notC
. Furthermore Fasta also generates throw of a compile error inX.problem
body.@lrhn can you take a look at this code and say if it is expected to compile and run correctly according to the current thinking about mixins in Dart 2.
If it is we will need this to be fixed in Fasta.
I am marking this as P0 because this completely blocks us from running Flutter in strong mode with no workaround available.
/cc @lrhn @leafpetersen @stereotype441 @kmillikin @a-siva
The text was updated successfully, but these errors were encountered: