[clang-tools-extra] 716469b - [clang-tidy] Fix segfault in bugprone-standalone-empty

[Christopher Di Bella via cfe-commits]

Author: Denis Nikitin
Date: 2023-01-25T20:06:41Z
New Revision: 716469b6139ab5ec5c5b5dac32891300260db8eb

URL: https://github.com/llvm/llvm-project/commit/716469b6139ab5ec5c5b5dac32891300260db8eb
DIFF: https://github.com/llvm/llvm-project/commit/716469b6139ab5ec5c5b5dac32891300260db8eb.diff

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

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

Reviewed By: cjdb

Differential Revision: https://reviews.llvm.org/D142423

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
    clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst
    clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
index 67d8e3693fb56..a66f838f1c8fa 100644
--- a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
@@ -29,10 +29,12 @@ 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;
@@ -70,10 +72,13 @@ const Expr *getCondition(const BoundNodes &Nodes, const StringRef NodeId) {
 }
 
 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 =
@@ -154,6 +159,8 @@ void StandaloneEmptyCheck::check(const MatchFinder::MatchResult &Result) {
       return;
     if (ParentReturnStmt)
       return;
+    if (NonMemberCall->getNumArgs() != 1)
+      return;
 
     SourceLocation NonMemberLoc = NonMemberCall->getExprLoc();
     SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc();

diff  --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst
index 049698a8b0f7a..80805bafddd50 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst
@@ -3,8 +3,8 @@
 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
@@ -29,3 +29,8 @@ A call to ``clear()`` would appropriately clear the contents of the range:
   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.

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
index d7615fdbbe922..53c651879f84b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
@@ -80,6 +80,10 @@ 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 {
@@ -306,6 +310,9 @@ bool test_qualified_empty() {
 
     test::empty(v);
     // no-warning
+
+    test_no_args::empty();
+    // no-warning
   }
 
   {
@@ -876,3 +883,24 @@ bool test_clear_with_qualifiers() {
     // 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
+}