consider updating `has{*}` annotation getters to consider augmentations

[pq]

Follow-up from https://dart-review.googlesource.com/c/sdk/+/363142 where @bwilkerson notes:

hasImmutable doesn't appear to take into account any augmentations (metadata is cumulative).

We probably need to add all of the hasX getters to the annotated elements and use those anywhere we're currently using the versions on the elements.

Or maybe we need to rethink the whole element model. Sigh.

---

Aside: I imagine this generalizes too to other annotations? For example, @Deprecated, @Immutable, @UseResult... Oiiiii...

Or (maybe hopefully?) I'm overthinking?

[bwilkerson]

Unfortunately, you're not overthinking it. According to the spec, annotations / metadata are cumulative across augmentations, for any kind of declaration that can be augmented.

The hasX getters I was referring to are on the class Element, so this issue should probably be in the sdk issue tracker.

We decided that all of our existing element classes refer to a single declaration in the augmentation chain, and we introduced new AugmentedY classes to represent the result of applying all of the declaration in a single augmentation chain. By doing so we've made it so that some questions that we used to be able to ask of the base declaration's element now need to be asked of the augmented element. I think these has-annotation getters are some of them.

It probably still makes sense to be able to ask each declaration in the chain whether it has a given annotation, so I suspect the "only" thing that needs to be done is to duplicate those getters.

[pq]

@scheglov: I'd be curious to get your thoughts?

[scheglov]

Yes, hasX should be in AugmentedY.

It probably still makes sense to be able to ask each declaration in the chain whether it has a given annotation

What are use cases?

[bwilkerson]

Somewhat speculative, but I can imagine wanting to write fixes where we'd need to know exactly which declarations have (or don't have) the annotation. For example, I could imagine a style in which non-generated annotations are all expected to be on the base declaration so that they can be found in one place, and a fix that would move misplaced annotations.

[scheglov]

But fixes would look at AST, not the element model, because they need to know which portion of code to (re)move?

[bwilkerson]

In a world in which the declaration of an element can be spread across multiple files, we'll need a way to identify which files we need to get an AST for. It would be inefficient to get all of the ASTs in order to figure out which ones we actually need.

The other option I can think of is for ElementAnnotations to record the location (file and offset) of the Annotation they represent. That has the potential disadvantage of increasing the size of the summary files, so I'm not sure it's a good alternative.

[scheglov]

I guess you are talking about a fix for a reported lint like "Put @deprecated at the declaration". In this case the lint will be reported in the specific file - in the augmentation. And the fix producer will be activated in this specific file. I don't understand what additional work we will need to do to identify files to get AST.

[bwilkerson]

In this case the lint will be reported in the specific file - in the augmentation.

Ok, but I think we usually do that by looking at the element model, not by looking at the AST. Maybe that needs to change, but maybe not.

I'm just not convinced that it's a bad idea to be able to ask each of the declarations in the chain which annotations are associated introduced by them. Given that it's a breaking change to remove them and that they might have value, I have to wonder whether we shouldn't just leave them.

[scheglov]

Ok, but I think we usually do that by looking at the element model, not by looking at the AST. Maybe that needs to change, but maybe not.

We need AST to report the lint at the correct location, I think?

I'm just not convinced that it's a bad idea to be able to ask each of the declarations in the chain which annotations are associated introduced by them.

Introduced annotations, i.e. metadata - sure. Semantic meaning, i.e. hasX - no. Only the merged state, i.e. AugmentedY, has semantics.

[bwilkerson]

If the annotations are in metadata, then it's pointless to require every user to have to write the loop to look through all of the annotations to determine whether the one they care about is in the list.

I suspect that our difference of opinion might be a result of thinking of has differently. I think it's necessary for users of the API to understand that things they ask of one element in the chain are related to that one element and not to the augmented element. If I ask an individual element whether it has an annotation it's a lexical question of whether that annotation was introduced by the element's declaration. If I ask the augmented element whether it has an annotation it's a semantic question of whether that annotation was associated with the element in some way. So I see no problem being able to ask the non-augmented elements whether they have a given annotation.

[scheglov]

I still don't see how hasX can be useful outside AugmentedY, but I accept your decision.

[bwilkerson]

FWIW, this is one of the issues that got me thinking that maybe we have the wrong element model for annotations.