[clang-tidy] Fix segfault in bugprone-standalone-empty

[denik]

Summary

The check incorrectly identified empty() method call in the template
class definition as a stand-alone function call.
This led to a crash because the checker did not expect empty() function
calls without arguments.

Fixes: https://github.com/llvm/llvm-project/issues/59487

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 
 namespace clang::tidy::bugprone {
 
 using ast_matchers::BoundNodes;
 using ast_matchers::callee;
 using ast_matchers::callExpr;
+using ast_matchers::classTemplateDecl;
 using ast_matchers::cxxMemberCallExpr;
 using ast_matchers::cxxMethodDecl;
 using ast_matchers::expr;
 using ast_matchers::functionDecl;
+using ast_matchers::hasAncestor;
 using ast_matchers::hasName;
 using ast_matchers::hasParent;
 using ast_matchers::ignoringImplicit;
 using ast_matchers::ignoringParenImpCasts;
 using ast_matchers::MatchFinder;
 using ast_matchers::optionally;
 using ast_matchers::returns;
 using ast_matchers::stmt;
const Expr *getCondition(const BoundNodes &Nodes, const StringRef NodeId) {
   const auto *Switch = Nodes.getNodeAs<SwitchStmt>(NodeId);
   if (Switch != nullptr)
     return Switch->getCond();
 
   return nullptr;
 }
 
 void StandaloneEmptyCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  // Ignore empty calls in a template definition which fall under callExpr
+  // non-member matcher even if they are methods.
   const auto NonMemberMatcher = expr(ignoringImplicit(ignoringParenImpCasts(
       callExpr(
           hasParent(stmt(optionally(hasParent(stmtExpr().bind("stexpr"))))
                         .bind("parent")),
+          unless(hasAncestor(classTemplateDecl())),
           callee(functionDecl(hasName("empty"), unless(returns(voidType())))))
           .bind("empty"))));
   const auto MemberMatcher =
       expr(ignoringImplicit(ignoringParenImpCasts(cxxMemberCallExpr(
                hasParent(stmt(optionally(hasParent(stmtExpr().bind("stexpr"))))
                              .bind("parent")),
                callee(cxxMethodDecl(hasName("empty"),
                                     unless(returns(voidType()))))))))
diag(MemberLoc, "ignoring the result of 'empty()'");
                  Result.Nodes.getNodeAs<CallExpr>("empty")) {
     if (ParentCond == NonMemberCall->getExprStmt())
       return;
     if (PParentStmtExpr && ParentCompStmt &&
         ParentCompStmt->body_back() == NonMemberCall->getExprStmt())
       return;
     if (ParentReturnStmt)
       return;
+    if (NonMemberCall->getNumArgs() != 1)
+      return;
 
     SourceLocation NonMemberLoc = NonMemberCall->getExprLoc();
     SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc();
 
     const Expr *Arg = NonMemberCall->getArg(0);
     CXXRecordDecl *ArgRecordDecl = Arg->getType()->getAsCXXRecordDecl();
     if (ArgRecordDecl == nullptr)
       return;

clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst

 .. title:: clang-tidy - bugprone-standalone-empty
 
 bugprone-standalone-empty
 =========================
 
-Warns when `empty()` is used on a range and the result is ignored. Suggests 
-`clear()` if it is an existing member function.
+Warns when ``empty()`` is used on a range and the result is ignored. Suggests
+``clear()`` if it is an existing member function.
 
 The ``empty()`` method on several common ranges returns a Boolean indicating
 whether or not the range is empty, but is often mistakenly interpreted as
 a way to clear the contents of a range. Some ranges offer a ``clear()``
 method for this purpose. This check warns when a call to empty returns a
 result that is ignored, and suggests replacing it with a call to ``clear()``
 if it is available as a member function of the range.
 
 For example, the following code could be used to indicate whether a range
 is empty or not, but the result is ignored:
 
 .. code-block:: c++
 
   std::vector<int> v;
   ...
   v.empty();
 
 A call to ``clear()`` would appropriately clear the contents of the range:
 
 .. code-block:: c++
 
   std::vector<int> v;
   ...
   v.clear();
+
+Limitations:
+- Doesn't warn if ``empty()`` is defined and used with the ignore result in the
+  class template definition (for example in the library implementation). These
+  error cases can be caught with ``[[nodiscard]]`` attribute.

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

 bool empty(T &&);
 } // namespace absl
 
 namespace test {
 template <class T>
 void empty(T &&);
 } // namespace test
 
+namespace test_no_args {
+bool empty();
+} // namespace test_no_args
+
 namespace base {
 template <typename T>
 struct base_vector {
     void clear();
 };
 
 template <typename T>
 struct base_vector_clear_with_args {
bool test_qualified_empty() {
     // CHECK-FIXES: {{^  }}  v.clear();{{$}}
 
     absl::empty(v);
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'absl::empty'; did you mean 'clear()'? [bugprone-standalone-empty]
     // CHECK-FIXES: {{^  }}  v.clear();{{$}}
 
     test::empty(v);
     // no-warning
+
+    test_no_args::empty();
+    // no-warning
   }
 
   {
     absl::string s;
     std::empty(s);
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty]
   }
 
bool test_clear_with_qualifiers() {
   }
 
   {
     volatile std::vector_with_clear<int> v;
     return empty(v);
     // no-warning
   }
 }
+
+namespace user_lib {
+template <typename T>
+struct vector {
+  bool empty();
+  bool test_empty_inside_impl() {
+    empty();
+    // no-warning
+    return empty();
+    // no-warning
+  }
+};
+} // namespace user_lib
+
+bool test_template_empty_outside_impl() {
+  user_lib::vector<int> v;
+  v.empty();
+  // CHECK-MESSAGES: :[[#@LINE-1]]:3: warning: ignoring the result of 'empty()' [bugprone-standalone-empty]
+  return v.empty();
+  // no-warning
+}