[AST Matchers] Fix bug in 'optionally' matcher wherein all previous bindings are...

Summary: The implementation of 'optionally' doesn't preserve bindings when none of the submatchers succeed. This patch adds a regression test for that behavior and fixes it.

Reviewers: aaron.ballman, sbenza

Subscribers: cfe-commits

Tags: #clang

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

diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 777400f..6f4132c 100644 (file)
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -347,12 +347,18 @@ bool OptionallyVariadicOperator(const DynTypedNode &DynNode,
                                 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;
 }
 

diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
index 65fb11c..edfd1c3 100644 (file)
--- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2012,6 +2012,19 @@ TEST(Optionally, SubmatchersDoNotMatch) {
       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; };",