`TargetKind.parameter` doesn't consider extension type "parameters"

[matanlurey]

This was an interesting one!

import 'package:meta/meta.dart';

// The annotation 'mustBeConst' can only be used on extension types or parameters.dart
extension type const FancyInt(@mustBeConst int _value) {}

If you move it to the explicit constructor syntax, it WAI:

import 'package:meta/meta.dart';

extension type const FancyInt._(int _value) {
  const FancyInt(@mustBeConst this._value);
}

I am guessing TargetKind.parameter needs to "see" extension type default constructors parameters as parameters 😓

[srawlins]

In our AST the representation of an extension type is considered to declare field stuff: https://pub.dev/documentation/analyzer/latest/dart_ast_ast/RepresentationDeclaration-class.html

fieldElement, fieldMetadata, fieldName, fieldType. Given the text

It declares at the same time the representation field, and the primary constructor.

I think it makes sense to allow annotations with either TargetKind.parameter or TargetKind.field on a primary constructor field/parameter.

[bwilkerson]

I agree. The representation declaration defines two kinds of elements, so it's reasonable that annotations for either kind of element are also allowed on a representation declaration.

[srawlins]

Actually, it would also strongly depend on whether the resulting field (e.g., accessed via ExtensionType.members) has the annotation, and whether the resulting constructor (e.g. accessed via ExtensionType.members) has the annotation.

[bwilkerson]

I would state that differently. I think the logical conclusion is that any annotations on the representation declaration must be added to both the field and the constructor parameter. If they aren't, then I'd consider that a bug.

That said, I don't see anything in the spec to confirm that interpretation.

@eernstg Should the spec say something about what metadata associated with the representation declaration applies to?

[eernstg]

Should the spec say something about what metadata associated with the representation declaration applies to?

Yes, it should say something. However, the situation isn't totally unambiguous.

IIRC, an instance variable declaration gives rise to a getter declaration which isSynthetic, and possibly a corresponding setter declaration. They may or may not have metadata which is syntactically associated with the variable declaration ('dart:mirrors' does not report any metadata on the synthetic members, and I think the analyzer does the same).

So if a parameter of a primary constructor (currently known as a subterm of a <representationDeclaration>) is going to be considered as an "actual" (non-synthetic) declaration that implicitly induces a formal parameter declaration (of the implicitly induced constructor) as well as an implicitly induced instance variable declaration, then we might want to treat the synthetic elements the same, for consistency, or we might treat them differently, for some other reason.

It is probably (very!) useful if we can ensure that code that looks up the metadata of the representation variable of an extension type will work in the same way as code that looks up the metadata of any other instance variable declaration, and similarly for code that looks up the metadata of the corresponding formal parameter declaration. That would be an argument in favor of letting these synthetic entities carry the metadata, rather than expecting the developer who needs this information to look up an Element for the primary constructor parameter (that is, the hybrid parameter-and-variable thingy).

Perhaps those synthetic entities (both the ones that are induced by the parameter-and-variable and the ones that are induced by an instance variable declaration) should have a metadata getter that performs this lookup, rather than returning null?

[bwilkerson]

I hadn't thought of it in terms of synthetic elements. I'm not sure whether the instance field element and the parameter element that are currently being created are marked in the element model as being synthetic. Probably not.

I guess I was thinking of the representation declaration more as syntactic sugar for the two declarations (the field and the constructor) rather than as a separate declaration all its own. Even though the spec doesn't use any "is equivalent to" kind of language.

Probably because (I think) we're not currently representing the representation declaration as an explicit element. Doing so would be kind of odd because it can't be referenced (only the implied field can be referenced), but maybe its worth considering.

But I do agree that it would probably make sense for synthetic elements that are derived from a declaration that can have metadata (like the field, parameter, and induced getters and setters) should probably be reporting the metadata as their own (in the element model). We've probably written a fair bit of code to get around that by checking to see if a getter is synthetic and accessing the metadata through the non-synthetic field when it is. (Although that raises the question of what metadata if any should be returned by a synthetic field.)

I don't have a firm opinion about any of this yet.

