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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman edited subscribers, added: ymandel; removed: aaron.ballman.
aaron.ballman added a comment.

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

> In D89743#2409001 <https://reviews.llvm.org/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!



================
Comment at: clang/lib/AST/ASTTypeTraits.cpp:138
+    return ASTNodeKind(NKI_##A##Attr);
+#include "clang/Basic/AttrList.inc"
+  }
----------------
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.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1887
+  // On windows, some nodes have an implicit visibility attribute.
+  EXPECT_TRUE(
+      notMatches("struct F{}; int x(int *);", attr(unless(isImplicit()))));
----------------
Can you add an expects false test for an unknown attribute and another one for an attribute with no AST node associated with it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743