[ASTMatchers] Add forCallable(), a generalization of forFunction().

[NoQ]

Summary

It additionally covers Objective-C methods and blocks that don't inherit from FunctionDecl.

I'm open to suggestions here. For instance, it might make sense to have three different matchers instead (forFunction(), forBlock(), forObjCMethod()) and anyOf() them. I don't think it's practical though; most of the time you want any callable and if you don't you can always narrow it down with forCallable(functionDecl()) or something like that. I guess it might make sense to implement both approaches.

I'm also open to suggestions with respect to the very fact that such matchers exist in the first place. From existing use cases it looks to me that most of the time (including my use case) they're used to avoid the problem with hasDescendant() (and similar matchers) digging into nested declarations (eg., inspecting the body of a lambda within a function when you only want it to inspect the function itself). I would be totally satisfied with a better alternative for hasDescendant() instead - that only traverses statements; that'd probably be faster as well as more precise and concise. @stephenkelly, IIRC you've voiced some strong opinions on this subject on the mailing list.

[steveire]

I'm not certain what strong opinions I've voiced on the mailing list about that. Was it just that I think hasDescendant can lead to unexpected matches and a more-specific matcher should always be used when available (such as hasArgument)? I do think has, hasDescendant, hasAncestor etc have their uses and they get more useful with utilities like forFunction.

I don't know anything about objc. Is it possible to have callable decls nested within each other, like in c++? Is it common? forFunction is mostly useful for the very common case of lambdas within functions.

Does it make sense to add an overload for LambdaExpr so that you can write forCallable(lambdaExpr())?

Is there a missing test for forFunction(functionDecl())?

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

7563

The formatting changes are good, but should be done in a separate NFC commit as they're unrelated.

7583–7585

It looks like you can re-flow the comments a bit, and add lambdas to the description.

7597

You should also update Registry.cpp to expose this via the dynamic matchers.

[NoQ]

In D102213#2750050, @steveire wrote:

I'm not certain what strong opinions I've voiced on the mailing list about that. Was it just that I think hasDescendant can lead to unexpected matches and a more-specific matcher should always be used when available (such as hasArgument)?

Yeah probably :D So, anyway, you sound like the right person to ask: do you have any immediate thoughts on a variant of hasDescendant that doesn't lead to unexpected matches (or leads to less unexpected matches) because it stops exploring insides of nested declarations? Would that be useful in your endeavors?

I do think has, hasDescendant, hasAncestor etc have their uses and they get more useful with utilities like forFunction.

I don't know anything about objc. Is it possible to have callable decls nested within each other, like in c++? Is it common? forFunction is mostly useful for the very common case of lambdas within functions.

ObjC class/interface declarations or method implementations cannot be nested into functions. Blocks, however, are like lambdas, they can appear anywhere (in fact, blocks and lambdas are implicitly convertible into each other). Additionally, in Objective-C++ you can have lambdas nested anywhere including methods or blocks and you can still have nested C++ classes everywhere.

Does it make sense to add an overload for LambdaExpr so that you can write forCallable(lambdaExpr())?

Dunno, maybe but I've never encountered such use-cases. If at all, the same problem applies to forFunction() as well.

Is there a missing test for forFunction(functionDecl())?

Uhm yeah, I should add them. There's some code duplication so it's worth it to at least duplicate the tests.

clang/include/clang/ASTMatchers/ASTMatchers.h

7597

Uh-oh, i knew i was forgetting something!

Do we have a checklist? (Do we want to?)

[NoQ]

Address comments. Yay, diff is no longer confused!

[vsavchenko]

I honestly don't see a reason why it's not a part of forFunction. forFunction matches C++ methods and lambdas, Obj-C methods and blocks don't seem that much more special to have an extended matcher just for those.
I really don't think that it will break someone's workflow and that someone really relied on the fact that forFunction doesn't match those.
Other than that it's a great patch IMO!

[aaron.ballman]

In D102213#2753388, @vsavchenko wrote:

I honestly don't see a reason why it's not a part of forFunction. forFunction matches C++ methods and lambdas, Obj-C methods and blocks don't seem that much more special to have an extended matcher just for those.
I really don't think that it will break someone's workflow and that someone really relied on the fact that forFunction doesn't match those.