[lrhn]

Should the spec say something about what metadata associated with the representation declaration applies to?

My answer here is "no".

The language specification specifies syntactic positions where metadata can be placed, and that it must be a valid constant, but that is it. The specification assigns no meaning to any metadata annotation, leaving that entirely to the tool that the annotation is intended for. Even assuming that metadata applies to any semantic entity is outside the scope of the language specification.

Metadata is a way to put tokens into a program without the language having to assign any meaning to them.

What dart:mirrors does when reflecting on a synthetic semantic entity is a design question for that library, the language doesn't need to know or care.

Same for the analyzer's introspection API. If it's not inspecting syntax, and it isn't, then there is a choice about where to attach metadata. I'd put it on all the derived semantic entities, because then it's definitely on the one where it's needed. If the analyzer understands @TargetKind annotations, it can choose to filter on that too. It's a choice

But the language has no opinion on metadata, other than allowing it to exist.

[srawlins]

Metadata is a way to put tokens into a program without the language having to assign any meaning to them.

Is this true for macro-applying metadata? If I ask, "does a macro application on a representation declaration apply to the associated field?", is the answer "That is unspecified. Each compiler may choose freely." ?

[lrhn]

Macros is, essentially, a tooling feature. The language doesn't need to know that macros exist, it's just that compilers, which are tools, interrupt compilation if they detect certain metadata annotations, then run an external tool on the program source, producing a new program source (by injecting a single augmentation file and including it in the original, per file that applies macros). The compiler then carries on and compiles the new program using normal Dart semantics (with augmentations, which will be a normal Dart feature independent of macros).

The external program happens to be written in Dart, which is again completely normal Dart.

The language doesn't need to know about macros (of I have a say), it's an SDK feature.

We should still implement it properly and consistently across our entire tool chain. But it's a tool/feature design choice, not a something the language specification should need to care about defining, because of doesn't know what the annotation means.

Each annotation gets to decide how it wants to be associated, because the only one who should care is the tool it's intended for.

[eernstg]

@lrhn wrote:

But the language has no opinion on metadata, other than allowing it to exist.

I think we have to strike a meaningful balance here. I do agree, though, that the specification documents have largely been silent on this topic so far, or at least very vague.

It makes sense to agree on the binding of each occurrence of <metadata> in a program and some other syntactic construct. That part is probably not very hard or controversial: With <myNonterminal> ::= <metadata> ...;, the given <metadata> is associated with the syntax that is derived from <myNonterminal>, and with ... (<metadata> <someTerm>)* ..., the <metadata> is associated with the syntax that is derived from <someTerm>. Adding in a couple of oddballs, that covers the constructs involving <metadata> which are currently used in the grammar.

However, the actual question which will be asked is what metadata is associated with a given semantic entity (say, a class or a method).

