[Dart fix] Allow specifying multiple elements per fix

[guidezpl]

Dart fix currently only allow a single element per fix, leading to unnecessary duplication. For example, for each ThemeData fix for https://github.com/flutter/flutter/issues/91772), we need 3 identical fixes for the unnamed constructor, raw constructor, and copyWith.

[asashour]

https://dart-review.googlesource.com/c/sdk/+/267701

[guidezpl]

fyi @piinks

[Piinks]

Ooooo! What a great idea! Thanks for looking into this @asashour!

[bwilkerson]

I like the idea in principle (I'm always in favor of making less work for others), but it might be good to have some discussion about the design of this feature. I haven't really had time to think about it much, but I do have a couple of preliminary questions:

How often does this come up? Is it worth the extra implementation complexity and maintenance cost?

Are there any restrictions we want/need to have on the elements that can be specified? Does it make sense if they of different kinds (such as class and method)? Are in different libraries?

Not all changes apply to all element kinds, so I'm guessing we want to report a diagnostic if there is a change that doesn't apply to all of the element kinds for all of the elements. (Might be we're not validating this at all yet, but we should document the restriction if we want to be able to enforce a restriction later.)

[guidezpl]

This is fairly common, e.g. https://github.com/flutter/flutter/blob/master/packages/flutter/lib/fix_data.yaml#L1193-L1335 is repeated two times above. By consolidating these, I'd roughly estimate we could cut that file 5k lines down to 3-4k.

I think restricting by library makes sense. In some cases the class is the same, but there's also simple examples where the same member in two different classes/methods needs to be renamed and so restriction by class/method name woudl be a slight hindrance.

[asashour]

A transform has currently a single element, and multiple changes.

Why restrict the uris? This is relevant per element, and I don't think restricting it would add a value (simpler implementation, or a value to the user). It is the responsibility of the transform author to handle it, similar to as what (s)he would do with current element.

When multiple elements are allowed, I guess it should have the same restrictions between changes and element, but there should not be a restriction among elements, they are just a list, and each one is having its restriction with the changes.

- title: 'Remove argument'
  date: 2020-09-09
  element:
    uris: ['$importUri']
    method: 'm'
    inClass: 'C'
  changes:
    - kind: 'removeParameter'
      index: 1

[bwilkerson]

This is fairly common ...

Thanks for the concrete example. It's probably not uncommon for classes with a copyWith to have a constructor with exactly the same parameters. That doesn't tell me now common it really is in Flutter, but given two votes for the support I'm willing to conclude that it's useful enough to our users that we should support it.

... I don't think restricting it would add a value (simpler implementation, or a value to the user).

Let me address the issue of restrictions more generally, not just a possible restriction of URIs.

It wouldn't make the implementation any simpler; if anything it would make the implementation a bit more complex because of the required error checking. And it doesn't impact the end user either way because they won't see the data file.

But I think I could argue that it makes it easier for the tooling to catch errors in the data file and therefore does add value for the package author that's writing the transform.

For example, what's the probability that both a class and a method will have the same name and want to be renamed to the same thing? Probably zero, given that classes typically have names that start with an uppercase letter and methods have names that start with a lowercase letter. So if someone writes a transform with two elements, one a class and the other a method, that's most likely an error. But if we say "anything goes, and we'll just do the best we can with the provided data", then that means that we can't point out to the author that the elements don't make sense together.

It would kind of be like saying that the Dart compiler will just ignore any code that isn't valid and try to run as much of the valid code as possible, and won't tell you when there's invalid code that was skipped. Trying to develop with such a compiler would be a nightmare.

And I think this case is a bit analogous. Debugging a broken transform is really hard. If there's no feedback from analyzing the data file then there's no information that tells you why it doesn't do what you think it should. Given that, I think we'd be doing our authors a disservice to not restrict the feature as much as possible so that we can report as many error as possible via static analysis.

If the Flutter team doesn't want the safety net, then I can probably be persuaded to let them operate without one, but I'd much rather start with known use cases and only allow those specific cases, expanding the capabilities of the feature only as much as needed.

[asashour]

On having a quick look, it seems that Flutter has:

• 255 transforms
• element kinds are:
  • 94 constructors
  • 92 methods
  • 38 fields
  • 28 getters
  • 2 classes
  • 1 function
• 101 pairs have identical changes (if my local equals are correct).

The only different types with identical changes are method and constructor (in 8 pairs), but other types could be considered compatible, e.g. field/setter, method/function, class/enum, etc.

As said, enforcing restrictions would add value, and I would image that between changes and element is even more important. Currently renameParameter is allowed for class for example.

How about restricting the element kinds, so that they are all the same (or compatible, like method/constructor)?

Also, overriding == and hashCode of the changes could ease having a test case in Flutter, to hint about duplicated transforms (when the Flutter team considers such test).

[bwilkerson]

Currently renameParameter is allowed for class for example.

Yeah, that doesn't have any meaning and should be caught.

How about restricting the element kinds, so that they are all the same (or compatible, like method/constructor)?

If we flag cases where at least one of the changes isn't appropriate for at least one of the elements, then I think we will have effectively implemented the same logic, but I think it will be less likely to need to be updated as new language features are introduced. But maybe I'm missing a case.

Also, overriding == and hashCode of the changes could ease having a test case in Flutter, to hint about duplicated transforms (when the Flutter team considers such test).

I'd be fine with implementing those methods when we need them. They're not there today because we don't need them and we generally try to avoid writing and maintaining code we aren't using because it tends to bit rot.