I think this is a good observation -- if you modify forFunction(), do you see any test failures that look unexpected when running all the clang-tidy tests?

[NoQ]

I totally agree that changing forFunction() to work correctly is the right solution but backwards compatibility breakage is very real because forFunction() accepts a Matcher<FunctionDecl> whereas forCallable() accepts Matcher<Decl> which is a smaller set of matchers. Eg., forFunction(hasName()) is valid code but forCallable(hasName()) is not because BlockDecl isn't a NamedDecl. So, like, I suspect that not a lot of code expects this to be the function, but a lot of code expects it to be a function. See how much types did I have to change just to get the infinite loop checker to compile in D102214.

If we're ok with such compatibility breakage then i'm all for it.

[aaron.ballman]

In D102213#2754495, @NoQ wrote:

I totally agree that changing forFunction() to work correctly is the right solution but backwards compatibility breakage is very real because forFunction() accepts a Matcher<FunctionDecl> whereas forCallable() accepts Matcher<Decl> which is a smaller set of matchers. Eg., forFunction(hasName()) is valid code but forCallable(hasName()) is not because BlockDecl isn't a NamedDecl. So, like, I suspect that not a lot of code expects this to be the function, but a lot of code expects it to be a function. See how much types did I have to change just to get the infinite loop checker to compile in D102214.

If we're ok with such compatibility breakage then i'm all for it.

That's a fair point. We try to not let changes break existing code, but we also don't make any stability guarantees that prevent us from doing so when that's the right answer. Would this breakage be loud for users of the C++ interface, or would the behavior silently change so that anyone who fails to add a namedDecl() in the right place stops getting expected results?

[NoQ]

The breakage is loud; the code will no longer compile when the intermediate decl() (or namedDecl(), or whatever) is not present. The more annoying part is that when you add namedDecl() back (or if you had it spelled out this way from the start, which doesn't make much sense but is valid and shorter than spelling out functionDecl()), your Node.get<FunctionDecl>() in the match callback will silently start returning null (on anything that isn't a functionDecl()) which may lead to unexpected crashes (previously there was no match at all, now there's a match but the node isn't of the expected type). So a relatively distant piece of code will require manual audit in order to address the potential breakage.

[NoQ]

I'm also mildly worried that Function is not the technically correct term. Maybe we should mark the old matcher as deprecated instead?

[NoQ]

Additionally, forCallable(functionDecl()) is not equivalent to forFunction() (the former silently matches less stuff). So anybody who had functionDecl() spelled out explicitly in their code will observe a silent change in behavior. I think that most of the time the old behavior was a footgun (i.e., most of the time when people wrote forFunction() they actually expected it to behave like forCallable(functionDecl())) but I can't be 100% sure.

[vsavchenko]

In D102213#2755963, @NoQ wrote:

I totally agree that changing forFunction() to work correctly is the right solution but backwards compatibility breakage is very real because forFunction() accepts a Matcher<FunctionDecl> whereas forCallable() accepts Matcher<Decl>

