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

Fri Nov 20 11:56:35 PST 2020

aaron.ballman added a comment.

In D89743#2406017 <https://reviews.llvm.org/D89743#2406017>, @sammccall wrote:

> 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...)

That part LGTM and is fine to land.

> 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...

I think the `attr()` matcher is definitely reasonable as it stands and can be landed as well.

>>> 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.

Yeah, that's a fair point.

>> 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.

That's my guess as well -- also, I wasn't aware of the `long` vs `long int` behavioral difference with `asString()`, that's.. neat.

> 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 agree that this is the most common use-case.

> 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("..."))`?

I think this design is the most flexible and will be the most understandable in the long-run because it makes it clear which name-matching behavior is being used. But do we want to stick `attr` in the name of this to limit it to just attributes or do we think this is a generalized concept that should apply to matching other names as well? e.g., `hasType(equivalentName("long"))` to match either `long`, `long int` or `signed long int` and `hasType(exactName(`long int`))` to only match `long int` and not the other forms? Or do you think this is a bridge too far because it would be reasonable to expect `hasType(equivalentName("long"))` to match typedef names because those are equivalent (or perhaps more problematically, would the user expect `hasType(equivalentName("struct foo<bar>::baz<quux>"))` to match as an equivalent name for the parameter type (spelled with a `using` declaration) in `void foo(blah b)`)? (I think I've just about talked myself into keeping `attr` in the name so we don't try to use this for other kinds of identifiers with equivalent names.)

Go ahead and land the dyn node bits and the `attr` matcher. If you feel like being on the hook for doing the other matchers that we're discussing in another review, then yay, but don't feel obligated (though I appreciate the design discussion!).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743