Large-scale dart fix for hashValues and hashList

[werainkhatri]

hashValues and hashList being top-level functions are difficult and lengthy to write data-driven fixes for. It would be nice if there was a global dart fix for these functions instead, which can be applied to them regardless of the class/import.

PR that migrates these functions: flutter/flutter#96109
PR that deprecates these functions: flutter/engine#30674
Original issue: #25217

cc @Piinks @bwilkerson

[asashour]

Should this be a hint?

[bwilkerson]

Sorry I didn't notice this earlier.

hashValues and hashList being top-level functions are difficult and lengthy to write data-driven fixes for.

Could you elaborate a bit on what makes them difficult and lengthy to write?

It would be nice if there was a global dart fix for these functions instead, which can be applied to them regardless of the class/import.

I'm not sure what you're suggesting here.

Should this be a hint?

These functions are marked as deprecated, so users should already be getting a diagnostic pointing them to the replacement. I don't think a second hint would help.

[Piinks]

I think this sounds similar to #46464, maybe?

The migration @werainkhatri did in flutter/flutter#96109 migrated over a hundred classes, which would mean over a hundred dart fixes would need to be written, one for every class. I think @werainkhatri was looking to see if there is a way to consolidate and simplify that.

[asashour]

If I may say something for the discussion, one of the reasons to have a fix, is that it would be used for many use cases, preferably by a wide user base, otherwise, we would need a fix for each deprecated method, which would frequently happen on the long run.

On the other hand, I wonder if there is some specific API to be exposed to a potential project team to handle specific project code modifications.

[bwilkerson]

The migration @werainkhatri did in flutter/flutter#96109 migrated over a hundred classes, which would mean over a hundred dart fixes would need to be written, one for every class.

I must be really confused. Perhaps a VC is in order. Because when I look at the PR it appears to be the case that two top-level functions were replaced by two static methods. The invocation sites might have been in a hundred classes, but you would only need two transforms to make all those changes. So I'm not sure how we'd consolidate that.

If I may say something for the discussion ...

Of course. Your input is always welcome.

... one of the reasons to have a fix, is that it would be used for many use cases, preferably by a wide user base, otherwise, we would need a fix for each deprecated method, which would frequently happen on the long run.

Absolutely.

On the other hand, I wonder if there is some specific API to be exposed to a potential project team to handle specific project code modifications.

That's actually what we're discussing here. We have something called data-driven fixes that allows the Flutter team to describe the fixes appropriate for API changes. We are exploring whether to make this more widely available to other package authors.

[werainkhatri]

so IIUTC, the below transform should be the only fix required (for hashValues at least).

  - title: 'Replace `hashValues` with `Object.hash`'
    date: 2020-11-20
    element:
      uris: [ 'material.dart' ]
      function: 'hashValues'
    changes:
      - kind: 'replacedBy'
        newElement:
          uris: [ 'material.dart' ]
          method: 'hash' # EDIT: was field, my bad
          inClass: 'Object'

but it gives an analysis error.

An element of kind 'function' can't be replaced by an element of kind 'field'.

[bwilkerson]

Yes, modulo the fact that Object is defined in dart:core.

So I believe that there are two enhancements needed here:

1. Allow zero argument functions to be replaceable with getters (and vice versa).
2. Allow replacements between packages and dart: libraries.

Both of those are being requested for other reasons, but the work hasn't been scheduled yet.

[asashour]

• I am probably missing something, but Object.hash is a method, not a field, and the test case passes with it even when using non-core uris.

• For the replacement to a dart: library, please see https://dart-review.googlesource.com/c/sdk/+/241004

• Replacement from dart is prevented by the below, I hesitate to change it before discussing.

sdk/pkg/analysis_server/lib/src/services/correction/fix/data_driven/element_matcher.dart

Lines 85 to 89 in 5ca6eec

	//
	// Check whether the element is in an imported library.
	//
	var libraryUris = element.libraryUris;
	for (var importedUri in importedUris) {

[bwilkerson]

@pq @keertip Who were going to look at this and several related issues.

... Object.hash is a method, not a field ...

Yes. I was probably thinking about Object.hashCode when I got that wrong. Which is good, because we should already support replacing functions with methods and vice versa.

[werainkhatri]

any update on this? i see cross-package replacedBy is allowed now, but

Allow replacements between packages and dart: libraries.

is yet to be supported.

[keertip]

@werainkhatri, are you still looking to write a fix for this type of migrations?

[werainkhatri]

yes!