Yeah, I see now, it's not that straightforward.
Parameter type difference can be combated even keeping backward compatibility (so to speak). You can do AST_MATCHER_P_OVERLOAD (like it's done for hasPrefix, for example) to have two overloads of forFunction: one for Matcher<FunctionDecl> (aka old implementation) and one for Matcher<Decl> for new implementation. I didn't check it, but it probably should work. It is not elegant though and I'm not pushing it.

The breakage is loud; the code will no longer compile when the intermediate decl() (or namedDecl(), or whatever) is not present. The more annoying part is that when you add namedDecl() back (or if you had it spelled out this way from the start, which doesn't make much sense but is valid and shorter than spelling out functionDecl()), your Node.get<FunctionDecl>() in the match callback will silently start returning null (on anything that isn't a functionDecl()) which may lead to unexpected crashes (previously there was no match at all, now there's a match but the node isn't of the expected type). So a relatively distant piece of code will require manual audit in order to address the potential breakage.

The breakage example has a bit weird precondition IMO. In that scenario, the user had to use decl or namedDecl where they actually meant functionDecl AND they should run their matchers on code with blocks. And even if it does happen to someone, I think it's not going to be very painful because a simple dump of the node will show what's going on.

Additionally, forCallable(functionDecl()) is not equivalent to forFunction() (the former silently matches less stuff).

I'm not sure I understand this one. Can you please show an example where one matches something that the other doesn't?

[NoQ]

In D102213#2756254, @vsavchenko wrote:

Additionally, forCallable(functionDecl()) is not equivalent to forFunction() (the former silently matches less stuff).

I'm not sure I understand this one. Can you please show an example where one matches something that the other doesn't?

void foo() {
  ^{
    int x = 1;
  }
}

Here declStmt(forCallable(functionDecl())) doesn't match because the callable the variable belongs to isn't a FunctionDecl but declStmt(forFunction()) matches 'x' for 'foo'.

[vsavchenko]

In D102213#2757253, @NoQ wrote:

In D102213#2756254, @vsavchenko wrote:

Additionally, forCallable(functionDecl()) is not equivalent to forFunction() (the former silently matches less stuff).

I'm not sure I understand this one. Can you please show an example where one matches something that the other doesn't?

void foo() {
  ^{
    int x = 1;
  }
}

Here declStmt(forCallable(functionDecl())) doesn't match because the callable the variable belongs to isn't a FunctionDecl but declStmt(forFunction()) matches 'x' for 'foo'.

Ahhh, right! Thanks!

[aaron.ballman]

In D102213#2755064, @NoQ wrote:

I'm also mildly worried that Function is not the technically correct term. Maybe we should mark the old matcher as deprecated instead?

Thank you for the explanations as to why there's a separate matcher. I think using a separate matcher is the safer approach to introduce the functionality. I'd be fine if we marked the old matcher as deprecated in the documentation for it. (I don't believe that we've added any louder deprecation markings yet aside from comments.)

clang/include/clang/ASTMatchers/ASTMatchers.h

7597

I don't think we have a checklist, but the mental checklist I use is:

• If there's a change to doc comments in ASTMatchers.h, did the HTML file get regenerated?
• If there's a new matcher added, did Registry.cpp get updated for it?
• If there are changes to the list in Registry.cpp, is the list still alphabetical?
• Testcases?

7601

Might as well fix this clang-format warning.

[NoQ]

Address review comments. Add a deprecation notice to forFunction().

clang/include/clang/ASTMatchers/ASTMatchers.h

7597

Wait, it's autogenerated? I updated it by hand :D
Fxd.

[vsavchenko]

OK, I agree with Aaron that having a separate matcher is reasonable. Thanks for the patient explanation :)
LGTM from my side then.

[aaron.ballman]

LGTM, thank you!

[NoQ]

Make examples for forCallable() more relatable given that the original matcher is now deprecated.

[NoQ]

Fix typo in documentation.

clang/docs/LibASTMatchersReference.html

F& operator=(const F& o) {
   return *this;
 }
 returnStmt(forFunction(hasName("operator=")))
   matches 'return *this'
   but does not match 'return v > 0'
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html">Stmt</a>&gt;</td><td class="name" onclick="toggle('forCallable0')"><a name="forCallable0Anchor">forCallable</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Decl.html">Decl</a>&gt; InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="forCallable0"><pre>
+Matches declaration of the function, method, or block the statement belongs to.
+Similar to 'forFunction' but additionally covers Objective-C methods and blocks.
+
+Given:
+-(void) foo {
+  int x = 1;
+  dispatch_sync(queue, ^{ int y = 2; });
+}
+
+declStmt(forCallable(objcMethodDecl()))
+  matches 'int x = 1'
+  but does not match 'int y = 2'.
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html">Stmt</a>&gt;</td><td class="name" onclick="toggle('sizeOfExpr0')"><a name="sizeOfExpr0Anchor">sizeOfExpr</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1UnaryExprOrTypeTraitExpr.html">UnaryExprOrTypeTraitExpr</a>&gt;  InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="sizeOfExpr0"><pre>Same as unaryExprOrTypeTraitExpr, but only matching
 sizeof.
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1SubstTemplateTypeParmType.html">SubstTemplateTypeParmType</a>&gt;</td><td class="name" onclick="toggle('hasReplacementType0')"><a name="hasReplacementType0Anchor">hasReplacementType</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Type.html">Type</a>&gt;</td></tr>
 <tr><td colspan="4" class="doc" id="hasReplacementType0"><pre>Matches template type parameter substitutions that have a replacement

clang/include/clang/ASTMatchers/ASTMatchers.h

 /// returnStmt(forFunction(hasName("operator=")))
 ///   matches 'return *this'
 ///   but does not match 'return v > 0'
 AST_MATCHER_P(Stmt, forFunction, internal::Matcher<FunctionDecl>,
               InnerMatcher) {
   const auto &Parents = Finder->getASTContext().getParents(Node);
 
   llvm::SmallVector<DynTypedNode, 8> Stack(Parents.begin(), Parents.end());
   while (!Stack.empty()) {
     const auto &CurNode = Stack.back();
     Stack.pop_back();
     if (const auto *FuncDeclNode = CurNode.get<FunctionDecl>()) {
       if (InnerMatcher.matches(*FuncDeclNode, Finder, Builder)) {
         return true;
       }
     } else if (const auto *LambdaExprNode = CurNode.get<LambdaExpr>()) {
       if (InnerMatcher.matches(*LambdaExprNode->getCallOperator(), Finder,
                                Builder)) {
         return true;
       }
     } else {
       for (const auto &Parent : Finder->getASTContext().getParents(CurNode))
         Stack.push_back(Parent);
     }
   }
   return false;
 }
 
+/// Matches declaration of the function, method, or block the statement
+/// belongs to. Similar to 'forFunction' but additionally covers Objective-C
+/// methods and blocks.
+///
+/// Given:
+/// \code
+/// -(void) foo {
+///   int x = 1;
+///   dispatch_sync(queue, ^{ int y = 2; });
+/// }
+/// \endcode
+/// declStmt(forCallable(objcMethodDecl()))
+///   matches 'int x = 1'
+///   but does not match 'int y = 2'.
+AST_MATCHER_P(Stmt, forCallable, internal::Matcher<Decl>, InnerMatcher) {
+  const auto &Parents = Finder->getASTContext().getParents(Node);
+
+  llvm::SmallVector<DynTypedNode, 8> Stack(Parents.begin(), Parents.end());
+  while(!Stack.empty()) {
+    const auto &CurNode = Stack.back();
+    Stack.pop_back();
+    if (const auto *FuncDeclNode = CurNode.get<FunctionDecl>()) {
+      if (InnerMatcher.matches(*FuncDeclNode, Finder, Builder)) {
+        return true;
+      }
+    } else if (const auto *LambdaExprNode = CurNode.get<LambdaExpr>()) {
+      if (InnerMatcher.matches(*LambdaExprNode->getCallOperator(), Finder,
+                               Builder)) {
+        return true;
+      }
+    } else if (const auto *ObjCMethodDeclNode = CurNode.get<ObjCMethodDecl>()) {
+      if (InnerMatcher.matches(*ObjCMethodDeclNode, Finder, Builder)) {
+        return true;
+      }
+    } else if (const auto *BlockDeclNode = CurNode.get<BlockDecl>()) {
+      if (InnerMatcher.matches(*BlockDeclNode, Finder, Builder)) {
+        return true;
+      }
+    } else {
+      for (const auto &Parent : Finder->getASTContext().getParents(CurNode))
+        Stack.push_back(Parent);
+    }
+  }
+  return false;
+}
+
 /// Matches a declaration that has external formal linkage.
 ///
 /// Example matches only z (matcher = varDecl(hasExternalFormalLinkage()))
 /// \code
 /// void f() {
 ///   int x;
 ///   static int y;
 /// }

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(equalsBoundNode);
   REGISTER_MATCHER(equalsIntegralValue);
   REGISTER_MATCHER(explicitCastExpr);
   REGISTER_MATCHER(expr);
   REGISTER_MATCHER(exprWithCleanups);
   REGISTER_MATCHER(fieldDecl);
   REGISTER_MATCHER(fixedPointLiteral);
   REGISTER_MATCHER(floatLiteral);
