[augmentations] 🟡 support for `annotate_overrides`

[pq]

Blocked on WIP in analyzer.

Invalid argument(s): InterfaceType(s) can only be created for declarations

  package:analyzer/src/dart/element/type.dart 479:7                        new InterfaceTypeImpl
  package:analyzer/src/dart/element/element.dart 3704:20                   InterfaceElementImpl.instantiate
  package:analyzer/src/dart/element/element.dart 3629:26                   InterfaceElementImpl.thisType
  package:linter/src/extensions.dart 348:42                                InhertanceManager3Extension.overriddenMember

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

/fyi @scheglov

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/362660 probably unblock this.

[lrhn]

What is the intended behavior for annotate_overrides and augmentations?

Should an augment void foo(){} be annotated with @override if there is a superclass member, or should only the base declaration be?
It's probably only the base declaration. Otherwise we might end up with multiple @override annotations on the same declaration, one for each augment.

Or rather, this sounds like a lint where only the final fully-augmented result matters. The @override can be added by any of the declarations of that name, as long as there is one at the end.
(That does depend on what the goal is. If the goal is to make it clear at the base declaration that it intends to override, then the annotation should be there.)

[bwilkerson]

Or rather, this sounds like a lint where only the final fully-augmented result matters.

I agree and I think that's should be a general principle for any tooling checking for the absence of an annotation. Here's my reasoning.

According to the spec (https://github.com/dart-lang/language/blob/main/working/augmentation-libraries/feature-specification.md#metadata-annotations-and-macro-applications), metadata annotations and macro applications are cumulative across the base declaration and the augmentations.

So semantically, having an @override annotation on any one of the declarations in the chain ought to be sufficient. For most annotations, having them repeated would be harmless (but for macro applications it generally won't be harmless). That being the case, I don't think we want to require any annotation to be duplicated, nor do we want to dictate which declaration the annotation is physically associated with. (We might see common style evolve in the community, in which case we might later want to add a lint to enforce that style, but I think it's too early for that at this point.)

I suspect that in this case the intent of the work item is to only report the lint on the base declaration. That would also mean that our existing fix will continue to work without changes.

If the goal is to make it clear at the base declaration that it intends to override, then the annotation should be there.

I believe the goal of the lint is to ensure that (a) users are aware if a method starts to override a method added to a supertype, and (b) that users will be aware if a method that was added in order to override a method from a supertype is no longer needed because the method has been removed.

But I do think that it's an interesting UX question: will it be confusing to users to have annotations on an augmentation and hence not visible when looking at the base declaration? If so, we might want to consider ways for the IDE tooling to mitigate that confusion.

[pq]

Should an augment void foo(){} be annotated with @override if there is a superclass member, or should only the base declaration be?

I think only the base declaration. My sense is that we should not lint on augmented fields or methods. (Just on fields and methods added by augmentation classes.)

I believe the goal of the lint is to ensure that (a) users are aware if a method starts to override a method added to a supertype, and (b) that users will be aware if a method that was added in order to override a method from a supertype is no longer needed because the method has been removed.

That's my understanding too.

[pq]

Fixed w/ d3d3efb