Record the matcher type when storing a binding

[steveire]

Summary

This is necessary so that when we wish to print the matchers for a
binding of type CXXMeethodDecl, but which was matched with a base
matcher such as functionDecl() we can inform the user that they can
write functionDecl(cxxMethodDecl(isOverride())) etc.

[aaron.ballman]

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

include/clang/ASTMatchers/ASTMatchersInternal.h

198–199

Use of a std::pair here is unfortunate because it's really hard to keep track of what first and second actually mean. A tiny aggregate with two named members would help.

[sbenza]

In D54407#1294934, @aaron.ballman wrote:

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

I also don't see any actual use of this newly stored data so I don't see the goal. Is this for diagnostics on failures or something like that?

[steveire]

In D54407#1295555, @sbenza wrote:

In D54407#1294934, @aaron.ballman wrote:

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

I also don't see any actual use of this newly stored data so I don't see the goal. Is this for diagnostics on failures or something like that?

This commit is part of a series. You can see the use of this in the next commit: https://reviews.llvm.org/D54408

[steveire]

Update

[aaron.ballman]

In D54407#1295555, @sbenza wrote:

In D54407#1294934, @aaron.ballman wrote:

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

Ah, I think I was remembering the issues from binary sizes due to template instantiations. I didn't think this would introduce new instantiations, but I thought it might make them all larger in a harmful way. I don't think that's a concern here, but your second set of eyes doesn't hurt.

include/clang/ASTMatchers/ASTMatchersInternal.h

153

other -> Other

154

This doesn't provide the right ordering guarantees. Use std::tie() instead: return std::tie(DynNode, NodeKind) < std::tie(Other.DynNode, Other.NodeKind);

[steveire]

Update

[aaron.ballman]

Aside from a minor formatting nit, LGTM.

include/clang/ASTMatchers/ASTMatchersInternal.h

154

80 col.

[steveire]

include/clang/ASTMatchers/ASTMatchersInternal.h

154

Thanks, I forgot about tie :).

include/clang/ASTMatchers/ASTMatchersInternal.h

 
 inline const FunctionProtoType *getFunctionProtoType(const FunctionDecl &Node) {
   return Node.getType()->getAs<FunctionProtoType>();
 }
 
 /// Internal version of BoundNodes. Holds all the bound nodes.
 class BoundNodesMap {
 public:
+  struct NodeEntry {
+    ast_type_traits::DynTypedNode DynNode;
+    ast_type_traits::ASTNodeKind NodeKind;
+
+    bool operator<(const NodeEntry &Other) const {
+      return std::tie(DynNode, NodeKind) < std::tie(Other.DynNode, Other.NodeKind);
+    }
+  };
+
   /// Adds \c Node to the map with key \c ID.
   ///
   /// The node's base type should be in NodeBaseType or it will be unaccessible.
-  void addNode(StringRef ID, const ast_type_traits::DynTypedNode& DynNode) {
-    NodeMap[ID] = DynNode;
+  void addNode(StringRef ID, const ast_type_traits::DynTypedNode &DynNode,
+               ast_type_traits::ASTNodeKind NodeKind) {
+    NodeMap[ID] = NodeEntry{DynNode, NodeKind};
   }
 
   /// Returns the AST node bound to \c ID.
   ///
   /// Returns NULL if there was no node bound to \c ID or if there is a node but
   /// it cannot be converted to the specified type.
   template <typename T>
   const T *getNodeAs(StringRef ID) const {
     IDToNodeMap::const_iterator It = NodeMap.find(ID);
     if (It == NodeMap.end()) {
       return nullptr;
     }
-    return It->second.get<T>();
+    return It->second.DynNode.get<T>();
   }
 
   ast_type_traits::DynTypedNode getNode(StringRef ID) const {
     IDToNodeMap::const_iterator It = NodeMap.find(ID);
     if (It == NodeMap.end()) {
       return ast_type_traits::DynTypedNode();
     }
-    return It->second;
+    return It->second.DynNode;
   }
 
   /// Imposes an order on BoundNodesMaps.
   bool operator<(const BoundNodesMap &Other) const {
     return NodeMap < Other.NodeMap;
   }
 
   /// A map from IDs to the bound nodes.
   ///
   /// Note that we're using std::map here, as for memoization:
   /// - we need a comparison operator
   /// - we need an assignment operator
-  using IDToNodeMap = std::map<std::string, ast_type_traits::DynTypedNode>;
+  using IDToNodeMap = std::map<std::string, NodeEntry>;
 
   const IDToNodeMap &getMap() const {
     return NodeMap;
   }
 
   /// Returns \c true if this \c BoundNodesMap can be compared, i.e. all
   /// stored nodes have memoization data.
   bool isComparable() const {
     for (const auto &IDAndNode : NodeMap) {
-      if (!IDAndNode.second.getMemoizationData())
+      if (!IDAndNode.second.DynNode.getMemoizationData())
         return false;
     }
     return true;
   }
 
 private:
   IDToNodeMap NodeMap;
 };
public:
 
     /// Called multiple times during a single call to VisitMatches(...).
     ///
     /// 'BoundNodesView' contains the bound nodes for a single match.
     virtual void visitMatch(const BoundNodes& BoundNodesView) = 0;
   };
 
   /// Add a binding from an id to a node.
