[PATCH] D75365: [AST Matchers] Fix bug in 'optionally' matcher wherein all previous bindings are cleared when all inner matchers fail.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 247299.
ymandel added a comment.

removed change to unrelated file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75365

Files:
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp


Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===================================================================
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2012,6 +2012,19 @@
       std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v")));
 }
 
+// Regression test.
+TEST(Optionally, SubmatchersDoNotMatchButPreserveBindings) {
+  std::string Code = "class A { int a; int b; };";
+  auto Matcher = recordDecl(decl().bind("decl"),
+                            
optionally(has(fieldDecl(hasName("c")).bind("v"))));
+  // "decl" is still bound.
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code, Matcher, std::make_unique<VerifyIdIsBoundTo<RecordDecl>>("decl")));
+  // "v" is not bound, but the match still suceeded.
+  EXPECT_TRUE(matchAndVerifyResultFalse(
+      Code, Matcher, std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v")));
+}
+
 TEST(Optionally, SubmatchersMatch) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
       "class A { int a; int c; };",
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===================================================================
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -347,12 +347,18 @@
                                 BoundNodesTreeBuilder *Builder,
                                 ArrayRef<DynTypedMatcher> InnerMatchers) {
   BoundNodesTreeBuilder Result;
+  bool Matched = false;
   for (const DynTypedMatcher &InnerMatcher : InnerMatchers) {
     BoundNodesTreeBuilder BuilderInner(*Builder);
-    if (InnerMatcher.matches(DynNode, Finder, &BuilderInner))
+    if (InnerMatcher.matches(DynNode, Finder, &BuilderInner)){
+      Matched = true;
       Result.addMatch(BuilderInner);
+    }
   }
-  *Builder = std::move(Result);
+  // If there were no matches, we can't assign to `*Builder`; we'd 
(incorrectly)
+  // clear it because `Result` is empty.
+  if (Matched)
+    *Builder = std::move(Result);
   return true;
 }
 



Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===================================================================
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2012,6 +2012,19 @@
       std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v")));
 }
 
+// Regression test.
+TEST(Optionally, SubmatchersDoNotMatchButPreserveBindings) {
+  std::string Code = "class A { int a; int b; };";
+  auto Matcher = recordDecl(decl().bind("decl"),
+                            optionally(has(fieldDecl(hasName("c")).bind("v"))));
+  // "decl" is still bound.
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code, Matcher, std::make_unique<VerifyIdIsBoundTo<RecordDecl>>("decl")));
+  // "v" is not bound, but the match still suceeded.
+  EXPECT_TRUE(matchAndVerifyResultFalse(
+      Code, Matcher, std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v")));
+}
+
 TEST(Optionally, SubmatchersMatch) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
       "class A { int a; int c; };",
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===================================================================
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -347,12 +347,18 @@
                                 BoundNodesTreeBuilder *Builder,
                                 ArrayRef<DynTypedMatcher> InnerMatchers) {
   BoundNodesTreeBuilder Result;
+  bool Matched = false;
   for (const DynTypedMatcher &InnerMatcher : InnerMatchers) {
     BoundNodesTreeBuilder BuilderInner(*Builder);
-    if (InnerMatcher.matches(DynNode, Finder, &BuilderInner))
+    if (InnerMatcher.matches(DynNode, Finder, &BuilderInner)){
+      Matched = true;
       Result.addMatch(BuilderInner);
+    }
   }
-  *Builder = std::move(Result);
+  // If there were no matches, we can't assign to `*Builder`; we'd (incorrectly)
+  // clear it because `Result` is empty.
+  if (Matched)
+    *Builder = std::move(Result);
   return true;
 }
 

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits