Support Attr in DynTypedNode and ASTMatchers.

[hokein]

Looks like there are some failing tests in premerge bots, e.g. https://reviews.llvm.org/harbormaster/unit/view/185822/

Also run clang/docs/tools/dump_ast_matchers.py script to update the LibASTMatchersReference.html file.

[sammccall]

Update docs, dynamic registry and tests.

[aaron.ballman]

This is *awesome*, thank you so much for working on it! One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute" -- are you planning to extend this functionality so that you can do something like attr(hasName("foo")), or specify the syntax the attribute uses, isImplicit(), etc?

clang/include/clang/ASTMatchers/ASTMatchers.h

6737

Can you also add an example using __declspec()?

Should I expect this to work on attributes added in other, perhaps surprising ways, like through #pragma or keywords?

// Some pragmas add attributes under the hood
#pragma omp declare simd 
int min(int a, int b) { return a < b ? a : b; }

// Other pragmas only exist to add attributes
#pragma clang attribute push (__attribute__((annotate("custom"))), apply_to = function)
void function(); // The function now has the annotate("custom") attribute
#pragma clang attribute pop

// Still other attributes come through keywords
alignas(16) int i;

If this is expected to match, we should document it more clearly.

clang/lib/ASTMatchers/ASTMatchFinder.cpp

238

Should we be properly handling IgnoreUnlessSpelledInSource for implicit attributes? e.g., [[gnu::interrupt]] void func(int *i); which gets two attributes (the interrupt attribute and an implicit used attribute).

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

1879

Why?

[sammccall]

In D89743#2341779, @aaron.ballman wrote:

This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

are you planning to extend this functionality so that you can do something like attr(hasName("foo")), or specify the syntax the attribute uses, isImplicit(), etc?

I hadn't planned to - it definitely makes sense though I don't have any immediate use cases. I can do any of:

• leave as-is, waiting for someone to add matchers to make it useful
• scope down the patch to exclude the matcher (and write the ASTTypeTraitsTest another way)
• add some simple matcher like hasName (I guess in a followup patch) to make it minimally useful, with space for more

What do you think?

clang/include/clang/ASTMatchers/ASTMatchers.h

6737

I think the answer to all of these is yes, I'll update the docs.

clang/lib/ASTMatchers/ASTMatchFinder.cpp

238

I think we should, I just wasn't thinking clearly about where that should go :-)

[aaron.ballman]

In D89743#2341900, @sammccall wrote:

In D89743#2341779, @aaron.ballman wrote:

This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

Okay, that makes sense to me.

are you planning to extend this functionality so that you can do something like attr(hasName("foo")), or specify the syntax the attribute uses, isImplicit(), etc?

I hadn't planned to - it definitely makes sense though I don't have any immediate use cases. I can do any of:

• leave as-is, waiting for someone to add matchers to make it useful
• scope down the patch to exclude the matcher (and write the ASTTypeTraitsTest another way)
• add some simple matcher like hasName (I guess in a followup patch) to make it minimally useful, with space for more

What do you think?

My preference is to add support for hasName() as that's going to be the most common need for matching attributes. If you also added support for matching which syntax is used, support attributes for hasArgument() (which I suppose could be interesting given that there are the parsed attribute arguments and the semantic attribute arguments, and they may differ), etc, I certainly wouldn't complain, but I think at least name matching support is required to make the matcher marginally useful. I'm totally fine if this happens in a follow-up patch (or patches). WDYT?

[sammccall]

Address review comments.
Add hasAttrName() and make isImplicit() support Attr too.

[sammccall]

In D89743#2342229, @aaron.ballman wrote:

In D89743#2341900, @sammccall wrote:

In D89743#2341779, @aaron.ballman wrote:

This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

Okay, that makes sense to me.

are you planning to extend this functionality so that you can do something like attr(hasName("foo")), or specify the syntax the attribute uses, isImplicit(), etc?

I hadn't planned to - it definitely makes sense though I don't have any immediate use cases. I can do any of:

• leave as-is, waiting for someone to add matchers to make it useful
• scope down the patch to exclude the matcher (and write the ASTTypeTraitsTest another way)
• add some simple matcher like hasName (I guess in a followup patch) to make it minimally useful, with space for more

What do you think?

My preference is to add support for hasName() as that's going to be the most common need for matching attributes.

Sounds good (though I ran into issues calling this hasName specifically, see below). There's overlap with hasAttr(Kind) but I think that only handles decls, you can't bind the attribute itself, and it's harder to extend to the other stuff you mentioned.
So maybe hasAttr gets deprecated, or maybe has(attr(hasName("foo"))) is a mouthful for a common case so the shorthand is nice.

If you also added support for matching which syntax is used, support attributes for hasArgument() (which I suppose could be interesting given that there are the parsed attribute arguments and the semantic attribute arguments, and they may differ), etc, I certainly wouldn't complain, but I think at least name matching support is required to make the matcher marginally useful.

Yeah, in the interests of time I think i'll need to stop there (my understanding of the attribute model is pretty superficial).

I'm totally fine if this happens in a follow-up patch (or patches). WDYT?

For hasAttrName and isImplicit (which simplifies the tests), I've tried to fold it into this patch, it's simple enough.
I went with hasAttrName (matching Attr::getAttrName()) because I wasn't easily able to get attrName to work.

The issue is that hasName is already the name of a Decl matcher with the same signature, so we'd have to make it polymorphic.
(This means they'd appear to be one matcher, with one set of docs etc, despite having very different semantics - doesn't really seem right...)
The implementation of the Decl version is the fairly complex HasNameMatcher (shared with hasAnyName) that copies strings into a vector on construction. This doesn't fit into the polymorphic matcher model as far as I can find, so we're adding heap allocations to the fastpath of matching hasName("foo") which seems... not obviously a good tradeoff.

If you think the hasName name is important I'm happy to drop hasAttrName from this patch, and someone can work out how to add it with the better name, but I'm not sure I want to pick this battle with the template gods...

[sammccall]

Fix nullptr condition (probably dead, but...)

[aaron.ballman]

In D89743#2348487, @sammccall wrote:

In D89743#2342229, @aaron.ballman wrote:

My preference is to add support for hasName() as that's going to be the most common need for matching attributes.

Sounds good (though I ran into issues calling this hasName specifically, see below). There's overlap with hasAttr(Kind) but I think that only handles decls, you can't bind the attribute itself, and it's harder to extend to the other stuff you mentioned.
So maybe hasAttr gets deprecated, or maybe has(attr(hasName("foo"))) is a mouthful for a common case so the shorthand is nice.