-  void setBinding(StringRef Id, const ast_type_traits::DynTypedNode &DynNode) {
+  void setBinding(StringRef Id, const ast_type_traits::DynTypedNode &DynNode,
+                  ast_type_traits::ASTNodeKind NodeKind) {
     if (Bindings.empty())
       Bindings.emplace_back();
     for (BoundNodesMap &Binding : Bindings)
-      Binding.addNode(Id, DynNode);
+      Binding.addNode(Id, DynNode, NodeKind);
   }
 
   /// Adds a branch in the tree.
   void addMatch(const BoundNodesTreeBuilder &Bindings);
 
   /// Visits all matches that this BoundNodesTree represents.
   ///
   /// The ownership of 'ResultVisitor' remains at the caller.
public:
   virtual ~DynMatcherInterface() = default;
 
   /// Returns true if \p DynNode can be matched.
   ///
   /// May bind \p DynNode to an ID via \p Builder, or recurse into
   /// the AST via \p Finder.
   virtual bool dynMatches(const ast_type_traits::DynTypedNode &DynNode,
                           ASTMatchFinder *Finder,
-                          BoundNodesTreeBuilder *Builder) const = 0;
+                          BoundNodesTreeBuilder *Builder,
+                          ast_type_traits::ASTNodeKind NodeKind) const = 0;
 };
 
 /// Generic interface for matchers on an AST node of type T.
 ///
 /// Implement this if your matcher may need to inspect the children or
 /// descendants of the node or bind matched nodes to names. If you are
 /// writing a simple matcher that only inspects properties of the
 /// current node and doesn't care about its children or descendants,
 /// implement SingleNodeMatcherInterface instead.
 template <typename T>
 class MatcherInterface : public DynMatcherInterface {
 public:
   /// Returns true if 'Node' can be matched.
   ///
   /// May bind 'Node' to an ID via 'Builder', or recurse into
   /// the AST via 'Finder'.
   virtual bool matches(const T &Node,
                        ASTMatchFinder *Finder,
                        BoundNodesTreeBuilder *Builder) const = 0;
 
   bool dynMatches(const ast_type_traits::DynTypedNode &DynNode,
-                  ASTMatchFinder *Finder,
-                  BoundNodesTreeBuilder *Builder) const override {
+                  ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder,
+                  ast_type_traits::ASTNodeKind NodeKind) const override {
     return matches(DynNode.getUnchecked<T>(), Finder, Builder);
   }
 };
 
 /// Interface for matchers that only evaluate properties on a single
 /// node.
 template <typename T>
 class SingleNodeMatcherInterface : public MatcherInterface<T> {

lib/ASTMatchers/ASTMatchersInternal.cpp

if (S == SelString)
       return true;
   return false;
 }
 
 namespace internal {
 
 bool NotUnaryOperator(const ast_type_traits::DynTypedNode &DynNode,
                       ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder,
+                      ast_type_traits::ASTNodeKind NodeKind,
                       ArrayRef<DynTypedMatcher> InnerMatchers);
 
 bool AllOfVariadicOperator(const ast_type_traits::DynTypedNode &DynNode,
                            ASTMatchFinder *Finder,
                            BoundNodesTreeBuilder *Builder,
+                           ast_type_traits::ASTNodeKind NodeKind,
                            ArrayRef<DynTypedMatcher> InnerMatchers);
 
 bool EachOfVariadicOperator(const ast_type_traits::DynTypedNode &DynNode,
                             ASTMatchFinder *Finder,
                             BoundNodesTreeBuilder *Builder,
+                            ast_type_traits::ASTNodeKind NodeKind,
                             ArrayRef<DynTypedMatcher> InnerMatchers);
 
 bool AnyOfVariadicOperator(const ast_type_traits::DynTypedNode &DynNode,
                            ASTMatchFinder *Finder,
                            BoundNodesTreeBuilder *Builder,
+                           ast_type_traits::ASTNodeKind NodeKind,
                            ArrayRef<DynTypedMatcher> InnerMatchers);
 
 void BoundNodesTreeBuilder::visitMatches(Visitor *ResultVisitor) {
   if (Bindings.empty())
     Bindings.push_back(BoundNodesMap());
   for (BoundNodesMap &Binding : Bindings) {
     ResultVisitor->visitMatch(BoundNodes(Binding));
   }
 }
 
 namespace {
 
 using VariadicOperatorFunction = bool (*)(
     const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
-    BoundNodesTreeBuilder *Builder, ArrayRef<DynTypedMatcher> InnerMatchers);
+    BoundNodesTreeBuilder *Builder, ast_type_traits::ASTNodeKind NodeKind,
+    ArrayRef<DynTypedMatcher> InnerMatchers);
 
 template <VariadicOperatorFunction Func>
 class VariadicMatcher : public DynMatcherInterface {
 public:
   VariadicMatcher(std::vector<DynTypedMatcher> InnerMatchers)
       : InnerMatchers(std::move(InnerMatchers)) {}
 
   bool dynMatches(const ast_type_traits::DynTypedNode &DynNode,
-                  ASTMatchFinder *Finder,
-                  BoundNodesTreeBuilder *Builder) const override {
-    return Func(DynNode, Finder, Builder, InnerMatchers);
+                  ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder,
+                  ast_type_traits::ASTNodeKind NodeKind) const override {
+    return Func(DynNode, Finder, Builder, NodeKind, InnerMatchers);
   }
 
 private:
   std::vector<DynTypedMatcher> InnerMatchers;
 };
 
 class IdDynMatcher : public DynMatcherInterface {
 public:
   IdDynMatcher(StringRef ID,
                IntrusiveRefCntPtr<DynMatcherInterface> InnerMatcher)
       : ID(ID), InnerMatcher(std::move(InnerMatcher)) {}
 
   bool dynMatches(const ast_type_traits::DynTypedNode &DynNode,
-                  ASTMatchFinder *Finder,
-                  BoundNodesTreeBuilder *Builder) const override {
-    bool Result = InnerMatcher->dynMatches(DynNode, Finder, Builder);
-    if (Result) Builder->setBinding(ID, DynNode);
+                  ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder,
+                  ast_type_traits::ASTNodeKind NodeKind) const override {
+    bool Result = InnerMatcher->dynMatches(DynNode, Finder, Builder, NodeKind);
+    if (Result)
+      Builder->setBinding(ID, DynNode, NodeKind);
     return Result;
   }
 
 private:
   const std::string ID;
   const IntrusiveRefCntPtr<DynMatcherInterface> InnerMatcher;
 };
 
 /// A matcher that always returns true.
 ///
 /// We only ever need one instance of this matcher, so we create a global one
 /// and reuse it to reduce the overhead of the matcher and increase the chance
 /// of cache hits.
 class TrueMatcherImpl : public DynMatcherInterface {
 public:
   TrueMatcherImpl() {
     Retain(); // Reference count will never become zero.
   }
 
   bool dynMatches(const ast_type_traits::DynTypedNode &, ASTMatchFinder *,
-                  BoundNodesTreeBuilder *) const override {
+                  BoundNodesTreeBuilder *,
+                  ast_type_traits::ASTNodeKind) const override {
     return true;
   }
 };
 
 } // namespace
 
 static llvm::ManagedStatic<TrueMatcherImpl> TrueMatcherInstance;
 
