[AST Matchers] Restrict `optionally` matcher to a single argument.

[ymandel]

Summary

Currently, optionally can take multiple arguments, which commits it to a
particular strategy for those arguments (in this case, "for each"). We limit the
matcher to a single argument, which avoids any potential confusion and
simplifies the implementation. The user can retrieve multiple-argument
optionality, by explicitly using the desired operator (like forEach, anyOf,
allOf, etc.) with all children wrapped in optionally.

[ymandel]

Updated comments for matcher.

[ymandel]

Updated the matcher HTML doc.

[aaron.ballman]

LGTM, thank you!

clang/docs/LibASTMatchersReference.html

 Given
 void f() { for (;;) { int x = 42; if (true) { int x = 43; } } }
 compoundStmt(hasParent(ifStmt())) matches "{ int x = 43; }".
 
 Usable as: Any Matcher
 </pre></td></tr>
 
 
-<tr><td>Matcher&lt;*&gt;</td><td class="name" onclick="toggle('optionally0')"><a name="optionally0Anchor">optionally</a></td><td>Matcher&lt;*&gt;, ..., Matcher&lt;*&gt;</td></tr>
-<tr><td colspan="4" class="doc" id="optionally0"><pre>Matches any node regardless of the submatchers.
+<tr><td>Matcher&lt;*&gt;</td><td class="name" onclick="toggle('optionally0')"><a name="optionally0Anchor">optionally</a></td><td>Matcher&lt;*&gt;</td></tr>
+<tr><td colspan="4" class="doc" id="optionally0"><pre>Matches any node regardless of the submatcher.
 
-However, optionally will generate a result binding for each matching
-submatcher.
-
-Useful when additional information which may or may not present about a
-main matching node is desired.
+However, optionally will retain any bindings generated by the submatcher.
+Useful when additional information which may or may not present about a main
+matching node is desired.
 
 For example, in:
   class Foo {
     int bar;
   }
 The matcher:
   cxxRecordDecl(
     optionally(has(

clang/include/clang/ASTMatchers/ASTMatchers.h

 
 /// Matches if all given matchers match.
 ///
 /// Usable as: Any Matcher
 extern const internal::VariadicOperatorMatcherFunc<
     2, std::numeric_limits<unsigned>::max()>
     allOf;
 
-/// Matches any node regardless of the submatchers.
+/// Matches any node regardless of the submatcher.
 ///
-/// However, \c optionally will generate a result binding for each matching
-/// submatcher.
-///
-/// Useful when additional information which may or may not present about a
-/// main matching node is desired.
+/// However, \c optionally will retain any bindings generated by the submatcher.
+/// Useful when additional information which may or may not present about a main
+/// matching node is desired.
 ///
 /// For example, in:
 /// \code
 ///   class Foo {
 ///     int bar;
 ///   }
 /// \endcode
 /// The matcher:
 /// \code
 ///   cxxRecordDecl(
 ///     optionally(has(
 ///       fieldDecl(hasName("bar")).bind("var")
 ///   ))).bind("record")
 /// \endcode
 /// will produce a result binding for both "record" and "var".
 /// The matcher will produce a "record" binding for even if there is no data
 /// member named "bar" in that class.
 ///
 /// Usable as: Any Matcher
-extern const internal::VariadicOperatorMatcherFunc<
-    1, std::numeric_limits<unsigned>::max()>
-    optionally;
+extern const internal::VariadicOperatorMatcherFunc<1, 1> optionally;
 
 /// Matches sizeof (C99), alignof (C++11) and vec_step (OpenCL)
 ///
 /// Given
 /// \code
 ///   Foo x = bar;
 ///   int y = sizeof(x) + alignof(x);
 /// \endcode

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)) {
-      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)
+  if (InnerMatchers.size() != 1)
+    return false;
+
+  BoundNodesTreeBuilder Result(*Builder);
+  if (InnerMatchers[0].matches(DynNode, Finder, &Result))
     *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());
const internal::VariadicOperatorMatcherFunc<
     2, std::numeric_limits<unsigned>::max()>
     eachOf = {internal::DynTypedMatcher::VO_EachOf};
 const internal::VariadicOperatorMatcherFunc<
     2, std::numeric_limits<unsigned>::max()>
     anyOf = {internal::DynTypedMatcher::VO_AnyOf};
 const internal::VariadicOperatorMatcherFunc<
     2, std::numeric_limits<unsigned>::max()>
     allOf = {internal::DynTypedMatcher::VO_AllOf};
-const internal::VariadicOperatorMatcherFunc<
-    1, std::numeric_limits<unsigned>::max()>
-    optionally = {internal::DynTypedMatcher::VO_Optionally};
+const internal::VariadicOperatorMatcherFunc<1, 1> optionally = {
+    internal::DynTypedMatcher::VO_Optionally};
 const internal::VariadicFunction<internal::Matcher<NamedDecl>, StringRef,
                                  internal::hasAnyNameFunc>
     hasAnyName = {};
 
 const internal::VariadicFunction<internal::HasOpNameMatcher, StringRef,
                                  internal::hasAnyOperatorNameFunc>
     hasAnyOperatorName = {};
 const internal::VariadicFunction<internal::Matcher<ObjCMessageExpr>, StringRef,

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

EXPECT_TRUE(notMatches(
     "class A { int c; int d; };",
     recordDecl(eachOf(has(fieldDecl(hasName("a")).bind("v")),
                       has(fieldDecl(hasName("b")).bind("v"))))));
 }
 
 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")));
+      recordDecl(optionally(has(fieldDecl(hasName("c")).bind("c")))),
+      std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("c")));
 }
 
 // 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; };",
-      recordDecl(optionally(has(fieldDecl(hasName("c")).bind("v")),
-                            has(fieldDecl(hasName("b")).bind("v")))),
-      std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v", 2)));
+      recordDecl(optionally(has(fieldDecl(hasName("a")).bind("v")))),
+      std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v")));
 }
 
 TEST(IsTemplateInstantiation, MatchesImplicitClassTemplateInstantiation) {
   // Make sure that we can both match the class by name (::X) and by the type
   // the template was instantiated with (via a field).
 
   EXPECT_TRUE(matches(
     "template <typename T> class X {}; class A {}; X<A> x;",