[ASTMatchers] Make it possible to use empty variadic matchers

[steveire]

Summary

When debugging a matcher it is convenient to be able to comment out
nested matchers experimentally. This stops working when we have less
than two matchers.

Additionally, the removal of the contraint makes it possible to use
these matchers in generic code.

[aaron.ballman]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

402

Does it make sense to return true when there are no inner matchers? I would have thought that that if there are no matchers, nothing would match (so we'd return false)?

[steveire]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

402

When debugging a matcher like

callExpr(anyOf(
    hasType(pointerType()),
    callee(functionDecl(hasName("foo")))
    ))

and commenting out inner matchers to get

callExpr(anyOf(
#    hasType(pointerType()),
#    callee(functionDecl(hasName("foo")))
    ))

it would be very surprising for this to not match callExprs anymore.

[aaron.ballman]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

402

On the one hand, I see what you're saying. On the other hand, I think the behavior actually is surprising to some folks (like me). For instance, std::any_of() returns false when the range is empty, while std::all_of() returns true.

To be conservative with the change, rather than allowing zero matchers with potentially surprising behavior, we could require there be at least one matcher. In that case, if you want to comment out all of the inner matchers, you need to comment out the variadic one as well. e.g.,

# This is an error
callExpr(anyOf(
#    hasType(pointerType()),
#    callee(functionDecl(hasName("foo")))
    ))

# Do this instead
callExpr(
# anyOf(
#    hasType(pointerType()),
#    callee(functionDecl(hasName("foo")))
# )
)

It's a bit more commenting for the person experimenting, but it's also reduces the chances for surprising behavior. WDYT?

[steveire]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

402

On the one hand, I see what you're saying. On the other hand, I think the behavior actually is surprising to some folks (like me). For instance, std::any_of() returns false when the range is empty, while std::all_of() returns true.

Yes, I know this diverges from those. However, I think the semantic in this patch is the semantic that makes sense for AST Matchers. This patch prioritizes usefulness and consistency in the context of writing and debugging AST Matchers instead of prioritizing consistency with std::any_of (which would be non-useful and surprising in the context of debugging an AST Matcher).

if you want to comment out all of the inner matchers, you need to comment out the variadic one as well

Yes, that's what I'm trying to make unnecessary.

It's a bit more commenting for the person experimenting, but it's also reduces the chances for surprising behavior. WDYT?

I think your suggestion leaves zero-arg anyOf matcher inconsistent with allOf matcher (fails to compile versus does something useful).

[aaron.ballman]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

402

Yes, I know this diverges from those. However, I think the semantic in this patch is the semantic that makes sense for AST Matchers. This patch prioritizes usefulness and consistency in the context of writing and debugging AST Matchers instead of prioritizing consistency with std::any_of (which would be non-useful and surprising in the context of debugging an AST Matcher).

I'm not suggesting that it needs to be consistent for the sake of consistency, I'm saying that the behavior you are proposing for the any-of-like matchers (anyOf and eachOf) is confusing to me in the presence of no arguments *and* is inconsistent with other APIs that do set inclusion operations.

I think your suggestion leaves zero-arg anyOf matcher inconsistent with allOf matcher (fails to compile versus does something useful).

Then my preference is for the any-of-like matchers to return false when given zero arguments while the all-of-like matchers return true. Testing for existence requires at least one element while testing for universality does not.

[steveire]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

402

Then my preference is for the any-of-like matchers to return false when given zero arguments while the all-of-like matchers return true. Testing for existence requires at least one element while testing for universality does not.

That doesn't achieve the goal I have for this patch.

If that's a blocker, then the only remaining possibility is a different name for a matcher which returns true on empty and otherwise behaves like anyOf. I doubt a good name for such a thing exists.

So, I'll abandon it for now.

[aaron.ballman]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

402

If that's a blocker, then the only remaining possibility is a different name for a matcher which returns true on empty and otherwise behaves like anyOf. I doubt a good name for such a thing exists.

I don't think a parallel API would be a good approach. However, I'm happy to add a few more reviewers to the review to see if more opinions reach a different conclusion about your proposed changes. If you'd like to go that route, I'd suggest you add klimek, alexfh, and sammccall when you reopen the review, and maybe dblaikie and echristo for some variety outside of the usual AST matcher folks.

FWIW, I found some sources to back my belief that checking existence needs at least one element (https://en.wikipedia.org/wiki/Existential_quantification#The_empty_set) while universality does not (https://en.wikipedia.org/wiki/Universal_quantification#The_empty_set).

clang/include/clang/ASTMatchers/ASTMatchers.h

 ///                        has(fieldDecl(hasName("b")).bind("v"))))
 /// \endcode
 /// will generate two results binding "v", the first of which binds
 /// the field declaration of \c a, the second the field declaration of
 /// \c b.
 ///
 /// Usable as: Any Matcher
 extern const internal::VariadicOperatorMatcherFunc<
-    2, std::numeric_limits<unsigned>::max()>
+    0, std::numeric_limits<unsigned>::max()>
     eachOf;
 
 /// Matches if any of the given matchers matches.
 ///
 /// Usable as: Any Matcher
 extern const internal::VariadicOperatorMatcherFunc<
-    2, std::numeric_limits<unsigned>::max()>
+    0, std::numeric_limits<unsigned>::max()>
     anyOf;
 
 /// Matches if all given matchers match.
 ///
 /// Usable as: Any Matcher
 extern const internal::VariadicOperatorMatcherFunc<
-    2, std::numeric_limits<unsigned>::max()>
+    0, std::numeric_limits<unsigned>::max()>
     allOf;
 
 /// Matches any node regardless of the submatcher.
 ///
 /// 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.
 ///

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

return llvm::all_of(InnerMatchers, [&](const DynTypedMatcher &InnerMatcher) {
     return InnerMatcher.matchesNoKindCheck(DynNode, Finder, Builder);
   });
 }
 
 static bool eachOfVariadicOperator(const DynTypedNode &DynNode,
                                    ASTMatchFinder *Finder,
                                    BoundNodesTreeBuilder *Builder,
                                    ArrayRef<DynTypedMatcher> InnerMatchers) {
+  if (InnerMatchers.empty())
+    return true;
   BoundNodesTreeBuilder Result;
   bool Matched = false;
   for (const DynTypedMatcher &InnerMatcher : InnerMatchers) {
     BoundNodesTreeBuilder BuilderInner(*Builder);
     if (InnerMatcher.matches(DynNode, Finder, &BuilderInner)) {
       Matched = true;
       Result.addMatch(BuilderInner);
     }
   }
   *Builder = std::move(Result);
   return Matched;
 }
 
 static bool anyOfVariadicOperator(const DynTypedNode &DynNode,
                                   ASTMatchFinder *Finder,
                                   BoundNodesTreeBuilder *Builder,
                                   ArrayRef<DynTypedMatcher> InnerMatchers) {
+  if (InnerMatchers.empty())
+    return true;
   for (const DynTypedMatcher &InnerMatcher : InnerMatchers) {
     BoundNodesTreeBuilder Result = *Builder;
     if (InnerMatcher.matches(DynNode, Finder, &Result)) {
       *Builder = std::move(Result);
       return true;
     }
   }
   return false;
const internal::VariadicDynCastAllOfMatcher<Stmt, CXXFunctionalCastExpr>
     cxxFunctionalCastExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXTemporaryObjectExpr>
     cxxTemporaryObjectExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, PredefinedExpr>
     predefinedExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, DesignatedInitExpr>
     designatedInitExpr;
 const internal::VariadicOperatorMatcherFunc<
-    2, std::numeric_limits<unsigned>::max()>
+    0, std::numeric_limits<unsigned>::max()>
     eachOf = {internal::DynTypedMatcher::VO_EachOf};
 const internal::VariadicOperatorMatcherFunc<
-    2, std::numeric_limits<unsigned>::max()>
+    0, std::numeric_limits<unsigned>::max()>
     anyOf = {internal::DynTypedMatcher::VO_AnyOf};
 const internal::VariadicOperatorMatcherFunc<
-    2, std::numeric_limits<unsigned>::max()>
+    0, std::numeric_limits<unsigned>::max()>
     allOf = {internal::DynTypedMatcher::VO_AllOf};
 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,

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

EXPECT_TRUE(matches(
                    hasArgument(2, integerLiteral(equals(3)))))));
   EXPECT_TRUE(matches(
       Program, callExpr(allOf(
                    callee(functionDecl(hasName("f"))),
                    hasArgument(0, declRefExpr(to(varDecl()))),
                    hasArgument(1, hasType(pointsTo(recordDecl(hasName("T"))))),
                    hasArgument(2, integerLiteral(equals(3))),
                    hasArgument(3, integerLiteral(equals(4)))))));
