[macros] 🟡 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/ dart-lang/sdk@d3d3efb

[lrhn]

I'd probably want to allow @override on augmentations too.

In particular, a macro can add a super-interface, and it should be able to add the appropriate override too.

@someMacro
class C {
  int get length => 42;
}
// from macro
augment class C implements Iterable<String> {
  //  ...
  @override
  augment int get length;
}

[pq]

Agreed. Currently, the above code will (wrongly) flag the initial declaration of C so I'm re-opening this to work on a fix. Thanks @lrhn!

[pq]

See also: dart-lang/sdk#55579 -- If hasOverride considered augmentations, this would just work.

[bwilkerson]

I'd probably want to allow @override on augmentations too.

We should definitely allow it.

The question is whether the lint should require it. I'm inclined to say "no" because the language doesn't require it and it doesn't seem unreasonable for users to not want to duplicate annotations. By "duplicate" I mean that, as far as I understand, writing (with the right boilerplate around it)

class C {
  @override
  String toString() {}
}

augment class C {
  @override
  augment String toString() {}
}

means that the method C.toString has two override annotations associated with it.

For override the duplication is harmless, but I could imagine an annotation where the duplication wasn't. Even for something like

@Deprecated('as of 2.12')
@Deprecated('as of 3.1')
void f() {}

where the duplication seems harmless, it raises questions of what the analyzer should show in the diagnostic when f is invoked somewhere. (Right now it shows the argument to the constructor in the message, but should it show multiple messages when there are multiple diagnostics? Don't know because nobody does that so we haven't needed to answer the question, but if those two annotations were on a declaration and an augmentation we might have to think about it.

The point of all that being that we don't know what the community will want to do stylistically, and I don't think we want to make the lint enforce one choice or the other at this point.

Maybe a different lint to enforce duplication would make sense if that becomes a common pattern.