[clang] cba56e0 - [ASTMatcher] Correct memoization bug ignoring direction (descendants or ancestors)

[Sam McCall via cfe-commits]

Author: Loïc Joly
Date: 2020-06-22T12:56:29+02:00
New Revision: cba56e026c7beb91a2716276151c5b4360032834

URL: https://github.com/llvm/llvm-project/commit/cba56e026c7beb91a2716276151c5b4360032834
DIFF: https://github.com/llvm/llvm-project/commit/cba56e026c7beb91a2716276151c5b4360032834.diff

LOG: [ASTMatcher] Correct memoization bug ignoring direction (descendants or ancestors)

Summary:
In ASTMatcher, when we have `has(...)` and `hasParent(...)` called with the same internal matcher on the same node, the memoization process will mix-up the two calls because the direction of the traversal is not part of the memoization key.

This patch adds this information.

Reviewers: klimek

Reviewed By: klimek

Subscribers: Godin, njames93, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D80025

Added: 
    

Modified: 
    clang/lib/ASTMatchers/ASTMatchFinder.cpp
    clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 1b49067f0f9d..b4189069db83 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -43,6 +43,11 @@ typedef MatchFinder::MatchCallback MatchCallback;
 // optimize this on.
 static const unsigned MaxMemoizationEntries = 10000;
 
+enum class MatchType {
+  Ancestors,
+  Descendants
+};
+
 // We use memoization to avoid running the same matcher on the same
 // AST node twice.  This struct is the key for looking up match
 // result.  It consists of an ID of the MatcherInterface (for
@@ -60,10 +65,11 @@ struct MatchKey {
   DynTypedNode Node;
   BoundNodesTreeBuilder BoundNodes;
   TraversalKind Traversal = TK_AsIs;
+  MatchType Type;
 
   bool operator<(const MatchKey &Other) const {
-    return std::tie(Traversal, MatcherID, Node, BoundNodes) <
-           std::tie(Other.Traversal, Other.MatcherID, Other.Node,
+    return std::tie(Traversal, Type, MatcherID, Node, BoundNodes) <
+           std::tie(Other.Traversal, Other.Type, Other.MatcherID, Other.Node,
                     Other.BoundNodes);
   }
 };
@@ -446,7 +452,8 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
                                   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())
+    // When doing a single-level match, we don't need to memoize
+    if (!Node.getMemoizationData() || !Builder->isComparable() || MaxDepth == 1)
       return matchesRecursively(Node, Matcher, Builder, MaxDepth, Traversal,
                                 Bind);
 
@@ -456,7 +463,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
     // Note that we key on the bindings *before* the match.
     Key.BoundNodes = *Builder;
     Key.Traversal = Ctx.getParentMapContext().getTraversalKind();
-
+    Key.Type = MatchType::Descendants;
     MemoizationMap::iterator I = ResultCache.find(Key);
     if (I != ResultCache.end()) {
       *Builder = I->second.Nodes;
@@ -693,7 +700,8 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
                                             BoundNodesTreeBuilder *Builder,
                                             AncestorMatchMode MatchMode) {
     // For AST-nodes that don't have an identity, we can't memoize.
-    if (!Builder->isComparable())
+    // When doing a single-level match, we don't need to memoize
+    if (!Builder->isComparable() || MatchMode == AncestorMatchMode::AMM_ParentOnly)
       return matchesAncestorOfRecursively(Node, Ctx, Matcher, Builder,
                                           MatchMode);
 
@@ -702,6 +710,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
     Key.Node = Node;
     Key.BoundNodes = *Builder;
     Key.Traversal = Ctx.getParentMapContext().getTraversalKind();
+    Key.Type = MatchType::Ancestors;
 
     // Note that we cannot use insert and reuse the iterator, as recursive
     // calls to match might invalidate the result cache iterators.

diff  --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 50d101167ff2..380538697f46 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2864,6 +2864,34 @@ TEST(HasParent, MatchesOnlyParent) {
     compoundStmt(hasParent(ifStmt()))));
 }
 
+TEST(MatcherMemoize, HasParentDiffersFromHas) {
+  // Test introduced after detecting a bug in memoization
+  constexpr auto code = "void f() { throw 1; }";
+  EXPECT_TRUE(notMatches(
+    code,
+    cxxThrowExpr(hasParent(expr()))));
+  EXPECT_TRUE(matches(
+    code,
+    cxxThrowExpr(has(expr()))));
+  EXPECT_TRUE(matches(
+    code,
+    cxxThrowExpr(anyOf(hasParent(expr()), has(expr())))));
+}
+
+TEST(MatcherMemoize, HasDiffersFromHasDescendant) {
+  // Test introduced after detecting a bug in memoization
+  constexpr auto code = "void f() { throw 1+1; }";
+  EXPECT_TRUE(notMatches(
+    code,
+    cxxThrowExpr(has(integerLiteral()))));
+  EXPECT_TRUE(matches(
+    code,
+    cxxThrowExpr(hasDescendant(integerLiteral()))));
+  EXPECT_TRUE(notMatches(code, 
+    cxxThrowExpr(allOf(
+      hasDescendant(integerLiteral()),
+      has(integerLiteral())))));
+}
 TEST(HasAncestor, MatchesAllAncestors) {
   EXPECT_TRUE(matches(
     "template <typename T> struct C { static void f() { 42; } };"