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

[ymandel]

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.

[ymandel]

removed change to unrelated file.

[ymandel]

Aaron -- when fixing this bug, I had some other questions about the behavior of this matcher. Specifically, instead of adding bindings to the existing map, it creates a new match result for each matching node (with addMatch). But, I'm struggling to see how it makes sense for optionally -- why would you want a separate match result for each matching InnerMatch? This behavior seems best reserved for forEach and related matchers.

I'd propose (and have a patch ready) to instead accumulate all bindings in the same result, so that optionally(a, b) would be equivalent to allOf(optionally(a), optionally(b)) (which it currently is not). That is, 'optionally' would always give a single result and each submatcher can add more bindings to the result.

Alternatively, we could restrict optionally to take exactly one argument, rather than making it variadic, in which case there's no room for confusion as to its semantics. Multiple optional elements would require using optionally within an allof (either explicit or implicit). However, I realize this change is more ambitious and may prevent some valid uses.

WDYT?

[aaron.ballman]

Patch LGTM aside from a formatting nit.

In D75365#1898466, @ymandel wrote:

Aaron -- when fixing this bug, I had some other questions about the behavior of this matcher. Specifically, instead of adding bindings to the existing map, it creates a new match result for each matching node (with addMatch). But, I'm struggling to see how it makes sense for optionally -- why would you want a separate match result for each matching InnerMatch? This behavior seems best reserved for forEach and related matchers.

Agreed, that does seem a bit strange.

I'd propose (and have a patch ready) to instead accumulate all bindings in the same result, so that optionally(a, b) would be equivalent to allOf(optionally(a), optionally(b)) (which it currently is not). That is, 'optionally' would always give a single result and each submatcher can add more bindings to the result.

Alternatively, we could restrict optionally to take exactly one argument, rather than making it variadic, in which case there's no room for confusion as to its semantics. Multiple optional elements would require using optionally within an allof (either explicit or implicit). However, I realize this change is more ambitious and may prevent some valid uses.

WDYT?

Good question! My intuition is that optionally should take exactly one argument and the user should be explicit as to whether they mean allOf or anyOf when there is a list of optional matchers. Defaulting to the allOf behavior for a list may be surprising to some folks because the anyOf behavior also seems reasonable for a list of optional matchers. WDYT?

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

353–354

Please fix the formatting.

[ymandel]

fix formatting.

[ymandel]

In D75365#1898536, @aaron.ballman wrote:

...

Good question! My intuition is that optionally should take exactly one argument and the user should be explicit as to whether they mean allOf or anyOf when there is a list of optional matchers. Defaulting to the allOf behavior for a list may be surprising to some folks because the anyOf behavior also seems reasonable for a list of optional matchers. WDYT?

Sounds good to me. That's my ideal solution. Will update my patch and send along... Thanks!

[gribozavr2]

+1 to this fix.

However, regarding allOf vs. anyOf semantics, since optionally always succeeds, is there a difference between the two semantics?

It seems to me that there should be no difference between allOf(optionally(a), optionally(b)) vs. anyOf(optionally(a), optionally(b)).

[ymandel]

In D75365#1900784, @gribozavr2 wrote:

+1 to this fix.

However, regarding allOf vs. anyOf semantics, since optionally always succeeds, is there a difference between the two semantics?

It seems to me that there should be no difference between allOf(optionally(a), optionally(b)) vs. anyOf(optionally(a), optionally(b)).

I think the difference is in whether you continue with the submatchers after a success. Allof does while anyof does not. That said, the original issue was forEach vs allOf/anyOf. So, I think Aaron's point holds - let optionally take one argument and then leave it to the user to explicitly specify forEach, allOf, anyOf, etc.

[gribozavr2]

I think the difference is in whether you continue with the submatchers after a success. Allof does while anyof does not.

Oh, the short-circuiting makes a difference! I see.

+1 to making it non-variadic then.

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

bool AnyOfVariadicOperator(const DynTypedNode &DynNode, ASTMatchFinder *Finder,
   return false;
 }
 
 bool OptionallyVariadicOperator(const DynTypedNode &DynNode,
                                 ASTMatchFinder *Finder,
                                 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);
     }
+  }
+  // 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;
 }
 
 inline static
 std::vector<std::string> vectorFromRefs(ArrayRef<const StringRef *> NameRefs) {
   std::vector<std::string> Names;
   Names.reserve(NameRefs.size());
   for (auto *Name : NameRefs)

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

 TEST(Optionally, SubmatchersDoNotMatch) {
   EXPECT_TRUE(matchAndVerifyResultFalse(
       "class A { int a; int b; };",
       recordDecl(optionally(has(fieldDecl(hasName("c")).bind("v")),
                             has(fieldDecl(hasName("d")).bind("v")))),
       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; };",
       recordDecl(optionally(has(fieldDecl(hasName("a")).bind("v")),
                             has(fieldDecl(hasName("b")).bind("v")))),
       std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v", 1)));
   EXPECT_TRUE(matchAndVerifyResultTrue(
       "class A { int c; int b; };",