Apply dart fixes to abstract classes, and subsequent implementing classes

[Piinks]

I have noticed in writing a few flutter fixes that you cannot write a fix for an abstract class. Instead, you must write the fix for every implementing class. Not sure of it is possible, but it might be nice to just write a fix for the abstract class and have it apply to all the sub classes.

One example is https://github.com/flutter/flutter/pull/81858/files
Writing a fix for GestureDetector.kind meant writing fixes for 17 subclasses.

[srawlins]

Can you explain what you mean by a "fix"? Do you mean a quick fix, in the IDE, or a dart fix / flutter fix fix?

[bwilkerson]

I believe that this is referring to a data-driven fix.

[Piinks]

Yes, @bwilkerson is correct! :)

[bwilkerson]

The issue is that these are changes to constructors, and constructors don't override constructors. If it had been a change to an instance method in the abstract class then I believe it would have automatically applied to all overrides of the method.

I haven't thought deeply about it, but my first take is that we could support an 'apply to subclasses' flag for changes to constructors that would cause constructors in subclasses to be included similar to the way we include overrides. We'd probably want to restrict this to classes that directly or indirectly extend the class (as opposed to those that implement the class).

[pdblasi-google]

I ran into a case where this would be helpful as well. Specifically while making fixes for flutter #122323. The base class TestWidgetsFlutterBinding had a timeout parameter deprecated in the runTest method.

As another point in favor for allowing fixes to apply to a class and its implementations is in the case of a class that's expected to be implemented outside of the library with the base class. In this case, the integration_test package implements a TestWidgetsFlutterBinding. To implement that class, it needs to have an override of the runTest method, and to have that override be valid, it needs to include the deprecated timeout parameter.

If the fix created for TestWidgetsFlutterBinding could apply to the IntegrationTestWidgetsFlutterBinding class, then we could safely remove that parameter without having to set up a separate dart fix in two separate projects for this deprecation.

[keertip]

@pdblasi-google , what type of parameter is timeout? Optional named?

With regards to being able to making use of the data file across packages, as mentioned above, we are not looking to add support for this use case. For now creating a data file in the package, copy paste transforms and running dart fix again is the way to go.

[pdblasi-google]

@keertip timeout was an optional named parameter.

I had intended on copying and pasting as you suggested and as we have to do internal to a single package as well for this specific case. The intent on providing the example was to reinforce Piinks` use case.

If a fix written for a base class could apply to it's implementations, then when IntegrationTestWidgetsFlutterBinding was updated to remove the timeout parameter, it wouldn't have to create a separate fix because the one created for TestWidgetsFlutterBinding would be able to be applied to instances of IntegrationTestWidgetsFlutterBinding as well.

This same benefit would apply to the classes AutomatedTestWidgetsFlutterBinding and LiveTestWidgetsFlutterBinding that are in the same flutter_test library as the TestWidgetsFlutterBinding class, but I chose the IntegrationTestWidgetsFlutterBinding example as having to copy the fix for implementing classes had the highest cost in that case, highlighting why this would be a useful feature to have.

[bwilkerson]

There are some subtleties here that make this complicated.

It's certainly the case that if we know about a change to the parameters of a method m in the class C that we can know to update the arguments to invocations of C.m and to update any overrides of m in subclasses of C to match the new signature. But if there's a class D that extends C and overrides m, and there's an invocation of m where the static type of the receiver is D, then there won't be a diagnostic produced at the invocation site until after the declaration of D.m has been updated. (These fixes aren't full featured refactorings that can proactively find all of the places that need to be cleaned up, they require a diagnostic to point out where cleanup needs to happen.)

Given the TestWidgetsFlutterBinding example, it seems like it's reasonable to use the transform associated with C.m to update invocations of D.m (on the second pass, after there are diagnostics telling us where a fix is needed). What I haven't been able to convince myself of is that it's always safe to assume that the inherited transform is safe to apply. For example, if a required parameter is added, and if the transform for C.m specifies a default value, is it safe to assume that that default value is also appropriate for D.m? Because we're talking about two different packages the authors might be different people, so the author of the inherited transform might not even know about the package containing D.

It's details like that that we're concerned about and that make us think that we might not want to allow transforms for superclasses to be used on subclasses.

[Piinks]

I think that makes total sense.

Alternatively, I think it would go a long way if C.m was deprecated (and maybe there is a fix for C.m), that at least a warning would be surfaced to the author of subclass D.m so that they can decide what to do and not be surprised by a hard break later when C.m is removed.

Something like 'hey there, you are overriding a deprecated method.' could make a huge difference.

[bwilkerson]

That idea came up recently in another context as well. I think that makes sense most of the time, but there are a few times when it doesn't and I don't know how to avoid the false positives.