-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Analyzer fails Flutter with [error] The class ... cannot implement both 'State<...>' and 'State<StatefulWidget>' because the type arguments are different.
#32421
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 blocks Flutter engine roll. |
We can temporarily disable the error if necessary, but it's my understanding that this is disallowed in Dart 2. |
[error] The class ... cannot implement both 'State<AnimatedPhysicalModel>' and 'State<StatefulWidget>' because the type arguments are different.
[error] The class ... cannot implement both 'State<...>' and 'State<StatefulWidget>' because the type arguments are different.
At first glance this looks like a bug in mixin inference in the analyzer. I believe that the flutter code is already passing the new front end checks, which include the restriction on implementing multiple interfaces. I will work on getting a repro, but if I'm reading the flutter source correctly, the analyzer is incorrectly inferring |
How does flutter use the analyzer? Are they an analysis context consumer or a driver consumer? I cannot reproduce this with command line bleeding edge analyzer.
With a small repro as follows:
I see:
|
Note that dartdevc uses the analysis context path, hence my question about analysis context vs driver. |
Also, I confirmed that both the analyzer CLI and dartdevk will complain if I manually instantiate |
From looking, @danrubel, I had thought that we'd transitioned flutter analyze ontp just using dartanalyzer and the analysis server only, and not a package:analyzer based implementation. We'll likely want to do that to help unblock the roll. |
It looks like we only use the context based analysis when analyzing the code in the flutter repo itself, in non-watch mode. |
@danrubel, is this something you can take a look at? |
It's fine by me if you want to roll back this restriction for a bit to unblock the roll, but we should get that out there pretty soon to be sure that nothing incompatible with the CFE implementation creeps in. |
@leafpetersen wrote
Do you mean this https://dart-review.googlesource.com/c/sdk/+/45142 ? |
No, at least as I understand it. My understanding is that @mraleph already verified that the flutter code base is clean with this restriction (the new front end already implements it). The problem is that flutter analyze seems to be using a deprecated analyzer code path when analyzing the flutter repo, and that code path doesn't support mixin inference which is what makes this restriction work with flutter code. |
As an alternative to rolling back https://dart-review.googlesource.com/c/sdk/+/45142, consider this more targeted workaround, which merely disables the error when using the old task model: https://dart-review.googlesource.com/c/sdk/+/45200. The advantage of the more targeted workaround would be that users of the command-line analyzer and analysis server would continue seeing the error message. |
This is necessary since the old task model doesn't implement mixin type inference properly. Works around bug #32421. Change-Id: I6d7873d658d9d0fd8ea6aa5d2452bf9e92947032 Reviewed-on: https://dart-review.googlesource.com/45200 Reviewed-by: Alexander Aprelev <[email protected]> Commit-Queue: Paul Berry <[email protected]>
I think it would be better to only disable the error in the task model (though that also means disabling it for ddc) rather than to disable it everywhere. |
@stereotype441 I have no context for fixing this. Is this something you can look at before the end of the week, or should we schedule a time to discuss what needs to be done? |
The CL disabling this check in the task model has landed, so I believe this unblocks the roll. @aam can you confirm? @bwilkerson it wasn't clear to me whether this should be fixed in the analysis context path or whether flutter analyze should just be taken off of the analysis context. I definitely feel like the latter is something we really should do at some point soon, but you all have a better handle on the dependencies and timing. |
@leafpetersen wrote
Yes, that's right. |
Yes, we need to move As far as I know, whether it needs to be fixed in the task model depends on whether ddc needs this error to be supported. |
Good point. @vsmenon thoughts? Since dart2js has never supported this, presumably all web code is clean and will remain so. Is analysis of internal code during builds done using the task model or not? I think probably DDC support is not critical, but I'm a little worried if all of the non-DDC internal code doesn't get checked for this at least relatively soon. |
Feel free to close this issue as Flutter is unblocked. |
@leafpetersen FWIW I believe that DDC does not need this error to be reported, since it has never had the restriction that a given generic interface may only be implemented in one way. As for analysis of internal code during builds, I can confirm that this is done using the analysis driver. So even with the workaround we've landed, internal code will still be checked to make sure it's clean. |
Right, but once it switches to kernel, it will have that restriction, and we want to know about that earlier rather than later, but...
... as long is this is true, then we will know about it earlier rather than later, so I'm happy. |
We need to move this specific use case of flutter analyze - analyzing the flutter repo itself once - off the task model. We should do that by moving it to using the command line analyzer - like the rest of the use cases for flutter analyze - or move it to the analysis server. I really don't think we should be using package:analyzer here, and having three different code paths to analyze flutter code is too many. |
@devoncarew who will move analyzing the flutter repo itself off of the task model? |
Also, @devoncarew does this have additional work required for the 2.0-beta2 milestone? |
From talking with Paul, his workaround did land. There's no additional work here for beta2. We will want to move flutter analyze off the task model; I'll track that work in a separate issue. |
Between ee15c8e and a4c69b7 analyzer seems to have implemented analysis rules that flag existing Flutter framework codebase erroneous:
The text was updated successfully, but these errors were encountered: