[Clang-Tidy] Empty Check

Adds a clang-tidy check for the incorrect use of `empty()` on a
container when the result of the call is ignored.

Authored-by: Abraham Corea Diaz <abrahamcd@google.com>
Co-authored-by: Denis Nikitin <denik@google.com>

Reviewed By: cjdb

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

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

@@ -49,6 +49,7 @@
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
+#include "StandaloneEmptyCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
@@ -156,6 +157,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-sizeof-expression");
     CheckFactories.registerCheck<SpuriouslyWakeUpFunctionsCheck>(
         "bugprone-spuriously-wake-up-functions");
+    CheckFactories.registerCheck<StandaloneEmptyCheck>(
+        "bugprone-standalone-empty");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -45,6 +45,7 @@ add_clang_library(clangTidyBugproneModule
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp
+  StandaloneEmptyCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp

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

@@ -0,0 +1,207 @@
+//===--- StandaloneEmptyCheck.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "StandaloneEmptyCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Casting.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+using ast_matchers::BoundNodes;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+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;
+using ast_matchers::stmtExpr;
+using ast_matchers::unless;
+using ast_matchers::voidType;
+
+const Expr *getCondition(const BoundNodes &Nodes, const StringRef NodeId) {
+  const auto *If = Nodes.getNodeAs<IfStmt>(NodeId);
+  if (If != nullptr)
+    return If->getCond();
+
+  const auto *For = Nodes.getNodeAs<ForStmt>(NodeId);
+  if (For != nullptr)
+    return For->getCond();
+
+  const auto *While = Nodes.getNodeAs<WhileStmt>(NodeId);
+  if (While != nullptr)
+    return While->getCond();
+
+  const auto *Do = Nodes.getNodeAs<DoStmt>(NodeId);
+  if (Do != nullptr)
+    return Do->getCond();
+
+  const auto *Switch = Nodes.getNodeAs<SwitchStmt>(NodeId);
+  if (Switch != nullptr)
+    return Switch->getCond();
+
+  return nullptr;
+}
+
+void StandaloneEmptyCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  const auto NonMemberMatcher = expr(ignoringImplicit(ignoringParenImpCasts(
+      callExpr(
+          hasParent(stmt(optionally(hasParent(stmtExpr().bind("stexpr"))))
+                        .bind("parent")),
+          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()))))))))
+          .bind("empty");
+
+  Finder->addMatcher(MemberMatcher, this);
+  Finder->addMatcher(NonMemberMatcher, this);
+}
+
+void StandaloneEmptyCheck::check(const MatchFinder::MatchResult &Result) {
+  // Skip if the parent node is Expr.
+  if (Result.Nodes.getNodeAs<Expr>("parent"))
+    return;
+
+  const auto PParentStmtExpr = Result.Nodes.getNodeAs<Expr>("stexpr");
+  const auto ParentCompStmt = Result.Nodes.getNodeAs<CompoundStmt>("parent");
+  const auto *ParentCond = getCondition(Result.Nodes, "parent");
+
+  if (const auto *MemberCall =
+          Result.Nodes.getNodeAs<CXXMemberCallExpr>("empty")) {
+    // Skip if it's a condition of the parent statement.
+    if (ParentCond == MemberCall->getExprStmt())
+      return;
+    // Skip if it's the last statement in the GNU extension
+    // statement expression.
+    if (PParentStmtExpr && ParentCompStmt &&
+        ParentCompStmt->body_back() == MemberCall->getExprStmt())
+      return;
+
+    SourceLocation MemberLoc = MemberCall->getBeginLoc();
+    SourceLocation ReplacementLoc = MemberCall->getExprLoc();
+    SourceRange ReplacementRange = SourceRange(ReplacementLoc, ReplacementLoc);
+
+    ASTContext &Context = MemberCall->getRecordDecl()->getASTContext();
+    DeclarationName Name =
+        Context.DeclarationNames.getIdentifier(&Context.Idents.get("clear"));
+
+    auto Candidates = MemberCall->getRecordDecl()->lookupDependentName(
+        Name, [](const NamedDecl *ND) {
+          return isa<CXXMethodDecl>(ND) &&
+                 llvm::cast<CXXMethodDecl>(ND)->getMinRequiredArguments() ==
+                     0 &&
+                 !llvm::cast<CXXMethodDecl>(ND)->isConst();
+        });
+
+    bool HasClear = !Candidates.empty();
+    if (HasClear) {
+      const CXXMethodDecl *Clear = llvm::cast<CXXMethodDecl>(Candidates.at(0));
+      QualType RangeType = MemberCall->getImplicitObjectArgument()->getType();
+      bool QualifierIncompatible =
+          (!Clear->isVolatile() && RangeType.isVolatileQualified()) ||
+          RangeType.isConstQualified();
+      if (!QualifierIncompatible) {
+        diag(MemberLoc,
+             "ignoring the result of 'empty()'; did you mean 'clear()'? ")
+            << FixItHint::CreateReplacement(ReplacementRange, "clear");
+        return;
+      }
+    }
+
+    diag(MemberLoc, "ignoring the result of 'empty()'");
+
+  } else if (const auto *NonMemberCall =
+                 Result.Nodes.getNodeAs<CallExpr>("empty")) {
+    if (ParentCond == NonMemberCall->getExprStmt())
+      return;
+    if (PParentStmtExpr && ParentCompStmt &&
+        ParentCompStmt->body_back() == NonMemberCall->getExprStmt())
+      return;
+
+    SourceLocation NonMemberLoc = NonMemberCall->getExprLoc();
+    SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc();
+
+    const Expr *Arg = NonMemberCall->getArg(0);
+    CXXRecordDecl *ArgRecordDecl = Arg->getType()->getAsCXXRecordDecl();
+    if (ArgRecordDecl == nullptr)
+      return;
+
+    ASTContext &Context = ArgRecordDecl->getASTContext();
+    DeclarationName Name =
+        Context.DeclarationNames.getIdentifier(&Context.Idents.get("clear"));
+
+    auto Candidates =
+        ArgRecordDecl->lookupDependentName(Name, [](const NamedDecl *ND) {
+          return isa<CXXMethodDecl>(ND) &&
+                 llvm::cast<CXXMethodDecl>(ND)->getMinRequiredArguments() ==
+                     0 &&
+                 !llvm::cast<CXXMethodDecl>(ND)->isConst();
+        });
+
+    bool HasClear = !Candidates.empty();
+
+    if (HasClear) {
+      const CXXMethodDecl *Clear = llvm::cast<CXXMethodDecl>(Candidates.at(0));
+      bool QualifierIncompatible =
+          (!Clear->isVolatile() && Arg->getType().isVolatileQualified()) ||
+          Arg->getType().isConstQualified();
+      if (!QualifierIncompatible) {
+        std::string ReplacementText =
+            std::string(Lexer::getSourceText(
+                CharSourceRange::getTokenRange(Arg->getSourceRange()),
+                *Result.SourceManager, getLangOpts())) +
+            ".clear()";
+        SourceRange ReplacementRange =
+            SourceRange(NonMemberLoc, NonMemberEndLoc);
+        diag(NonMemberLoc,
+             "ignoring the result of '%0'; did you mean 'clear()'?")
+            << llvm::dyn_cast<NamedDecl>(NonMemberCall->getCalleeDecl())
+                   ->getQualifiedNameAsString()
+            << FixItHint::CreateReplacement(ReplacementRange, ReplacementText);
+        return;
+      }
+    }
+
+    diag(NonMemberLoc, "ignoring the result of '%0'")
+        << llvm::dyn_cast<NamedDecl>(NonMemberCall->getCalleeDecl())
+               ->getQualifiedNameAsString();
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

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

@@ -0,0 +1,38 @@
+//===--- StandaloneEmptyCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STANDALONEEMPTYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STANDALONEEMPTYCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Checks for ignored calls to `empty()` on a range and suggests `clear()`
+/// as an alternative if it is an existing member function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/standalone-empty.html
+class StandaloneEmptyCheck : public ClangTidyCheck {
+public:
+  StandaloneEmptyCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STANDALONEEMPTYCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -121,6 +121,11 @@ New checks
   Warns when using ``static`` function or variables at global scope, and suggests
   moving them into an anonymous namespace.
 
+- New :doc:`bugprone-standalone-empty <clang-tidy/checks/bugprone/standalone-empty>` check.
+
+  Warns when `empty()` is used on a range and the result is ignored. Suggests `clear()`
+  if it is an existing member function.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

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

@@ -0,0 +1,31 @@
+.. 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.
+
+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();

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -115,6 +115,7 @@ Clang-Tidy Checks
    `bugprone-sizeof-container <bugprone/sizeof-container.html>`_,
    `bugprone-sizeof-expression <bugprone/sizeof-expression.html>`_,
    `bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions.html>`_,
+   `bugprone-standalone-empty <bugprone/standalone-empty.html>`_, "Yes"
    `bugprone-string-constructor <bugprone/string-constructor.html>`_, "Yes"
    `bugprone-string-integer-assignment <bugprone/string-integer-assignment.html>`_, "Yes"
    `bugprone-string-literal-with-embedded-nul <bugprone/string-literal-with-embedded-nul.html>`_,