Copy.RestrictKind =
       ast_type_traits::ASTNodeKind::getMostDerivedType(Kind, RestrictKind);
   return Copy;
 }
 
 bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
                               ASTMatchFinder *Finder,
                               BoundNodesTreeBuilder *Builder) const {
   if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
-      Implementation->dynMatches(DynNode, Finder, Builder)) {
+      Implementation->dynMatches(DynNode, Finder, Builder, RestrictKind)) {
     return true;
   }
   // Delete all bindings when a matcher does not match.
   // This prevents unexpected exposure of bound nodes in unmatches
   // branches of the match tree.
   Builder->removeBindings([](const BoundNodesMap &) { return true; });
   return false;
 }
 
 bool DynTypedMatcher::matchesNoKindCheck(
     const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
     BoundNodesTreeBuilder *Builder) const {
   assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
-  if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+  if (Implementation->dynMatches(DynNode, Finder, Builder, RestrictKind)) {
     return true;
   }
   // Delete all bindings when a matcher does not match.
   // This prevents unexpected exposure of bound nodes in unmatches
   // branches of the match tree.
   Builder->removeBindings([](const BoundNodesMap &) { return true; });
   return false;
 }

 }
 
 void BoundNodesTreeBuilder::addMatch(const BoundNodesTreeBuilder &Other) {
   Bindings.append(Other.Bindings.begin(), Other.Bindings.end());
 }
 
 bool NotUnaryOperator(const ast_type_traits::DynTypedNode &DynNode,
                       ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder,
+                      ast_type_traits::ASTNodeKind NodeKind,
                       ArrayRef<DynTypedMatcher> InnerMatchers) {
   if (InnerMatchers.size() != 1)
     return false;
 
   // The 'unless' matcher will always discard the result:
   // If the inner matcher doesn't match, unless returns true,
   // but the inner matcher cannot have bound anything.
   // If the inner matcher matches, the result is false, and
   // any possible binding will be discarded.
   // We still need to hand in all the bound nodes up to this
   // point so the inner matcher can depend on bound nodes,
   // and we need to actively discard the bound nodes, otherwise
   // the inner matcher will reset the bound nodes if it doesn't
   // match, but this would be inversed by 'unless'.
   BoundNodesTreeBuilder Discard(*Builder);
   return !InnerMatchers[0].matches(DynNode, Finder, &Discard);
 }
 
 bool AllOfVariadicOperator(const ast_type_traits::DynTypedNode &DynNode,
                            ASTMatchFinder *Finder,
                            BoundNodesTreeBuilder *Builder,
+                           ast_type_traits::ASTNodeKind NodeKind,
                            ArrayRef<DynTypedMatcher> InnerMatchers) {
   // allOf leads to one matcher for each alternative in the first
   // matcher combined with each alternative in the second matcher.
   // Thus, we can reuse the same Builder.
   for (const DynTypedMatcher &InnerMatcher : InnerMatchers) {
     if (!InnerMatcher.matchesNoKindCheck(DynNode, Finder, Builder))
       return false;
   }
   return true;
 }
 
 bool EachOfVariadicOperator(const ast_type_traits::DynTypedNode &DynNode,
                             ASTMatchFinder *Finder,
                             BoundNodesTreeBuilder *Builder,
+                            ast_type_traits::ASTNodeKind NodeKind,
                             ArrayRef<DynTypedMatcher> InnerMatchers) {
   BoundNodesTreeBuilder Result;
   bool Matched = false;
   for (const DynTypedMatcher &InnerMatcher : InnerMatchers) {
     BoundNodesTreeBuilder BuilderInner(*Builder);
     if (InnerMatcher.matches(DynNode, Finder, &BuilderInner)) {
       Matched = true;
       Result.addMatch(BuilderInner);
     }
   }
   *Builder = std::move(Result);
   return Matched;
 }
 
 bool AnyOfVariadicOperator(const ast_type_traits::DynTypedNode &DynNode,
                            ASTMatchFinder *Finder,
                            BoundNodesTreeBuilder *Builder,
+                           ast_type_traits::ASTNodeKind NodeKind,
                            ArrayRef<DynTypedMatcher> InnerMatchers) {
   for (const DynTypedMatcher &InnerMatcher : InnerMatchers) {
     BoundNodesTreeBuilder Result = *Builder;
     if (InnerMatcher.matches(DynNode, Finder, &Result)) {
       *Builder = std::move(Result);
       return true;
     }
   }

lib/Tooling/RefactoringCallbacks.cpp

case TemplateElement::Literal:
       break;
     case TemplateElement::Identifier: {
       auto NodeIter = NodeMap.find(Element.Value);
       if (NodeIter == NodeMap.end()) {
         llvm::errs() << "Node " << Element.Value
                      << " used in replacement template not bound in Matcher \n";
         llvm::report_fatal_error("Unbound node in replacement template.");
       }
-      CharSourceRange Source =
-          CharSourceRange::getTokenRange(NodeIter->second.getSourceRange());
+      CharSourceRange Source = CharSourceRange::getTokenRange(
+          NodeIter->second.DynNode.getSourceRange());
       ToText += Lexer::getSourceText(Source, *Result.SourceManager,
                                      Result.Context->getLangOpts());
       break;
     }
     }
   }
   if (NodeMap.count(FromId) == 0) {
     llvm::errs() << "Node to be replaced " << FromId
                  << " not bound in query.\n";
     llvm::report_fatal_error("FromId node not bound in MatchResult");
   }
   auto Replacement =
