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

[Loïc Joly via Phabricator via cfe-commits]

loic-joly-sonarsource created this revision.
loic-joly-sonarsource added a reviewer: klimek.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80025

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


Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===================================================================
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2591,6 +2591,14 @@
     compoundStmt(hasParent(ifStmt()))));
 }
 
+TEST(MatcherMemoize, HasParentDiffersFromHas) {
+  // Test introduced after detecting a bug in memoization
+    EXPECT_TRUE(matches(
+      "void f() { throw 1; }",
+      expr(eachOf(cxxThrowExpr(hasParent(expr())),
+                  cxxThrowExpr(has(expr()))))));
+}
+
 TEST(HasAncestor, MatchesAllAncestors) {
   EXPECT_TRUE(matches(
     "template <typename T> struct C { static void f() { 42; } };"
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===================================================================
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -43,6 +43,11 @@
 // optimize this on.
 static const unsigned MaxMemoizationEntries = 10000;
 
+enum class MatchDirection {
+  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,11 +65,12 @@
   ast_type_traits::DynTypedNode Node;
   BoundNodesTreeBuilder BoundNodes;
   ast_type_traits::TraversalKind Traversal = ast_type_traits::TK_AsIs;
+  MatchDirection Direction;
 
   bool operator<(const MatchKey &Other) const {
-    return std::tie(MatcherID, Node, BoundNodes, Traversal) <
+    return std::tie(MatcherID, Node, BoundNodes, Traversal, Direction) <
            std::tie(Other.MatcherID, Other.Node, Other.BoundNodes,
-                    Other.Traversal);
+                    Other.Traversal, Other.Direction);
   }
 };
 
@@ -457,6 +463,7 @@
     // Note that we key on the bindings *before* the match.
     Key.BoundNodes = *Builder;
     Key.Traversal = Ctx.getTraversalKind();
+    Key.Direction = MatchDirection::Descendants;
 
     MemoizationMap::iterator I = ResultCache.find(Key);
     if (I != ResultCache.end()) {
@@ -706,6 +713,7 @@
     Key.Node = Node;
     Key.BoundNodes = *Builder;
     Key.Traversal = Ctx.getTraversalKind();
+    Key.Direction = MatchDirection::Ancestors;
 
     // Note that we cannot use insert and reuse the iterator, as recursive
     // calls to match might invalidate the result cache iterators.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80025.264293.patch
Type: text/x-patch
Size: 2468 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200515/cc30f083/attachment-0001.bin>