It isn't obvious that there is a 1:1 mapping from specified grammar elements (non-terminals) and implemented program representations, so maybe there is no AST node that corresponds to <myNonterminal>. Nevertheless, I think this source of ambiguity hasn't been a real problem (I haven't heard about it, anyway), and we're probably able to talk about things like "the metadata of a given class".

When it comes to implicitly induced getters and setters it seems rather impractical to insist that the analyzer and the CFE can disagree on whether or not they have the <metadata> which is associated with their corresponding variable declaration ... especially now that macros will allow metadata to have non-trivial semantic effects on programs.

(By the way, I definitely do not agree that macros are a tool-only issue, nor that it is important (or even desirable) that every tool can have its own rules about macros and their semantics. For example, the macro modifier on a class is manifestly a part of the language.)

In summary, I agree that metadata have a very small amount of semantics in the language (they identify a source code construct and hence indirectly one or more semantic entities, and associate an object with that entity / those entities). However, I do think it's useful to have a shared understanding of this small amount of semantics across all tools.

[lrhn]

I agree that whatever our tools do with metadata, they should be consistent.

For synthetic declarations in the semantic model, my personal preference would be to not move metadata onto the declaration, but to link the synthetic declaration to the source declaration, and require anyone who cares about metadata to have to follow that link. That avoids introducing any interpretation of the metadata. Then models can choose to have a transitiveMetadata helper on synthetic entities, but it'll be clear where it looks for the annotations.

Whether an annotation "applies to" a declaration has always been semantic anyway. That's why some annotations are "inherited" by subclasses. That just means that the actual annotation on one declaration has a semantic effect, for some tool, in a number of places that include places in subclasses. The annotation is never moved.

The same way an annotation on an extension type representation variable declaration can have an effect on the synthetic getter, and/or on the synthetic constructor parameter, or maybe on an argument to that parameter, depending on what the tool decides. The annotation is still just in one place.

That said, of tools have so far copied annotations of the source declarations onto synthetic declarations, and it's documented behavior, then it should probably happen here too. And then they should be on all corresponding synetic declarations. (Which should also be the case for the synthetic getter and setter introduced by a mutable variable declaration today.)

(I do consider macros entirely a tooling feature, that shouldn't need any language changes, other than the generally available augmentation feature that works without macros too. The language doesn't need to know that macros exist. That's what makes interpreting the result of macros easy, it's just plain Dart. The hard parts are interpreting the source of an incomplete, not valid Dart, program before and during macro execution. Afterwards, it's just a transformed and hopefully complete program of normal Dart.)

[matanlurey]

There is a lot of text above but if I were to send a patch to consider primary constructor parameters and parameters (with accompanying tests) would it get an LGTM?

[bwilkerson]

If you're asking whether I think it makes sense to allow annotations that are marked using TargetKind.parameter to occur on the constructor inducing representation declaration, the answer is 'yes'.

If you're asking whether there's consensus about whether to "copy" metadata on the representation declaration to the induced constructor, then the answer is "no". I am persuaded by

That said, of tools have so far copied annotations of the source declarations onto synthetic declarations, and it's documented behavior, then it should probably happen here too.

I think we want to see how we handle this kind of copying in other cases and be consistent.

Not copying, which I suspect is what we're doing today, has the benefit that we can treat mustBeConst as being valid and meaning that only a const value should be allowed as a parameter to the constructor without having to also copy it onto the field, where I don't think it's allowed and would hence be an error. (Even if that particular annotation is valid on a field there can hypothetically be an annotation that's valid for one and not the other.)

[matanlurey]

I don't really have an opinion on that, I am trying to strictly answer the question of:

extension type const Foo(@mustBeConst int value) {}

void main() {
  var notConst = 5;

  // LINT or nah?
  Foo(notConst);
}

I think we're debating whether to lint:

class Foo {
  @mustBeConst
  final int value;

 const Foo(this.value);
}

Which, while interesting, is a different case, right? In the first there is only a primary constructor.

[bwilkerson]

I'll try to be more explicit. I think there are now three question.

1. Is it ok to flag the invocation of Foo.new in

extension type const Foo(@mustBeConst int value) {}

void main() {
  var notConst = 5;

  // LINT or nah?
  Foo(notConst);
}

to which I believe the answer is 'yes'.

2. Is it ok for Element.metadata, when invoked on the field induced by a representation declaration to return metadata declared on the representation declaration, I think the answer is undecided (and needs some research to be answered).

3. Is it ok to use mustBeConst on a final instance field, I'd say 'no' because getters don't have parameters and there's no induced setter on a final field. If the field isn't final, then I could see an argument for saying that the annotation applies to the parameter of the induced setter, but that's a new and different topic than I think was being discussed above.

[matanlurey]

Gotcha. Should we open up other issues? My question was originally narrowly scoped to (1).

[bwilkerson]

Should we open up other issues?

I don't have a strong preference, but I'd probably wait until we have some use cases to help motivate a solution.

[srawlins]

I think the answer to "(1) Is it ok to flag the invocation of Foo.new ..." is also 'yes'.

And I also think we can punt on the answers to the other questions.

I think there is a risk of under-reporting and over-reporting invalid annotation positions, which gets more expensive to fix as time goes. But we have a heap of 'false-negative' lint rule issues and maybe warning issues, which also have that cost, and I don't think figuring out (2) or (3) is high priority.