How will annotations on a class with a primary constructor apply?

[srawlins]

(Note the sibiling issue, #62254, for instance-variable-declaring parameters.)

With primary constructors, you can write a class like so:

@visibleForTesting
class C(final int x);

The constructor and class declarations are rather intertwined, and there is no way to annotate the constructor individually (you cannot write class C @visibleForTesting (final int x);). So given the above declaration, does @visibleForTesting apply to the class? The constructor? Both?

One option is to say: that annotation only applies to the class; if you want any annotation on the constructor, you cannot write this class with a primary constructor. That seems pretty harsh, and may block a significant set of potential primary constructor use cases.

A second option is to say: each tool (e.g. analyzer, freezed code generator, JsonSerializable code generator) must choose a sensible option, based on the annotation. (And the choice should be documented with the annotation.) I'd like to explore this option. Regarding the analyzer's static analysis, It only works for annotations that the analyzer understands and acts on, which includes annotations from:

• dart:core (@Deprecated(),@override, @Since()),
• package:meta (@immutable, @mustCallSuper, @visibleForTesting, etc.),
• a few other spots, like I think dart:ffi, dart:js_interop (@Js), package:test_reflective_loader,
• @Preview from somewhere in Flutter,
• possibly specific cases of @pragma.
• Maybe more. The analyzer does not act on external things like a @freezed annotation (except maybe to report when it is applied wrongly, if it uses @Target; we'll get to that below).

(I'll note that "each tool must choose a sensible option" is not really drifting from how things work today. When you annotate a class with @visibleForTesting, we choose whether or not that applies to members, like constructors. When you annotate a method with @protected, we choose whether or not that applies to overrides.)

So, what might this look like? Let's rundown package:meta:

Annotation	is considered to apply to
@awaitNotRequired	n/a (Future-returning function)
@doNotStore	The constructor
@doNotSubmit	Both
@experimental	Both
@factory	n/a (static method)
@immutable	The class
@internal	Both
@isTest	n/a (non-constructor function)
@isTestGroup	n/a (non-constructor function)
@literal	The constructor
@mustBeConst	n/a (parameter)
@mustBeOverridden	n/a (instance member)
@mustCallSuper	n/a (instance member)
@nonVirtual	n/a (instance member)
@optionalTypeArgs	The class
@protected	n/a (instance member)
@RecordUse	Maybe the constructor
@redeclare	n/a (instance member)
@reopen	The class
@Target	The class
@sealed	The class
@useResult	The constructor, or maybe n/a
@visibleForOverriding	n/a (instance member)
@visibleForTesting	Both

In all of these cases, "The class" is shorthand for "The class, enum, extension type (or extension constructor?)" where applicable.

How about dart:core:

Annotation	is considered to apply to
@Deprecated	Both
@override	n/a (instance member)
@Since	Both
@pragma	Does the analyzer act on any pragma annotations?

If this seems a sensible route, we can move forward and define what annotations from dart:ffi etc. apply to.

TargetKind

TargetKind offers a neat way for the analyzer to report on whether an arbitrary annotation has been applied to a sensible element. For example, the @immutable annotation is itself annotated with @Target({.classType, .extensionType, .mixinType}) which indicates that it is only appropriate to use this annotation on a class, extension type, or mixin declaration. If it is found on any other element, like a variable, the analyzer will report a warning.

That set of TargetKinds for @immutable serves as a good example: if a class with a primary constructor is annotated with @immutable, is that "ok?" It seems obviously "yes" in this case, we can take a wild guess at what the user is intending. And we don't have to complain that it looks like the annotation might apply to the constructor. Indeed, classes with a bunch of final fields declared in a primary constructor is a prime target for an @immutable annotation. So what about the other TargetKinds?

• classType, enumType, extensionType, mixinType, type are appropriate, as the annotation can be considered to apply to the type declaration.
• constructor is appropriate, as the annotation can be considered to apply to the constructor declaration.
• directive, enumValue, extension, field, function, getter, library, method, optionalParameter, overridableMember, parameter, setter, topLevelVariable, typedefType, typeParameter, values are not appropriate; continue to report.

CC @bwilkerson @szakarias @eernstg @pq

[bwilkerson]

We don't want to add a lot of special cased logic to try to determine where annotations apply. I think it would be better to say that we're going to use the Target kinds when they're available and report the annotation as being on both when there's no Target on the annotation's class. Then we should go through and add Targets to all of the dart-team owned annotations.

I'd use the same logic for annotations on declaring parameters.

[srawlins]

I think it would be better to say that we're going to use the Target kinds when they're available and report the annotation as being on both when there's no Target on the annotation's class.

This is a good path forward. So in terms of the tables above, dart:core annotations cannot have TargetKinds, so they just apply to "both," which is my recommendation above: @deprecated deprecates both (except @Deprecated.extend() etc), @override applies to niether, @Since applies to both.

In terms of the package:meta:

• _DoNotStore should only apply to the constructor, and it indeed has TargetKind.constructor. (Okay it also has TargetKind.classType which I do not understand. To investigate.)
• _DoNotSubmit has TargetKind.constructor, which I guess is good enough 🤷 . We can say it only applies to the constructor.
• Experimental does not use TargetKinds. Maybe it can?
• Immutable has TargetKind.classType, TargetKind.extensionType, and TargetKind.mixinType, so that works.
• _Internal does not use TargetKinds. Maybe it can?
• _Literal does not use TargetKinds. Maybe it can?
• _OptionalTypeArgs has TargetKind.classType and other types. That works.
• RecordUse does not use TargetKinds. Maybe it can?
• _Reopen has TargetKind.classType and TargetKind.mixinType. That works.
• _Sealed has TargetKind.classType. That works.
• Target has TargetKind.classType. That works.
• UseResult has TargetKind.constructor. That works.
• VisibleForTesting does not use TargetKinds. Maybe it can?

Also, dart:ffi and dart:js_interop annotations cannot have TargetKinds.

[eernstg]

One option is to say: that annotation only applies to the class

The language as such doesn't determine exactly which syntactic construct is the target of any given occurrence of <metadata>, but my impression (from discussions at the language team meeting and elsewhere) is that the language team assumed this choice: Metadata that occurs as the frontmost part of a class declaration applies to the class, and only the class. Other kinds of 'membered' declarations like mixins, mixin classes, enums, extension types, and extensions are treated the same.

Moreover, it has been mentioned many times in those discussions that the body part of a primary constructor can be used to attach metadata to the constructor:

class C(final int x) {
  @visibleForTesting
  this;
}

If the primary constructor doesn't need to have a body or an initializer list then this; is all we need.

Augmentations can do similar things, as mentioned in this comment.

It would of course also be possible to say that metadata at the front of a class declaration applies to the class as a whole, and it applies to the primary constructor, if there is one.

However, I suspect that this would be an inconvenience more often than a convenience. It might also be confusing for readers of the code. For example, @visibleForTesting class C(final int x) {...} would make the constructor as well as the class visible for testing, which may or may not be what we want.

In general, I suspect that it is a good idea to make the targeting of metadata more precise rather than letting each piece of metadata target a larger set of program elements, and then using Target and similar mechanisms to filter out the ones that we don't want—I'm simply not convinced that the filters will work as needed all the time. For this issue, this means that I'd recommend letting up-front metadata on a class target the class, only, and use a more precise instrument to apply metadata to the primary constructor. This will work even in the case where there is no Target, or it's too broad for the given situation.

[srawlins]

WDYT of this propsal, @bwilkerson ? An annotation on a class only applies to the class; if you want to annotate the primary constructor, you must the body part. Very easy for me to flip the one-or-two CLs I've landed for option two.

[bwilkerson]

If I were looking at this one situation in isolation, I'd say that it's probably fine to not allow the annotations for the constructor to be on the class declaration. And if users didn't like that they could let us know and we could start allowing them on the class declaration.

But if we're looking at this more generally, then I have to wonder why we'd make a different rule here than we do for declaring parameters. That seems inconsistent to me, despite the fact that there isn't an alternative in the parameter case. I have to wonder whether the inconsistency will frustrate our users. Something to think about.

[eernstg]

why we'd make a different rule here than we do for declaring parameters

So you're suggesting that we adjust the grammar such that it is possible to attach metadata to the primary constructor?

@m1 class @m2 const C.n(@m3 final int i);

With this, the class would have metadata m1, the primary constructor would have metadata m2, and the declaring parameter would have metadata m3.

The language team thought that it would be sufficient to use the body part of the primary constructor as the anchor for metadata on the primary constructor, but putting it in the header itself is not impossible, and it would allow for some declarations to be one-liners which would otherwise need several lines.

I don't think it's going to work as a general rule that metadata on a bigger term is considered to be metadata on subterms:

@m class const C.n(final int i) {
  void foo() {}
}

If we consider m to be associated with the class and the primary constructor then why wouldn't it also be considered to be on the declaring parameter, and on every member declared in the body, etc?

We do have cases where metadata is indeed considered to be on subterms of the term they're textually associated with, but I'm not aware of any case where it is associated with the greater term as well as with the subterms:

const m = 1;

void main() {
  @m int i = 1, j = 2;
}

Both i and j have metadata m, but there is no enclosing entity that corresponds to the entire variable declaration statement int i = 1, j = 2;. Hence, if the metadata is supposed to be associated with anything then it must be something like each of the variables declared by this variable declaration statement.

[bwilkerson]

So you're suggesting that we adjust the grammar such that it is possible to attach metadata to the primary constructor?

No, I wasn't suggesting that. What I was suggesting is that the metadata attached to the declaring parameters (likely) applies to either the parameter, the field, or both, so it seems consistent to me to say that the metadata attached to the class could also apply to either the class, the primary constructor, or both.

If we're going to allow metadata attached to one declaration to apply to multiple elements in one place, then it's consistent to allow that in other places.

But my bigger point is that I think we want to be consistent. Consider all of the places where metadata is allowed and have a single consistent rule that covers all of them. Don't think about metadata attached to a class as being a separable topic to the metadata associated with primary constructors, or fields, or anything else.

... putting it in the header itself is not impossible ...

I find that to be unattractive, but that's just one opinion.

I don't think it's going to work as a general rule that metadata on a bigger term is considered to be metadata on subterms ...

Interesting. I don't think of metadata as being associated with grammar terms, I think of them as being associated with elements. In the case of a declaring parameter declaration, we are considering allowing the metadata to be associated with both the field and the parameter (and the local variable, though I don't think we define any annotations for local variables) because this single declaration defines both elements. In the same way, a class declaration that includes a primary constructor declares both the class and the constructor.

(And yes, this implies that I don't think of the primary constructor as a separate declaration, it's just part of the class declaration. That's probably why I find the idea of allowing metadata on the primary constructor to be unattractive.)

I'm not aware of any case where it is associated with the greater term as well as with the subterms ...

It isn't quite the same thing, but the deprecated annotation works that way. If you attach it to a class then both the class and all of its members are considered to be deprecated. I don't consider that to be a strong argument, just pointing out some of the subtleties of the topic. :-)