-      tooling::Replacement(*Result.SourceManager, &NodeMap.at(FromId), ToText,
-                           Result.Context->getLangOpts());
+      tooling::Replacement(*Result.SourceManager, &NodeMap.at(FromId).DynNode,
+                           ToText, Result.Context->getLangOpts());
   llvm::Error Err = Replace.add(Replacement);
   if (Err) {
     llvm::errs() << "Query and replace failed in " << Replacement.getFilePath()
                  << "! " << llvm::toString(std::move(Err)) << "\n";
     llvm::report_fatal_error("Replacement failed");
   }
 }
 
 } // end namespace tooling
 } // end namespace clang

unittests/ASTMatchers/ASTMatchersNodeTest.cpp

TEST(ObjCAutoreleaseMatcher, AutoreleasePool) {
   EXPECT_FALSE(matchesObjC(ObjCStringNoPool, autoreleasePoolStmt()));
 }
 
 TEST(MatchFinderAPI, matchesDynamic) {
 
   std::string SourceCode = "struct A { void f() {} };";
   auto Matcher = functionDecl(isDefinition()).bind("method");
 
+  using namespace ast_type_traits;
+
   auto astUnit = tooling::buildASTFromCode(SourceCode);
 
   auto GlobalBoundNodes = matchDynamic(Matcher, astUnit->getASTContext());
 
   EXPECT_EQ(GlobalBoundNodes.size(), 1u);
   EXPECT_EQ(GlobalBoundNodes[0].getMap().size(), 1u);
+  auto GlobalMapPair = *GlobalBoundNodes[0].getMap().begin();
+  EXPECT_TRUE(GlobalMapPair.second.DynNode.getNodeKind().isSame(
+      ASTNodeKind::getFromNodeKind<CXXMethodDecl>()));
+  EXPECT_TRUE(GlobalMapPair.second.NodeKind.isSame(
+      ASTNodeKind::getFromNodeKind<FunctionDecl>()));
 
   auto GlobalMethodNode = GlobalBoundNodes[0].getNodeAs<FunctionDecl>("method");
   EXPECT_TRUE(GlobalMethodNode != nullptr);
 
   auto MethodBoundNodes =
       matchDynamic(Matcher, *GlobalMethodNode, astUnit->getASTContext());
   EXPECT_EQ(MethodBoundNodes.size(), 1u);
   EXPECT_EQ(MethodBoundNodes[0].getMap().size(), 1u);
+  auto MethodMapPair = *MethodBoundNodes[0].getMap().begin();
+  EXPECT_TRUE(MethodMapPair.second.DynNode.getNodeKind().isSame(
+      ASTNodeKind::getFromNodeKind<CXXMethodDecl>()));
+  EXPECT_TRUE(MethodMapPair.second.NodeKind.isSame(
+      ASTNodeKind::getFromNodeKind<FunctionDecl>()));
+  EXPECT_EQ(MethodMapPair.second.DynNode, GlobalMapPair.second.DynNode);
+  EXPECT_TRUE(
+      MethodMapPair.second.NodeKind.isSame(GlobalMapPair.second.NodeKind));
 
   auto MethodNode = MethodBoundNodes[0].getNodeAs<FunctionDecl>("method");
   EXPECT_EQ(MethodNode, GlobalMethodNode);
 }
 
 } // namespace ast_matchers
 } // namespace clang

unittests/ASTMatchers/ASTMatchersTest.h

if (Nodes->getNodeAs<T>(Id)) {
       } else if (const NestedNameSpecifier *NNS =
         Nodes->getNodeAs<NestedNameSpecifier>(Id)) {
         llvm::raw_string_ostream OS(Name);
         NNS->print(OS, PrintingPolicy(LangOptions()));
       }
       BoundNodes::IDToNodeMap::const_iterator I = M.find(Id);
       EXPECT_NE(M.end(), I);
       if (I != M.end()) {
-        EXPECT_EQ(Nodes->getNodeAs<T>(Id), I->second.get<T>());
+        EXPECT_EQ(Nodes->getNodeAs<T>(Id), I->second.DynNode.get<T>());
       }
       return true;
     }
     EXPECT_TRUE(M.count(Id) == 0 ||
-      M.find(Id)->second.template get<T>() == nullptr);
+                M.find(Id)->second.DynNode.get<T>() == nullptr);
     return false;
   }
 
   bool run(const BoundNodes *Nodes, ASTContext *Context) override {
     return run(Nodes);
   }
 
 private: