diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/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" @@ -155,6 +156,8 @@ "bugprone-sizeof-expression"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); + CheckFactories.registerCheck( + "bugprone-standalone-empty"); CheckFactories.registerCheck( "bugprone-string-constructor"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -45,6 +45,7 @@ SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp + StandaloneEmptyCheck.cpp StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h new file mode 100644 --- /dev/null +++ b/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 diff --git a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp @@ -0,0 +1,176 @@ +//===--- 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/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/STLExtras.h" +#include "llvm/ADT/SmallVector.h" +#include + +namespace clang { +namespace tidy { +namespace bugprone { + +using ast_matchers::BoundNodes; +using ast_matchers::MatchFinder; + +const Expr *getCondition(const BoundNodes &Nodes, const StringRef NodeId) { + const auto *If = Nodes.getNodeAs(NodeId); + if (If != nullptr) + return If->getCond(); + + const auto *For = Nodes.getNodeAs(NodeId); + if (For != nullptr) + return For->getCond(); + + const auto *While = Nodes.getNodeAs(NodeId); + if (While != nullptr) + return While->getCond(); + + const auto *Do = Nodes.getNodeAs(NodeId); + if (Do != nullptr) + return Do->getCond(); + + const auto *Switch = Nodes.getNodeAs(NodeId); + if (Switch != nullptr) + return Switch->getCond(); + + return nullptr; +} + +void StandaloneEmptyCheck::registerMatchers(ast_matchers::MatchFinder *Finder) { + 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::optionally; + using ast_matchers::returns; + using ast_matchers::stmt; + using ast_matchers::stmtExpr; + using ast_matchers::unless; + using ast_matchers::voidType; + + 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("parent")) + return; + + const auto PParentStmtExpr = Result.Nodes.getNodeAs("stexpr"); + const auto ParentCompStmt = Result.Nodes.getNodeAs("parent"); + const auto *ParentCond = getCondition(Result.Nodes, "parent"); + + if (const auto *MemberCall = + Result.Nodes.getNodeAs("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); + + CXXRecordDecl::method_range Methods = + MemberCall->getRecordDecl()->methods(); + DeclContext::specific_decl_iterator Clear = + llvm::find_if(Methods, [](const CXXMethodDecl *F) { + return F->getDeclName().getAsString() == "clear" && + F->getMinRequiredArguments() == 0; + }); + + if (Clear != Methods.end()) { + diag(MemberLoc, + "ignoring the result of 'empty()', did you mean 'clear()'? ") + << FixItHint::CreateReplacement(ReplacementRange, "clear"); + } else { + diag(MemberLoc, "ignoring the result of 'empty()'"); + } + + } else if (const auto *NonMemberCall = + Result.Nodes.getNodeAs("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 == NULL) + return; + CXXRecordDecl::method_range Methods = ArgRecordDecl->methods(); + DeclContext::specific_decl_iterator Clear = + llvm::find_if(Methods, [](const CXXMethodDecl *F) { + return F->getDeclName().getAsString() == "clear" && + F->getMinRequiredArguments() == 0; + }); + + if (Clear != Methods.end()) { + 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(NonMemberCall->getCalleeDecl()) + ->getQualifiedNameAsString() + << FixItHint::CreateReplacement(ReplacementRange, ReplacementText); + } else { + diag(NonMemberLoc, "ignoring the result of '%0'") + << llvm::dyn_cast(NonMemberCall->getCalleeDecl()) + ->getQualifiedNameAsString(); + } + } +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -122,6 +122,11 @@ Finds initializations of C++ shared pointers to non-array type that are initialized with an array. +- New :doc:`bugprone-standalone-empty ` check. + + Warns when `empty()` is used on a range and the result is ignored. Suggests `clear()` as an alternative + if it is an existing member function. + - New :doc:`bugprone-unchecked-optional-access ` check. 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst @@ -0,0 +1,30 @@ +.. title:: clang-tidy - bugprone-standalone-empty + +bugprone-standalone-empty +========================= + +Checks for inaccurate use of the ``empty()`` method. + +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 v; + ... + v.empty(); + +A call to ``clear()`` would appropriately clear the contents of the range: + +.. code-block:: c++ + + std::vector v; + ... + v.clear(); diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -115,6 +115,7 @@ `bugprone-sizeof-container `_, `bugprone-sizeof-expression `_, `bugprone-spuriously-wake-up-functions `_, + `bugprone-standalone-empty `_, "Yes" `bugprone-string-constructor `_, "Yes" `bugprone-string-integer-assignment `_, "Yes" `bugprone-string-literal-with-embedded-nul `_, 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp @@ -0,0 +1,313 @@ +// RUN: %check_clang_tidy %s bugprone-standalone-empty %t + +namespace std { +template +struct vector { + bool empty(); +}; + +template +struct vector_with_clear { + bool empty(); + void clear(); +}; + +template +struct vector_with_void_empty { + void empty(); + void clear(); +}; + +template +struct vector_with_int_empty { + int empty(); + void clear(); +}; + +template +bool empty(T &&); + +} // namespace std + +namespace absl { +struct string { + bool empty(); +}; + +struct string_with_clear { + bool empty(); + void clear(); +}; + +struct string_with_void_empty { + void empty(); + void clear(); +}; + +struct string_with_int_empty { + int empty(); + void clear(); +}; + +template +bool empty(T &&); +} // namespace absl + +namespace test { +template +void empty(T &&); +} // namespace test + +int test_member_empty() { + { + std::vector v; + + v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + } + + { + std::vector_with_void_empty v; + + v.empty(); + // no-warning + } + + { + std::vector_with_clear v; + + v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} v.clear();{{$}} + } + + { + std::vector_with_int_empty v; + + v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} v.clear();{{$}} + } + + { + absl::string s; + + s.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + } + + { + absl::string_with_void_empty s; + + s.empty(); + // no-warning + } + + { + absl::string_with_clear s; + + s.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } + + { + absl::string_with_int_empty s; + + s.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } +} + +int test_qualified_empty() { + { + absl::string_with_clear v; + + std::empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty', did you mean 'clear()'? [bugprone-standalone-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 + } + + { + absl::string s; + + std::empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + } +} + +int test_unqualified_empty() { + { + std::vector v; + + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + } + + { + std::vector_with_void_empty v; + + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} v.clear();{{$}} + } + + { + std::vector_with_clear v; + + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} v.clear();{{$}} + } + + { + std::vector_with_int_empty v; + + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} v.clear();{{$}} + } + + { + absl::string s; + + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty' [bugprone-standalone-empty] + } + + { + absl::string_with_void_empty s; + + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } + + { + absl::string_with_clear s; + + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } + + { + absl::string_with_int_empty s; + + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } + + { + std::vector v; + using std::empty; + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + } + + { + std::vector_with_clear v; + using std::empty; + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} v.clear();{{$}} + } + + { + absl::string s; + using absl::empty; + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty' [bugprone-standalone-empty] + } + + { + absl::string_with_clear s; + using absl::empty; + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } +} + +int test_empty_method_expressions() { + std::vector v; + + bool EmptyReturn(v.empty()); + // no-warning + + (void)v.empty(); + // no-warning + + // Don't warn in the if condition. + if (v.empty()) v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + + // Don't warn in the for condition. + for(v.empty();v.empty();v.empty()) v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + // CHECK-MESSAGES: :[[@LINE-3]]:38: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + + // Don't warn in the while condition. + while(v.empty()) v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + + // Don't warn in the do-while condition. + do v.empty(); while(v.empty()); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + + // Don't warn in the switch expression. + switch(v.empty()) { + // no-warning + case true: + v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + } + + // Don't warn in the return expression, which is the last statement. + bool StmtExprReturn = ({v.empty(); v.empty();}); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] +} + +int test_empty_expressions() { + absl::string v; + + bool test(std::empty(v)); + // no-warning + + (void)std::empty(v); + // no-warning + + if (std::empty(v)) std::empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + + for(std::empty(v);std::empty(v);std::empty(v)) std::empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + // CHECK-MESSAGES: :[[@LINE-2]]:35: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + // CHECK-MESSAGES: :[[@LINE-3]]:50: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + + while(std::empty(v)) std::empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + + do std::empty(v); while(std::empty(v)); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + + switch(std::empty(v)) { + // no-warning + case true: + std::empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + } + + bool StmtExprReturn = ({std::empty(v); std::empty(v);}); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] +}