[PATCH] D89743: Support Attr in DynTypedNode and ASTMatchers.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D89743#2363201 <https://reviews.llvm.org/D89743#2363201>, @aaron.ballman wrote:

> In D89743#2360258 <https://reviews.llvm.org/D89743#2360258>, @sammccall wrote:
>
>> (while this is useful to you, let's keep discussing, but I'm also happy to stop and land this with your preferred API/semantics - just LMK if changes are needed)
>
> Let's hold off on landing for just a little bit. I don't think changes are needed yet, but the discussion may change my opinion and we might as well avoid churn. However, if you need to land part of this while we discuss other parts, LMK.

Sorry I haven't had more to say on this. I would like to land at least the attr-in-DynTypedNode part, as this unblocks some stuff unrelated to matchers in clangd (someone just filed a dupe bug, which reminded me...)

Happy to cut out all of the matcher part, or land just attr() without any attr matchers, or all of it, or make another round of changes...

>> In D89743#2360032 <https://reviews.llvm.org/D89743#2360032>, @aaron.ballman wrote:
>>
>>> 
>
> My mental model is that there is no declaration of an attribute (in the usual sense, because users cannot specify their own attributes without changing the compiler), and so there's not a referential model like there are with a DeclRefExpr that refers back to a specific declaration. Instead, to me, an attribute is a bit more like a builtin type --  you may have multiple ways to spell the type (`char` vs `signed char` or `unsigned char`, `long int` vs `long`, etc), but it's the same type under the hood.

Ah, this also makes sense! Again, the wrinkle is IIUC `attr::Kind` is like a builtin type, while `Attr` is more like VectorType - it specifies a builtin type, but also some other stuff.

> However, that brings up an interesting observation: you can't do `hasType(hasName("long"))` and instead have to do `hasType(asString("long"))`. I don't recall the background about why there is `asString()` for matching string types vs `hasName()` -- you wouldn't happen to remember that context, would you?

That was before my time I'm afraid.
It seems the implementation checks whether QualType::getAsString() exactly matches the argument. `asString("long")` matches a type specified as `long` or `long int`. But `asString("long int")` doesn't match anything! (Sugar types muddle things a bit but don't fundamentally change this).
My guess is this implementation was just a question of practicality: parsing the matcher argument as a type isn't feasible, but having the node producing a single string to compare to is easy. And it generalizes to complicated template types.

It's reasonable to think of these as doing different things `hasName` is querying a property, while `asString` is doing a specialized comparison of the whole thing.
I'm not *sure* that's why the different names where chosen though. And the fact that `hasName` allows qualifiers definitely feels a bit bolted on here.

> For instance, is the correct pattern to allow `attr(asString("aligned"))` to map back to an `AlignedAttr` regardless of how the attribute was spelled in source, similar to how `asString("long")` matches `long int`?

If we're following precedents (not sure we have to):

- per your mental model, `asString` would pick one fixed name for each attribute kind - which may not be the one used!
- ... but also `asString` should roughly be "node as string" which logically includes arguments and should use the right name (since the node records it)
- `hasName` should match against the `name` attribute of Attr, possibly with optional qualifiers
- ... but there's no precedent for rejecting synonymous names per se, and this may be confusing behavior

So I don't think either of these are a *great* fit, assuming we think "attribute named XYZ or any synonym" is the most important operation (which I agree with).
I can't think of a pithy name, but I also think it's OK to be a bit verbose and explicit here - attributes aren't the most commonly used bit of the AST.

> Maybe `attr(equivalentAttrName("..."))` or so, possibly constrasting to `attr(exactAttrName("..."))`?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89743/new/

https://reviews.llvm.org/D89743