+
+  EXPECT_TRUE(
+      matches("int F() { return 1; }", integerLiteral(allOf(equals(1)))));
+  EXPECT_TRUE(matches("int F() { return 1; }", integerLiteral(allOf())));
 }
 
 TEST_P(ASTMatchersTest, Has) {
   if (!GetParam().isCXX()) {
     // FIXME: Add a test for `has()` that does not depend on C++.
     return;
   }
 
TEST_P(ASTMatchersTest, AnyOf) {
   StatementMatcher MixedTypes = stmt(anyOf(ifStmt(), binaryOperator()));
   EXPECT_TRUE(matches("int F() { return 1 + 2; }", MixedTypes));
   EXPECT_TRUE(matches("int F() { if (true) return 1; }", MixedTypes));
   EXPECT_TRUE(notMatches("int F() { return 1; }", MixedTypes));
 
   EXPECT_TRUE(
       matches("void f() try { } catch (int) { } catch (...) { }",
               cxxCatchStmt(anyOf(hasDescendant(varDecl()), isCatchAll()))));
+
+  EXPECT_TRUE(
+      matches("int F() { return 1; }", integerLiteral(anyOf(equals(1)))));
+  EXPECT_TRUE(matches("int F() { return 1; }", integerLiteral(anyOf())));
 }
 
 TEST_P(ASTMatchersTest, IsDerivedFrom) {
   if (!GetParam().isCXX()) {
     return;
   }
 
   DeclarationMatcher IsDerivedFromX = cxxRecordDecl(isDerivedFrom("X"));

 }
 
 TEST_P(ASTMatchersTest, EachOf_TriggersForEachMatch) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
       "class A { int a; int b; };",
       recordDecl(eachOf(has(fieldDecl(hasName("a")).bind("v")),
                         has(fieldDecl(hasName("b")).bind("v")))),
       std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v", 2)));
+
+  EXPECT_TRUE(
+      matches("int F() { return 1; }", integerLiteral(eachOf(equals(1)))));
+  EXPECT_TRUE(matches("int F() { return 1; }", integerLiteral(eachOf())));
 }
 
 TEST_P(ASTMatchersTest, EachOf_BehavesLikeAnyOfUnlessBothMatch) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
       "struct A { int a; int c; };",
       recordDecl(eachOf(has(fieldDecl(hasName("a")).bind("v")),
                         has(fieldDecl(hasName("b")).bind("v")))),
       std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v", 1)));