+  REGISTER_MATCHER(forCallable);
   REGISTER_MATCHER(forDecomposition);
   REGISTER_MATCHER(forEach);
   REGISTER_MATCHER(forEachArgumentWithParam);
   REGISTER_MATCHER(forEachArgumentWithParamType);
   REGISTER_MATCHER(forEachConstructorInitializer);
   REGISTER_MATCHER(forEachDescendant);
   REGISTER_MATCHER(forEachOverridden);
   REGISTER_MATCHER(forEachSwitchCase);

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

EXPECT_TRUE(
     matches(
       CppString1,
       returnStmt(forFunction(hasName("operator()")),
                  has(integerLiteral()))));
   EXPECT_TRUE(matches(CppString2, returnStmt(forFunction(hasName("F2")))));
   EXPECT_TRUE(notMatches(CppString2, returnStmt(forFunction(hasName("F")))));
 }
 
+TEST(StatementMatcher, ForCallable) {
+  // These tests are copied over from the forFunction() test above.
+  StringRef CppString1 = "struct PosVec {"
+                         "  PosVec& operator=(const PosVec&) {"
+                         "    auto x = [] { return 1; };"
+                         "    return *this;"
+                         "  }"
+                         "};";
+  StringRef CppString2 = "void F() {"
+                         "  struct S {"
+                         "    void F2() {"
+                         "       return;"
+                         "    }"
+                         "  };"
+                         "}";
+
+  EXPECT_TRUE(
+    matches(
+      CppString1,
+      returnStmt(forCallable(functionDecl(hasName("operator="))),
+                 has(unaryOperator(hasOperatorName("*"))))));
+  EXPECT_TRUE(
+    notMatches(
+      CppString1,
+      returnStmt(forCallable(functionDecl(hasName("operator="))),
+                 has(integerLiteral()))));
+  EXPECT_TRUE(
+    matches(
+      CppString1,
+      returnStmt(forCallable(functionDecl(hasName("operator()"))),
+                 has(integerLiteral()))));
+  EXPECT_TRUE(matches(CppString2,
+                      returnStmt(forCallable(functionDecl(hasName("F2"))))));
+  EXPECT_TRUE(notMatches(CppString2,
+                         returnStmt(forCallable(functionDecl(hasName("F"))))));
+
+  // These tests are specific to forCallable().
+  StringRef ObjCString1 = "@interface I"
+                          "-(void) foo;"
+                          "@end"
+                          "@implementation I"
+                          "-(void) foo {"
+                          "  void (^block)() = ^{ 0x2b | ~0x2b; };"
+                          "}"
+                          "@end";
+
+  EXPECT_TRUE(
+    matchesObjC(
+      ObjCString1,
+      binaryOperator(forCallable(blockDecl()))));
+
+  EXPECT_TRUE(
+    notMatchesObjC(
+      ObjCString1,
+      binaryOperator(forCallable(objcMethodDecl()))));
+
+  StringRef ObjCString2 = "@interface I"
+                          "-(void) foo;"
+                          "@end"
+                          "@implementation I"
+                          "-(void) foo {"
+                          "  0x2b | ~0x2b;"
+                          "  void (^block)() = ^{};"
+                          "}"
+                          "@end";
+
+  EXPECT_TRUE(
+    matchesObjC(
+      ObjCString2,
+      binaryOperator(forCallable(objcMethodDecl()))));
+
+  EXPECT_TRUE(
+    notMatchesObjC(
+      ObjCString2,
+      binaryOperator(forCallable(blockDecl()))));
+}
+
 TEST(Matcher, ForEachOverriden) {
   const auto ForEachOverriddenInClass = [](const char *ClassName) {
     return cxxMethodDecl(ofClass(hasName(ClassName)), isVirtual(),
                          forEachOverridden(cxxMethodDecl().bind("overridden")))
         .bind("override");
   };
   static const char Code1[] = "class A { virtual void f(); };"
                               "class B : public A { void f(); };"