I was sort of expecting hasAttr() to be deprecated because it is a leaky abstraction (consumers of the API have to know about our internal naming convention for attribute kind enumerations, it makes it harder for use to change the definition of the enumeration, etc.

If you also added support for matching which syntax is used, support attributes for hasArgument() (which I suppose could be interesting given that there are the parsed attribute arguments and the semantic attribute arguments, and they may differ), etc, I certainly wouldn't complain, but I think at least name matching support is required to make the matcher marginally useful.

Yeah, in the interests of time I think i'll need to stop there (my understanding of the attribute model is pretty superficial).

That's fine by me, thank you for the bits you've done!

I'm totally fine if this happens in a follow-up patch (or patches). WDYT?

For hasAttrName and isImplicit (which simplifies the tests), I've tried to fold it into this patch, it's simple enough.
I went with hasAttrName (matching Attr::getAttrName()) because I wasn't easily able to get attrName to work.

The issue is that hasName is already the name of a Decl matcher with the same signature, so we'd have to make it polymorphic.
(This means they'd appear to be one matcher, with one set of docs etc, despite having very different semantics - doesn't really seem right...)

I definitely don't agree that hasName() has different semantics for attributes from declarations -- both of them are named identifiers, potentially with scope information as part of the name. Making the matcher polymorphic matches my expectations (pun intended).

The implementation of the Decl version is the fairly complex HasNameMatcher (shared with hasAnyName) that copies strings into a vector on construction. This doesn't fit into the polymorphic matcher model as far as I can find, so we're adding heap allocations to the fastpath of matching hasName("foo") which seems... not obviously a good tradeoff.

This, on the other hand, is a very good reason to not push forward with those changes yet.

If you think the hasName name is important I'm happy to drop hasAttrName from this patch, and someone can work out how to add it with the better name, but I'm not sure I want to pick this battle with the template gods...

I'm curious if @klimek agrees, but I think hasName() is the correct way we want to eventually go. That said, I think having an attribute matcher with no way to match by name encourages an anti-pattern where people write a very generic matcher that matches a lot and then do further refinement work for every match (rather than taking advantage of memoization and other optimizations), so I think hasAttrName() is still better than nothing. Would it make sense to add the API but mark it as deprecated to alert users that this API may go away some day if we can swing it? It's a bit unfriendly since we don't have a replacement API lined up for users to use, so I wouldn't want to issue any diagnostics for using the matcher.

clang/include/clang/ASTMatchers/ASTMatchers.h

6763

If you want to avoid duplicating the call to getAttrName() (I don't insist).

if (const IdentifierInfo *II = Node.getAttrName())
  return II->isStr(Name);
return false;

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

729

Are the changes to the HasNameMatcher needed?

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

1893

nodes an -> nodes create an

[njames93]

clang/include/clang/ASTMatchers/ASTMatchers.h

6762

AST_MATCHER macros that take StringRef paramaters don't own the data for the string, which will result in UB if they are passed a reference to a string that is destroyed while the matcher is still matching. It's much safer to use std::string in this case.

[sammccall]

Address comments

[sammccall]

In D89743#2356172, @aaron.ballman wrote:

I was sort of expecting hasAttr() to be deprecated because it is a leaky abstraction (consumers of the API have to know about our internal naming convention for attribute kind enumerations, it makes it harder for use to change the definition of the enumeration, etc.

Sure, SGTM

I'm totally fine if this happens in a follow-up patch (or patches). WDYT?

I definitely don't agree that hasName() has different semantics for attributes from declarations -- both of them are named identifiers, potentially with scope information as part of the name. Making the matcher polymorphic matches my expectations (pun intended).

Fair enough - this isn't really my area, but the differences I was thinking of were:

• the Attr uses a name rather than owning it - it's more like DeclRefExpr than like NamedDecl
• it wasn't clear to me that you'd want the same rules about optional scope handling, though it sounds like you do

If you think the hasName name is important I'm happy to drop hasAttrName from this patch, and someone can work out how to add it with the better name, but I'm not sure I want to pick this battle with the template gods...

I'm curious if @klimek agrees, but I think hasName() is the correct way we want to eventually go. That said, I think having an attribute matcher with no way to match by name encourages an anti-pattern where people write a very generic matcher that matches a lot and then do further refinement work for every match (rather than taking advantage of memoization and other optimizations), so I think hasAttrName() is still better than nothing. Would it make sense to add the API but mark it as deprecated to alert users that this API may go away some day if we can swing it? It's a bit unfriendly since we don't have a replacement API lined up for users to use, so I wouldn't want to issue any diagnostics for using the matcher.

I've added a "deprecated" comment describing the intent to merge into hasName.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

729

Oops, these were left over from a polymorphism experiment

[aaron.ballman]

In D89743#2359764, @sammccall wrote:

In D89743#2356172, @aaron.ballman wrote:

I was sort of expecting hasAttr() to be deprecated because it is a leaky abstraction (consumers of the API have to know about our internal naming convention for attribute kind enumerations, it makes it harder for use to change the definition of the enumeration, etc.

Sure, SGTM

I'm totally fine if this happens in a follow-up patch (or patches). WDYT?

I definitely don't agree that hasName() has different semantics for attributes from declarations -- both of them are named identifiers, potentially with scope information as part of the name. Making the matcher polymorphic matches my expectations (pun intended).

Fair enough - this isn't really my area, but the differences I was thinking of were:

• the Attr uses a name rather than owning it - it's more like DeclRefExpr than like NamedDecl
• it wasn't clear to me that you'd want the same rules about optional scope handling, though it sounds like you do

Thank you for giving this feedback, it's helpful! I may have to rethink this a bit because it never dawned on me that someone would think of attributes being more like a DeclRefExpr than a NamedDecl. If I'm following along properly, this is because the AST node for the attribute could have been generated from various ways of spelling it? e.g., gcc::aligned vs _Alignas vs align vs alignas all making an AlignedAttr. I was thinking this is more like NamedDecl because *one* of those names generated the AST node and I'd hate for AST matchers to have to do something like anyOf(attr(hasSpelling("GNU"), hasName("aligned")), attr(hasSpelling("CXX"), hasName("gcc::aligned)), attr(hasSpelling("declspec"), hasName("align")), attr(hasSpelling("keyword"), hasAnyName("_Alignas", "alignas"))) to match an AlignedAttr regardless of how it is spelled. I figured it would make more sense for attr(hasName("aligned")) to match any AlignedAttr regardless of how the user wrote it in their source code, but perhaps others have different ideas here.

The scope handling is a bit of a question mark. Attributes don't really have namespaces like other identifiers, but they sort of have them when you squint at the name just right. For instance, the scope handling currently allows you to match ::foo in C mode even though C doesn't have the notion of namespaces. So I thought it wouldn't be unintuitive for it to also work for namespace scopes as well.

If you think the hasName name is important I'm happy to drop hasAttrName from this patch, and someone can work out how to add it with the better name, but I'm not sure I want to pick this battle with the template gods...

I'm curious if @klimek agrees, but I think hasName() is the correct way we want to eventually go. That said, I think having an attribute matcher with no way to match by name encourages an anti-pattern where people write a very generic matcher that matches a lot and then do further refinement work for every match (rather than taking advantage of memoization and other optimizations), so I think hasAttrName() is still better than nothing. Would it make sense to add the API but mark it as deprecated to alert users that this API may go away some day if we can swing it? It's a bit unfriendly since we don't have a replacement API lined up for users to use, so I wouldn't want to issue any diagnostics for using the matcher.

I've added a "deprecated" comment describing the intent to merge into hasName.

Thanks! LGTM! Please wait a day or two before landing for @klimek to respond if he has any concerns about introducing a new matcher that's already deprecated.

[aaron.ballman]

Adding @klimek as a reviewer since I forgot to do that last time.

[sammccall]

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

In D89743#2360032, @aaron.ballman wrote:

Thank you for giving this feedback, it's helpful! I may have to rethink this a bit because it never dawned on me that someone would think of attributes being more like a DeclRefExpr than a NamedDecl. If I'm following along properly, this is because the AST node for the attribute could have been generated from various ways of spelling it? e.g., gcc::aligned vs _Alignas vs align vs alignas all making an AlignedAttr.

My mental model is that the Attr classes own the names, and the Attr instances refer to the classes using the names.

So the class AlignedAttr has the name aligned, and the instance [[gnu::aligned(8)]] refers to it. Just like the FunctionDecl double sqrt(double); has the name sqrt, and the DeclRefExpr in sqrt(8) refers to it.

To reinforce this, I can't choose a different name for the instance: [[gnu::whatever]] doesn't produce an Attr instance at all. But I can choose a different name for the class (by modifying Attr.td).
So an Attr instance (what we're matching) feels more like a reference than a primary entity. (Similarly, the semantics are associated with the Attr class rather than the instance).

If clang's AST modeled syntax rather than semantics, I imagine there'd be one Attr class and each instance could have an arbitrary name and arg list. In that case an Attr would seem to "have" a name in the same sense a decl does.

I figured it would make more sense for attr(hasName("aligned")) to match any AlignedAttr regardless of how the user wrote it in their source code, but perhaps others have different ideas here.

That definitely seems like the common case. However I can certainly imagine wanting to e.g. write a migration from __attribute__((xxx)) to [[gnu::xxx]].

(If attr classes were AST nodes too, like DeclRefExpr/Decl or TypeLoc/Type then you'd resolve this by using a matcher on one or the other)

As it stands it seems too surprising that Attr::getAttrName() returns the particular name used for that instance, but attr(hasAttrName("...")) would match any name associated with the same class.
Maybe attr(equivalentAttrName("...")) or so, possibly constrasting to attr(exactAttrName("..."))?

The scope handling is a bit of a question mark. Attributes don't really have namespaces like other identifiers, but they sort of have them when you squint at the name just right. For instance, the scope handling currently allows you to match ::foo in C mode even though C doesn't have the notion of namespaces. So I thought it wouldn't be unintuitive for it to also work for namespace scopes as well.

Yup, I don't have a real opinion here, I just assumed it was different. Seems like it would work fine.

[aaron.ballman]

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

In D89743#2360032, @aaron.ballman wrote:

Thank you for giving this feedback, it's helpful! I may have to rethink this a bit because it never dawned on me that someone would think of attributes being more like a DeclRefExpr than a NamedDecl. If I'm following along properly, this is because the AST node for the attribute could have been generated from various ways of spelling it? e.g., gcc::aligned vs _Alignas vs align vs alignas all making an AlignedAttr.

My mental model is that the Attr classes own the names, and the Attr instances refer to the classes using the names.

So the class AlignedAttr has the name aligned, and the instance [[gnu::aligned(8)]] refers to it. Just like the FunctionDecl double sqrt(double); has the name sqrt, and the DeclRefExpr in sqrt(8) refers to it.

To reinforce this, I can't choose a different name for the instance: [[gnu::whatever]] doesn't produce an Attr instance at all. But I can choose a different name for the class (by modifying Attr.td).
So an Attr instance (what we're matching) feels more like a reference than a primary entity. (Similarly, the semantics are associated with the Attr class rather than the instance).

If clang's AST modeled syntax rather than semantics, I imagine there'd be one Attr class and each instance could have an arbitrary name and arg list. In that case an Attr would seem to "have" a name in the same sense a decl does.

Oh, how interesting, thank you for expounding!

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.

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

I figured it would make more sense for attr(hasName("aligned")) to match any AlignedAttr regardless of how the user wrote it in their source code, but perhaps others have different ideas here.

That definitely seems like the common case. However I can certainly imagine wanting to e.g. write a migration from __attribute__((xxx)) to [[gnu::xxx]].

Definitely true! I was imagining we'd want a separate checker for narrowing based on attribute syntax.

(If attr classes were AST nodes too, like DeclRefExpr/Decl or TypeLoc/Type then you'd resolve this by using a matcher on one or the other)

As it stands it seems too surprising that Attr::getAttrName() returns the particular name used for that instance, but attr(hasAttrName("...")) would match any name associated with the same class.
Maybe attr(equivalentAttrName("...")) or so, possibly constrasting to attr(exactAttrName("..."))?

Yeah, I am starting to see this more readily, so thank you for continuing the discussion!

The scope handling is a bit of a question mark. Attributes don't really have namespaces like other identifiers, but they sort of have them when you squint at the name just right. For instance, the scope handling currently allows you to match ::foo in C mode even though C doesn't have the notion of namespaces. So I thought it wouldn't be unintuitive for it to also work for namespace scopes as well.

Yup, I don't have a real opinion here, I just assumed it was different. Seems like it would work fine.

[sammccall]

In D89743#2363201, @aaron.ballman wrote:

In 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, @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("..."))?

[aaron.ballman]

In D89743#2406017, @sammccall wrote:

In D89743#2363201, @aaron.ballman wrote:

In 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, @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!).

[sammccall]

OK, I'm going to drop hasAttrName from the patch (it's a relatively tiny piece of the code/tests) and land this, and put that back up as another review for discussion. And thanks, it's been interesting and I learned a bunch!
We didn't talk about overloading isImplicit to apply to attrs too, but it doesn't seem like that was controversial (and it does help with the tests).

In D89743#2408747, @aaron.ballman wrote:

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

Putting attr in the name is the *safe* option - as we've seen, going for a generic name and hoping it can work consistently for all types over time is risky.
(The cost of a confusing "overload" set is also high because the polymorphism mechanism is poorly understood - most users probably can't easily look up the variant they're using and ignore the others).
On the other hand, when it is consistent, I think the uniformity is nice - so a generic name isn't without merit, it's just a bit of a gamble.

[sammccall]

In D89743#2409001, @sammccall wrote:

We didn't talk about overloading isImplicit to apply to attrs too, but it doesn't seem like that was controversial (and it does help with the tests).

I spoke too soon about this...
This prevents hasAncestor(isImplicit()) from compiling, because hasAncestor needs to deduce the node type from its argument to call ASTMatchFinder::matchesAncestorOf<T>().
This occurs in a few places in tree and many places in our private codebase...
The workaround is hasAncestor(decl(isImplicit())) which is reasonable, except that "is contained in *any* implicit node" is probably actually the intent. Well, at least it's not a regression.

In addition, while digging into this, I realized Attrs are not traversed in the parent map, and not supported by the parent/child/ancestor/descendant traversals.
So I'm fixing that... and adding some tests.

I'll need to send this for another round, even without the name matcher.

[sammccall]

Drop hasAttrName matcher.

Update uses of hasAncestor(isImplicit()) to hasAncestor(decl(isImplicit())),
as the former no longer compiles due to lack of deduction.

Implement and test up/down traversals involving attrs.

[aaron.ballman]

In D89743#2409115, @sammccall wrote:

In D89743#2409001, @sammccall wrote:

We didn't talk about overloading isImplicit to apply to attrs too, but it doesn't seem like that was controversial (and it does help with the tests).

I spoke too soon about this...
This prevents hasAncestor(isImplicit()) from compiling, because hasAncestor needs to deduce the node type from its argument to call ASTMatchFinder::matchesAncestorOf<T>().
This occurs in a few places in tree and many places in our private codebase...
The workaround is hasAncestor(decl(isImplicit())) which is reasonable, except that "is contained in *any* implicit node" is probably actually the intent. Well, at least it's not a regression.

Users can always traverse in IgnoreUnlessSpelledInSource mode for that situation though, so at least there's a reasonable path forward.

In addition, while digging into this, I realized Attrs are not traversed in the parent map, and not supported by the parent/child/ancestor/descendant traversals.
So I'm fixing that... and adding some tests.

Good catch!

I'll need to send this for another round, even without the name matcher.

Thank you!

clang/lib/AST/ASTTypeTraits.cpp

138

Oye, this brings up an interesting point. Plugin-based attributes currently cannot create their own semantic attribute, but will often instead reuse an existing semantic attribute like annotate. This means code like [[clang::plugin_attr]] int x; may or may not be possible to match. Further, some builtin attributes have no semantic attribute associated with them whatsoever: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Attr.td#L2740

I think the switch statement logic here is correct in these weird cases and we won't hit the llvm_unreachable. For attributes with no AST representation, there's no Attr object that could be passed in the first place. Unknown attributes similarly won't get here because there's no way to get an AST node for them. Plugin-based attributes are still going to be similarly surprising, but... I don't know that we can solve that here given there's no way to create a plugin-based semantic attribute yet.

Pining @ymandel to raise awareness of these sorts of issues that stencil may run into. For the AST matchers, I think it's reasonable for us to say "if there's no AST node, we can't match on it", but IIRC, stencil was looking to stay a bit closer to the user's source code rather than be strongly tied to the AST.

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

1887

Can you add an expects false test for an unknown attribute and another one for an attribute with no AST node associated with it?

[ymandel]

clang/lib/AST/ASTTypeTraits.cpp

138

@aaron.ballman Thanks for pinging me. However, Stencil is limited to AST nodes, for better or worse. They make it somewhat easier to _generate_ source plainly, but they are fundamentaly an abstraction over the AST. I think that the only way we'll get beyond the AST is Syntax Trees.

Still, nice to see this patch! I've been meaning to do the same for LambaCapture for a while and this will be a handy guide to what needs to be changed.

[aaron.ballman]

LGTM aside from a minor testing request.

clang/lib/AST/ASTTypeTraits.cpp

138

That's good to know, @ymandel, thank you!

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

1887

This comment may have been missed during the discussion.

[sammccall]

Finally getting around to landing this, sorry for the long delay!

clang/include/clang/AST/ASTFwd.h

 #include "clang/AST/StmtNodes.inc"
 class Type;
 #define TYPE(DERIVED, BASE) class DERIVED##Type;
 #include "clang/AST/TypeNodes.inc"
 class CXXCtorInitializer;
 class OMPClause;
 #define OMP_CLAUSE_CLASS(Enum, Str, Class) class Class;
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
-
+class Attr;
+#define ATTR(A) class A##Attr;
+#include "clang/Basic/AttrList.inc"
 
 } // end namespace clang
 
 #endif

clang/include/clang/AST/ASTTypeTraits.h

 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/Support/AlignOf.h"
 
 namespace llvm {
-
 class raw_ostream;
-
-}
+} // namespace llvm
 
 namespace clang {
 
 struct PrintingPolicy;
 
 /// Defines how we descend a level in the AST when we pass
 /// through expressions.
 enum TraversalKind {
public:
   }
 
   /// \{
   /// Construct an identifier for the dynamic type of the node
   static ASTNodeKind getFromNode(const Decl &D);
   static ASTNodeKind getFromNode(const Stmt &S);
   static ASTNodeKind getFromNode(const Type &T);
   static ASTNodeKind getFromNode(const OMPClause &C);
+  static ASTNodeKind getFromNode(const Attr &A);
   /// \}
 
   /// Returns \c true if \c this and \c Other represent the same kind.
   bool isSame(ASTNodeKind Other) const {
     return KindId != NKI_None && KindId == Other.KindId;
   }
 
   /// Returns \c true only for the default \c ASTNodeKind()

 #define STMT(DERIVED, BASE) NKI_##DERIVED,
 #include "clang/AST/StmtNodes.inc"
     NKI_Type,
 #define TYPE(DERIVED, BASE) NKI_##DERIVED##Type,
 #include "clang/AST/TypeNodes.inc"
     NKI_OMPClause,
 #define OMP_CLAUSE_CLASS(Enum, Str, Class) NKI_##Class,
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
+    NKI_Attr,
+#define ATTR(A) NKI_##A##Attr,
+#include "clang/Basic/AttrList.inc"
     NKI_NumberOfKinds
   };
 
   /// Use getFromNodeKind<T>() to construct the kind.
   ASTNodeKind(NodeKindId KindId) : KindId(KindId) {}
 
   /// Returns \c true if \c Base is a base kind of (or same as) \c
   ///   Derived.

 KIND_TO_KIND_ID(NestedNameSpecifier)
 KIND_TO_KIND_ID(NestedNameSpecifierLoc)
 KIND_TO_KIND_ID(QualType)
 KIND_TO_KIND_ID(TypeLoc)
 KIND_TO_KIND_ID(Decl)
 KIND_TO_KIND_ID(Stmt)
 KIND_TO_KIND_ID(Type)
 KIND_TO_KIND_ID(OMPClause)
+KIND_TO_KIND_ID(Attr)
 KIND_TO_KIND_ID(CXXBaseSpecifier)
 #define DECL(DERIVED, BASE) KIND_TO_KIND_ID(DERIVED##Decl)
 #include "clang/AST/DeclNodes.inc"
 #define STMT(DERIVED, BASE) KIND_TO_KIND_ID(DERIVED)
 #include "clang/AST/StmtNodes.inc"
 #define TYPE(DERIVED, BASE) KIND_TO_KIND_ID(DERIVED##Type)
 #include "clang/AST/TypeNodes.inc"
 #define OMP_CLAUSE_CLASS(Enum, Str, Class) KIND_TO_KIND_ID(Class)
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
+#define ATTR(A) KIND_TO_KIND_ID(A##Attr)
+#include "clang/Basic/AttrList.inc"
 #undef KIND_TO_KIND_ID
 
 inline raw_ostream &operator<<(raw_ostream &OS, ASTNodeKind K) {
   OS << K.asStringRef();
   return OS;
 }
 
 /// A dynamically typed AST node container.
struct DynTypedNode::BaseConverter<
     T, std::enable_if_t<std::is_base_of<Type, T>::value>>
     : public DynCastPtrConverter<T, Type> {};
 
 template <typename T>
 struct DynTypedNode::BaseConverter<
     T, std::enable_if_t<std::is_base_of<OMPClause, T>::value>>
     : public DynCastPtrConverter<T, OMPClause> {};
 
+template <typename T>
+struct DynTypedNode::BaseConverter<
+    T, std::enable_if_t<std::is_base_of<Attr, T>::value>>
+    : public DynCastPtrConverter<T, Attr> {};
+
 template <>
 struct DynTypedNode::BaseConverter<
     NestedNameSpecifier, void> : public PtrConverter<NestedNameSpecifier> {};
 
 template <>
 struct DynTypedNode::BaseConverter<
     CXXCtorInitializer, void> : public PtrConverter<CXXCtorInitializer> {};
 

clang/include/clang/ASTMatchers/ASTMatchFinder.h

public:
   void addMatcher(const NestedNameSpecifierLocMatcher &NodeMatch,
                   MatchCallback *Action);
   void addMatcher(const TypeLocMatcher &NodeMatch,
                   MatchCallback *Action);
   void addMatcher(const CXXCtorInitializerMatcher &NodeMatch,
                   MatchCallback *Action);
   void addMatcher(const TemplateArgumentLocMatcher &NodeMatch,
                   MatchCallback *Action);
+  void addMatcher(const AttrMatcher &NodeMatch, MatchCallback *Action);
   /// @}
 
   /// Adds a matcher to execute when running over the AST.
   ///
   /// This is similar to \c addMatcher(), but it uses the dynamic interface. It
   /// is more flexible, but the lost type information enables a caller to pass
   /// a matcher that cannot match anything.
   ///
struct MatchersByType {
     std::vector<std::pair<NestedNameSpecifierMatcher, MatchCallback *>>
         NestedNameSpecifier;
     std::vector<std::pair<NestedNameSpecifierLocMatcher, MatchCallback *>>
         NestedNameSpecifierLoc;
     std::vector<std::pair<TypeLocMatcher, MatchCallback *>> TypeLoc;
     std::vector<std::pair<CXXCtorInitializerMatcher, MatchCallback *>> CtorInit;
     std::vector<std::pair<TemplateArgumentLocMatcher, MatchCallback *>>
         TemplateArgumentLoc;
+    std::vector<std::pair<AttrMatcher, MatchCallback *>> Attr;
     /// All the callbacks in one container to simplify iteration.
     llvm::SmallPtrSet<MatchCallback *, 16> AllCallbacks;
   };
 
 private:
   MatchersByType Matchers;
 
   MatchFinderOptions Options;

clang/include/clang/ASTMatchers/ASTMatchers.h

 using DeclarationMatcher = internal::Matcher<Decl>;
 using StatementMatcher = internal::Matcher<Stmt>;
 using TypeMatcher = internal::Matcher<QualType>;
 using TypeLocMatcher = internal::Matcher<TypeLoc>;
 using NestedNameSpecifierMatcher = internal::Matcher<NestedNameSpecifier>;
 using NestedNameSpecifierLocMatcher = internal::Matcher<NestedNameSpecifierLoc>;
 using CXXCtorInitializerMatcher = internal::Matcher<CXXCtorInitializer>;
 using TemplateArgumentLocMatcher = internal::Matcher<TemplateArgumentLoc>;
+using AttrMatcher = internal::Matcher<Attr>;
 /// @}
 
 /// Matches any node.
 ///
 /// Useful when another matcher requires a child matcher, but there's no
 /// additional constraint. This will often be used with an explicit conversion
 /// to an \c internal::Matcher<> type such as \c TypeMatcher.
 ///

 ///   matches "ns::"
 AST_MATCHER_P(NestedNameSpecifier, specifiesNamespace,
               internal::Matcher<NamespaceDecl>, InnerMatcher) {
   if (!Node.getAsNamespace())
     return false;
   return InnerMatcher.matches(*Node.getAsNamespace(), Finder, Builder);
 }
 
+/// Matches attributes.
+///
+/// Given
+/// \code
+///   struct [[nodiscard]] Foo{};
+///   void bar(int * __attribute__((nonnull)) );
+/// \endcode
+/// attr()
+///   matches "nodiscard" and "nonnull"
+extern const internal::VariadicAllOfMatcher<Attr> attr;
+
 /// Overloads for the \c equalsNode matcher.
 /// FIXME: Implement for other node types.
 /// @{
 
 /// Matches if a node equals another node.
 ///
 /// \c Decl has pointer identity in the AST.
 AST_MATCHER_P_OVERLOAD(Decl, equalsNode, const Decl*, Other, 0) {
   return &Node == Other;
 }
 /// Matches if a node equals another node.
 ///
 /// \c Stmt has pointer identity in the AST.
 AST_MATCHER_P_OVERLOAD(Stmt, equalsNode, const Stmt*, Other, 1) {
   return &Node == Other;
 }
 /// Matches if a node equals another node.
 ///
 /// \c Type has pointer identity in the AST.
 AST_MATCHER_P_OVERLOAD(Type, equalsNode, const Type*, Other, 2) {
     return &Node == Other;
 }
 
 /// @}
 
 /// Matches each case or default statement belonging to the given switch
 /// statement. This matcher may produce multiple matches.
 ///
 /// Given

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

 ///
 /// See \c hasName() and \c hasAnyName() in ASTMatchers.h for details.
 class HasNameMatcher : public SingleNodeMatcherInterface<NamedDecl> {
  public:
   explicit HasNameMatcher(std::vector<std::string> Names);
 
   bool matchesNode(const NamedDecl &Node) const override;
 
  private:
   /// Unqualified match routine.
   ///
   /// It is much faster than the full match, but it only works for unqualified
   /// matches.
   bool matchesNodeUnqualified(const NamedDecl &Node) const;
 
   /// Full match routine
   ///

 struct IsBaseType {
   static const bool value =
       std::is_same<T, Decl>::value || std::is_same<T, Stmt>::value ||
       std::is_same<T, QualType>::value || std::is_same<T, Type>::value ||
       std::is_same<T, TypeLoc>::value ||
       std::is_same<T, NestedNameSpecifier>::value ||
       std::is_same<T, NestedNameSpecifierLoc>::value ||
       std::is_same<T, CXXCtorInitializer>::value ||
-      std::is_same<T, TemplateArgumentLoc>::value;
+      std::is_same<T, TemplateArgumentLoc>::value ||
+      std::is_same<T, Attr>::value;
 };
 template <typename T>
 const bool IsBaseType<T>::value;
 
 /// Interface that allows matchers to traverse the AST.
 /// FIXME: Find a better name.
 ///
 /// This provides three entry methods for each base node type in the AST:

clang/lib/AST/ASTTypeTraits.cpp

 //===--- ASTTypeTraits.cpp --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  Provides a dynamic type identifier and a dynamically typed node container
 //  that can be used to store an AST base node at runtime in the same storage in
 //  a type safe way.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/OpenMPClause.h"
 
 using namespace clang;
 
 const ASTNodeKind::KindInfo ASTNodeKind::AllKindInfo[] = {
-  { NKI_None, "<None>" },
-  { NKI_None, "TemplateArgument" },
-  { NKI_None, "TemplateArgumentLoc" },
-  { NKI_None, "TemplateName" },
-  { NKI_None, "NestedNameSpecifierLoc" },
-  { NKI_None, "QualType" },
-  { NKI_None, "TypeLoc" },
-  { NKI_None, "CXXBaseSpecifier" },
-  { NKI_None, "CXXCtorInitializer" },
-  { NKI_None, "NestedNameSpecifier" },
-  { NKI_None, "Decl" },
+    {NKI_None, "<None>"},
+    {NKI_None, "TemplateArgument"},
+    {NKI_None, "TemplateArgumentLoc"},
+    {NKI_None, "TemplateName"},
+    {NKI_None, "NestedNameSpecifierLoc"},
+    {NKI_None, "QualType"},
+    {NKI_None, "TypeLoc"},
+    {NKI_None, "CXXBaseSpecifier"},
+    {NKI_None, "CXXCtorInitializer"},
+    {NKI_None, "NestedNameSpecifier"},
+    {NKI_None, "Decl"},
 #define DECL(DERIVED, BASE) { NKI_##BASE, #DERIVED "Decl" },
 #include "clang/AST/DeclNodes.inc"
-  { NKI_None, "Stmt" },
+    {NKI_None, "Stmt"},
 #define STMT(DERIVED, BASE) { NKI_##BASE, #DERIVED },
 #include "clang/AST/StmtNodes.inc"
-  { NKI_None, "Type" },
+    {NKI_None, "Type"},
 #define TYPE(DERIVED, BASE) { NKI_##BASE, #DERIVED "Type" },
 #include "clang/AST/TypeNodes.inc"
-  { NKI_None, "OMPClause" },
+    {NKI_None, "OMPClause"},
 #define OMP_CLAUSE_CLASS(Enum, Str, Class) {NKI_OMPClause, #Class},
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
+    {NKI_None, "Attr"},
+#define ATTR(A) {NKI_Attr, #A "Attr"},
+#include "clang/Basic/AttrList.inc"
 };
 
 bool ASTNodeKind::isBaseOf(ASTNodeKind Other, unsigned *Distance) const {
   return isBaseOf(KindId, Other.KindId, Distance);
 }
 
 bool ASTNodeKind::isBaseOf(NodeKindId Base, NodeKindId Derived,
                            unsigned *Distance) {
case llvm::omp::Clause::Enum:                                                \
     return ASTNodeKind(NKI_##Class);
 #define OMP_CLAUSE_NO_CLASS(Enum, Str)                                         \
   case llvm::omp::Clause::Enum:                                                \
     llvm_unreachable("unexpected OpenMP clause kind");
   default:
     break;
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
   }
-  llvm_unreachable("invalid stmt kind");
+  llvm_unreachable("invalid omp clause kind");
+}
+
+ASTNodeKind ASTNodeKind::getFromNode(const Attr &A) {
+  switch (A.getKind()) {
+#define ATTR(A)                                                                \
+  case attr::A:                                                                \
+    return ASTNodeKind(NKI_##A##Attr);
+#include "clang/Basic/AttrList.inc"
+  }
+  llvm_unreachable("invalid attr kind");
 }
 
 void DynTypedNode::print(llvm::raw_ostream &OS,
                          const PrintingPolicy &PP) const {
   if (const TemplateArgument *TA = get<TemplateArgument>())
     TA->print(PP, OS);
   else if (const TemplateArgumentLoc *TAL = get<TemplateArgumentLoc>())
     TAL->getArgument().print(PP, OS);
void DynTypedNode::print(llvm::raw_ostream &OS,
   else if (const TypeLoc *TL = get<TypeLoc>())
     TL->getType().print(OS, PP);
   else if (const Decl *D = get<Decl>())
     D->print(OS, PP);
   else if (const Stmt *S = get<Stmt>())
     S->printPretty(OS, nullptr, PP);
   else if (const Type *T = get<Type>())
     QualType(T, 0).print(OS, PP);
+  else if (const Attr *A = get<Attr>())
+    A->printPretty(OS, PP);
   else
     OS << "Unable to print values of type " << NodeKind.asStringRef() << "\n";
 }
 
 void DynTypedNode::dump(llvm::raw_ostream &OS,
                         const ASTContext &Context) const {
   if (const Decl *D = get<Decl>())
     D->dump(OS);
SourceRange DynTypedNode::getSourceRange() const {
   if (const Decl *D = get<Decl>())
     return D->getSourceRange();
   if (const Stmt *S = get<Stmt>())
     return S->getSourceRange();
   if (const TemplateArgumentLoc *TAL = get<TemplateArgumentLoc>())
     return TAL->getSourceRange();
   if (const auto *C = get<OMPClause>())
     return SourceRange(C->getBeginLoc(), C->getEndLoc());
+  if (const auto *A = get<Attr>())
+    return A->getRange();
   return SourceRange();
 }

clang/lib/ASTMatchers/ASTMatchFinder.cpp

else if (const QualType *Q = DynNode.get<QualType>())
       traverse(*Q);
     else if (const TypeLoc *T = DynNode.get<TypeLoc>())
       traverse(*T);
     else if (const auto *C = DynNode.get<CXXCtorInitializer>())
       traverse(*C);
     else if (const TemplateArgumentLoc *TALoc =
                  DynNode.get<TemplateArgumentLoc>())
       traverse(*TALoc);
+    else if (const Attr *A = DynNode.get<Attr>())
+      traverse(*A);
     // FIXME: Add other base types after adding tests.
 
     // It's OK to always overwrite the bound nodes, as if there was
     // no match in this recursive branch, the result set is empty
     // anyway.
     *Builder = ResultBindings;
 
     return Matches;
if (!CtorInit)
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
     return traverse(*CtorInit);
   }
   bool TraverseTemplateArgumentLoc(TemplateArgumentLoc TAL) {
     ScopedIncrement ScopedDepth(&CurrentDepth);
     return traverse(TAL);
   }
+  bool TraverseAttr(Attr *A) {
+    ScopedIncrement ScopedDepth(&CurrentDepth);
+    return (A == nullptr || traverse(*A));
+  }
   bool TraverseLambdaExpr(LambdaExpr *Node) {
     if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
         TK_IgnoreUnlessSpelledInSource)
       return VisitorBase::TraverseLambdaExpr(Node);
     if (!Node)
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
 
private:
   }
   bool baseTraverse(const CXXCtorInitializer &CtorInit) {
     return VisitorBase::TraverseConstructorInitializer(
         const_cast<CXXCtorInitializer *>(&CtorInit));
   }
   bool baseTraverse(TemplateArgumentLoc TAL) {
     return VisitorBase::TraverseTemplateArgumentLoc(TAL);
   }
+  bool baseTraverse(const Attr &AttrNode) {
+    return VisitorBase::TraverseAttr(const_cast<Attr *>(&AttrNode));
+  }
 
   // Sets 'Matched' to true if 'Matcher' matches 'Node' and:
   //   0 < CurrentDepth <= MaxDepth.
   //
   // Returns 'true' if traversal should continue after this function
   // returns, i.e. if no match is found or 'Bind' is 'BK_All'.
   template <typename T>
   bool match(const T &Node) {
public:
   bool TraverseDecl(Decl *DeclNode);
   bool TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue = nullptr);
   bool TraverseType(QualType TypeNode);
   bool TraverseTypeLoc(TypeLoc TypeNode);
   bool TraverseNestedNameSpecifier(NestedNameSpecifier *NNS);
   bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS);
   bool TraverseConstructorInitializer(CXXCtorInitializer *CtorInit);
   bool TraverseTemplateArgumentLoc(TemplateArgumentLoc TAL);
+  bool TraverseAttr(Attr *AttrNode);
 
   // Matches children or descendants of 'Node' with 'BaseMatcher'.
   bool memoizedMatchesRecursively(const DynTypedNode &Node, ASTContext &Ctx,
                                   const DynTypedMatcher &Matcher,
                                   BoundNodesTreeBuilder *Builder, int MaxDepth,
                                   TraversalKind Traversal, BindKind Bind) {
     // For AST-nodes that don't have an identity, we can't memoize.
     if (!Node.getMemoizationData() || !Builder->isComparable())
void match(const DynTypedNode &Node) {
     } else if (auto *N = Node.get<NestedNameSpecifierLoc>()) {
       match(*N);
     } else if (auto *N = Node.get<TypeLoc>()) {
       match(*N);
     } else if (auto *N = Node.get<CXXCtorInitializer>()) {
       match(*N);
     } else if (auto *N = Node.get<TemplateArgumentLoc>()) {
       match(*N);
+    } else if (auto *N = Node.get<Attr>()) {
+      match(*N);
     }
   }
 
   template <typename T> void match(const T &Node) {
     matchDispatch(&Node);
   }
 
   // Implements ASTMatchFinder::getASTContext.
void matchDispatch(const NestedNameSpecifierLoc *Node) {
     matchWithoutFilter(*Node, Matchers->NestedNameSpecifierLoc);
   }
   void matchDispatch(const CXXCtorInitializer *Node) {
     matchWithoutFilter(*Node, Matchers->CtorInit);
   }
   void matchDispatch(const TemplateArgumentLoc *Node) {
     matchWithoutFilter(*Node, Matchers->TemplateArgumentLoc);
   }
+  void matchDispatch(const Attr *Node) {
+    matchWithoutFilter(*Node, Matchers->Attr);
+  }
   void matchDispatch(const void *) { /* Do nothing. */ }
   /// @}
 
   // Returns whether a direct parent of \p Node matches \p Matcher.
   // Unlike matchesAnyAncestorOf there's no memoization: it doesn't save much.
   bool matchesParentOf(const DynTypedNode &Node, const DynTypedMatcher &Matcher,
                        BoundNodesTreeBuilder *Builder) {
     for (const auto &Parent : ActiveASTContext->getParents(Node)) {
return RecursiveASTVisitor<MatchASTVisitor>::TraverseConstructorInitializer(
       CtorInit);
 }
 
 bool MatchASTVisitor::TraverseTemplateArgumentLoc(TemplateArgumentLoc Loc) {
   match(Loc);
   return RecursiveASTVisitor<MatchASTVisitor>::TraverseTemplateArgumentLoc(Loc);
 }
 
+bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) {
+  match(*AttrNode);
+  return RecursiveASTVisitor<MatchASTVisitor>::TraverseAttr(AttrNode);
+}
+
 class MatchASTConsumer : public ASTConsumer {
 public:
   MatchASTConsumer(MatchFinder *Finder,
                    MatchFinder::ParsingDoneTestCallback *ParsingDone)
       : Finder(Finder), ParsingDone(ParsingDone) {}
 
 private:
   void HandleTranslationUnit(ASTContext &Context) override {

 }
 
 void MatchFinder::addMatcher(const TemplateArgumentLocMatcher &NodeMatch,
                              MatchCallback *Action) {
   Matchers.TemplateArgumentLoc.emplace_back(NodeMatch, Action);
   Matchers.AllCallbacks.insert(Action);
 }
 
+void MatchFinder::addMatcher(const AttrMatcher &AttrMatch,
+                             MatchCallback *Action) {
+  Matchers.Attr.emplace_back(AttrMatch, Action);
+  Matchers.AllCallbacks.insert(Action);
+}
+
 bool MatchFinder::addDynamicMatcher(const internal::DynTypedMatcher &NodeMatch,
                                     MatchCallback *Action) {
   if (NodeMatch.canConvertTo<Decl>()) {
     addMatcher(NodeMatch.convertTo<Decl>(), Action);
     return true;
   } else if (NodeMatch.canConvertTo<QualType>()) {
     addMatcher(NodeMatch.convertTo<QualType>(), Action);
     return true;
if (NodeMatch.canConvertTo<Decl>()) {
     addMatcher(NodeMatch.convertTo<TypeLoc>(), Action);
     return true;
   } else if (NodeMatch.canConvertTo<CXXCtorInitializer>()) {
     addMatcher(NodeMatch.convertTo<CXXCtorInitializer>(), Action);
     return true;
   } else if (NodeMatch.canConvertTo<TemplateArgumentLoc>()) {
     addMatcher(NodeMatch.convertTo<TemplateArgumentLoc>(), Action);
     return true;
+  } else if (NodeMatch.canConvertTo<Attr>()) {
+    addMatcher(NodeMatch.convertTo<Attr>(), Action);
+    return true;
   }
   return false;
 }
 
 std::unique_ptr<ASTConsumer> MatchFinder::newASTConsumer() {
   return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone);
 }
 

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

const internal::ArgumentAdaptingMatcherFunc<
     internal::TypeList<Decl, NestedNameSpecifierLoc, Stmt, TypeLoc>,
     internal::TypeList<Decl, NestedNameSpecifierLoc, Stmt, TypeLoc>>
     hasAncestor = {};
 const internal::VariadicOperatorMatcherFunc<1, 1> unless = {
     internal::DynTypedMatcher::VO_UnaryNot};
 const internal::VariadicAllOfMatcher<NestedNameSpecifier> nestedNameSpecifier;
 const internal::VariadicAllOfMatcher<NestedNameSpecifierLoc>
     nestedNameSpecifierLoc;
+const internal::VariadicAllOfMatcher<Attr> attr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CUDAKernelCallExpr>
     cudaKernelCallExpr;
 const AstTypeMatcher<BuiltinType> builtinType;
 const AstTypeMatcher<ArrayType> arrayType;
 const AstTypeMatcher<ComplexType> complexType;
 const AstTypeMatcher<ConstantArrayType> constantArrayType;
 const AstTypeMatcher<DeducedTemplateSpecializationType>
     deducedTemplateSpecializationType;

clang/unittests/AST/ASTTypeTraitsTest.cpp

#define VERIFY_NAME(Node) EXPECT_EQ(#Node, DNT<Node>().asStringRef());
   VERIFY_NAME(CXXCtorInitializer);
   VERIFY_NAME(NestedNameSpecifier);
   VERIFY_NAME(Decl);
   VERIFY_NAME(CXXRecordDecl);
   VERIFY_NAME(Stmt);
   VERIFY_NAME(CallExpr);
   VERIFY_NAME(Type);
   VERIFY_NAME(ConstantArrayType);
+  VERIFY_NAME(NonNullAttr);
 #undef VERIFY_NAME
 }
 
 TEST(DynTypedNode, DeclSourceRange) {
   RangeVerifier<DynTypedNode> Verifier;
   Verifier.expectRange(1, 1, 1, 11);
   EXPECT_TRUE(Verifier.match("void f() {}", decl()));
 }

 
 TEST(DynTypedNode, NNSLocSourceRange) {
   RangeVerifier<DynTypedNode> Verifier;
   Verifier.expectRange(1, 33, 1, 34);
   EXPECT_TRUE(Verifier.match("namespace N { typedef void T; } N::T f() {}",
                              nestedNameSpecifierLoc()));
 }
 
+TEST(DynTypedNode, AttrSourceRange) {
+  RangeVerifier<DynTypedNode> Verifier;
+  Verifier.expectRange(1, 31, 1, 31);
+  EXPECT_TRUE(Verifier.match("void x(char *y __attribute__((nonnull)) );",
+                             ast_matchers::attr()));
+}
+
 TEST(DynTypedNode, DeclDump) {
   DumpVerifier Verifier;
   Verifier.expectSubstring("FunctionDecl");
   EXPECT_TRUE(Verifier.match("void f() {}", functionDecl()));
 }
 
 TEST(DynTypedNode, StmtDump) {
   DumpVerifier Verifier;

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

TEST_P(ASTMatchersTest, NestedNameSpecifier) {
 
   EXPECT_TRUE(matches("struct A { static void f() {} }; void g() { A::f(); }",
                       nestedNameSpecifier()));
   EXPECT_TRUE(
       notMatches("struct A { static void f() {} }; void g(A* a) { a->f(); }",
                  nestedNameSpecifier()));
 }
 
+TEST_P(ASTMatchersTest, Attr) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(matches("struct [[nodiscard]] F{};", attr()));
+  EXPECT_TRUE(matches("int x(int * __attribute__((nonnull)) );", attr()));
+  EXPECT_TRUE(notMatches("struct F{}; int x(int *);", attr()));
+}
+
 TEST_P(ASTMatchersTest, NullStmt) {
   EXPECT_TRUE(matches("void f() {int i;;}", nullStmt()));
   EXPECT_TRUE(notMatches("void f() {int i;}", nullStmt()));
 }
 
 TEST_P(ASTMatchersTest, NamespaceAliasDecl) {
   if (!GetParam().isCXX()) {
     return;
   }
   EXPECT_TRUE(matches("namespace test {} namespace alias = ::test;",
                       namespaceAliasDecl(hasName("alias"))));
 }
 
 TEST_P(ASTMatchersTest, NestedNameSpecifier_MatchesTypes) {
   if (!GetParam().isCXX()) {