Data driven fix support to convert annotation to replace-or-add return type

[srawlins]

We deprecated @alwaysThrows, as an annotation on a function, in favor of using a Never return type. Can we get data driven fix support for this? #49583

I can think of 3 ways the fix would be applied:

1. a function with an explicit non-Never return type. Remove @alwaysThrows and replace the written return type with Never.

   @alwaysThrows void foo() => throw 'x';
   
   // ====>
   
   Never foo() => throw 'x';

2. a function with an implicit return type. Remove @alwaysThrows and add an explicit return type, Never.

   @alwaysThrows foo() => throw 'x';
   
   // ====>
   
   Never foo() => throw 'x';

3. a function with an explicit return type of Never. Remove @alwaysThrows.

   @alwaysThrows Never foo() => throw 'x';
   
   // ====>
   
   Never foo() => throw 'x';

This annotation can appear on top-level functions and methods. I don't believe any special accommodation needs to be made in the fix.

[bwilkerson]

(It can presumably also appear on local functions, though I can't think of a reason why anyone would do so.)

My initial design thought would be to add two changes, one to removeAnnotation and one to changeReturnType, where the latter would handle implicit return types.

[srawlins]

Ah, I thought an annotation wouldn't be allowed in that position. But I'm wrong.

[asashour]

Is there a way to specify "all elements", which have that annotation, and should this be changed?

The current issue now is that the user specifies which element, and then selects what to do with it. In this case, the deprecation is at the annotation, but removal is of the parent method. In other words, the user shouldn't specify foo method, but rather would like to replace all methods which has the annotation x with y return type.

So we can have something like:

version: 1
transforms:
  - title: 'Replace @alwaysThrows with Never'
    date: 2023-01-27
    element:
      uris: ['my package']
      annotation:
        uris: ['package:meta/meta.dart']
        name: 'alwaysThrows'
    changes:
      - kind: 'removeAnnotation' // remove it, can be only 'remove', since it can be potentially applied to other elements
      - kind: 'changeReturnType' // if the parent is a FunctionDeclaration
        name: 'Never'
        uris: ['return type uri'] // optional

Or having annotatedWith

    element:
      uris: ['package:meta/meta.dart']
      annotatedWith: // or 'hasAnnotation'
          uris: ['package:meta/meta.dart']
          name: 'alwaysThrows'
    changes:
      - kind: 'removeAnnotation'
        uris: ['return type uri'] // optional
        name: 'alwaysThrows'  // optional, in case we have multiple annotations
      - kind: 'changeReturnType' // applies to only methods/functions
        uris: ['return type uri'] // optional
        name: 'Never'

I guess the second option is better.

[bwilkerson]

As far as identifying the element being transformed, I'll note that there's no such thing as an annotation declaration. An annotation is simply a reference to a constant (either a variable or a constructor). In this case we're talking about a top-level variable (marked const, of course), so that's the element that should be referenced in the transform. I would expect the element to be described as

    element:
      uris: ['package:meta/meta.dart']
      variable: 'alwaysThrows'

The changes are intended to describe what happened to the element. In this case the change is that the element is being (or was) removed. We haven't tried to model this kind of change yet because in most cases it's better to not delete the references to (or overrides of) the element without manual intervention because there's too big a chance that the code will need bigger changes and removing the reference could hide the need for the bigger changes.

The change should include whatever additional information might be needed in order to apply edits to the code that references the element. For example, an addParameter includes the information needed to update both overrides of the method and invocation sites. Even though we hadn't thought much about annotations, I believe that all of the existing changes would work perfectly fine with both top-level variables and constructors that are used as an annotation (at least from a design standpoint; the implementation is probably incomplete).

So, what information is necessary when removing a top-level variable? There can't be any overrides, so we only need to think about references. Outside of annotations, we probably don't want to apply any automated changes for the reasons given above. For a reference in an annotation, however, we could easily remove the annotation (though it isn't clear to me that that's a good idea in the case of a constructor because the user might want to capture the arguments passed to the constructor (such as a string) in some other way (such as a comment). But for the case of a top-level variable, it might sometimes make sense.

For this particular annotation, we want to replace the return type on the target of the annotation.

And it's at this point that I realized that the probability that we, or anyone else, will ever have the need to replace the return type on the target of an annotation as a result of removing the top-level variable is effectively zero. If we were talking about a targeted fix (that could be bulk applied, but could also be removed in a future release), then that would be one thing, but if we're talking about a general data-driven fix then we'll have no way of knowing whether it's being used and hence won't ever be able to remove the code even though the probability that it's being executed is almost zero.

So, I've changed my mind. I don't think we want to do this as a data-driven fix. I think we should instead implement a special purpose quick fix that will make these changes.