Encapsulate field assist doesn't move override annotations

[FMorschel]

trafficstars

Repro:

abstract class C {
  const C({required this.x});

  final int x;
}

class D extends C {
  D({required this.x}) : super(x: x);

  @override
  int x;
}

Now use the "encapsulate field" assist on D.x and see the output:

abstract class C {
  const C({required this.x});

  final int x;
}

class D extends C {
  D({required int x}) : _x = x, super(x: x);

  @override
  int _x;      //   <-- override_on_non_overriding_member

  int get x => _x;    //  <-- annotate_overrides

  set x(int value) {
    _x = value;
  }
}

The assist should move the override annotations to where they are actually needed.

[dart-github-bot]

Summary: The "encapsulate field" assist fails to move @override annotations when encapsulating a field that overrides a superclass field. This results in incorrect annotations on the generated getter and setter, leading to potential errors.

[scheglov]

@fshcheglov I think this is a good issue for you to solve.

[fshcheglov]

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

[bwilkerson]

Question: Which annotations should be moved by this action?

Moving the override annotation seems obvious because the field is being made private and it was the public name that was previously overriding something. Although I do have to ask about the case where a supertype already has a declaration of the private name. It might be that we want to leave the override annotation in that case.

But what about deprecated? If it's the public name that was deprecated, maybe the newly created getter and setter should also be deprecated? What about visibleForTesting?

Should all of the annotations be moved / duplicated? Or are there some that should and others that shouldn't? If the latter, is there a better way to know which ones than by having a hard-coded list?

[FMorschel]

My humble opinion is that all should be moved. If there is one certain case where the user wants to remove them, it is easy enough and the new getter/setter would be working as the variable was.

[scheglov]

Ah, this is a good question about all other annotations.

I also think moving all annotations to the getter / setter is right. It is impossible to know the intention of the user about something like @deprecated or @visibleForTesting. Fortunately we generate getter and setter immediately after the encapsulated field, so it would be easy for the user to go down a couple lines to clean up the annotations if necessary.

In the most cases @override would be the only one annotation, so I think it is worth special casing @override and keep it only on the getter or setter as necessary.

[bwilkerson]

I have no objection to being slightly more intelligent about override than we are about other annotations. I expect that a large percentage of the time it will be the only annotation, so improving the UX for it would be more beneficial than doing something similar for other annotations.