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 @@ -70,6 +70,7 @@ #include "UndelegatedConstructorCheck.h" #include "UnhandledExceptionAtNewCheck.h" #include "UnhandledSelfAssignmentCheck.h" +#include "UnusedNtdObjectCheck.h" #include "UnusedRaiiCheck.h" #include "UnusedReturnValueCheck.h" #include "UseAfterMoveCheck.h" @@ -198,6 +199,8 @@ "bugprone-unhandled-self-assignment"); CheckFactories.registerCheck( "bugprone-unhandled-exception-at-new"); + CheckFactories.registerCheck( + "bugprone-unused-ntd-object"); CheckFactories.registerCheck("bugprone-unused-raii"); CheckFactories.registerCheck( "bugprone-unused-return-value"); 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 @@ -66,6 +66,7 @@ UndelegatedConstructorCheck.cpp UnhandledExceptionAtNewCheck.cpp UnhandledSelfAssignmentCheck.cpp + UnusedNtdObjectCheck.cpp UnusedRaiiCheck.cpp UnusedReturnValueCheck.cpp UseAfterMoveCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.h @@ -0,0 +1,42 @@ +//===--- UnusedNtdObjectCheck.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_UNUSEDNTDOBJECTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDNTDOBJECTCHECK_H + +#include "../ClangTidyCheck.h" +#include + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Checks for unused objects of non-trivially-destructible (ntd) types +/// which are unlikely to be used for RAII. Trivially destructible objects are +/// covered with -Wunused, but ntd objects don't cause this warning due to +/// destructor side-effects. +/// One important ntd type is absl::Status. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unused-ntd-object.html +class UnusedNtdObjectCheck : public ClangTidyCheck { +public: + UnusedNtdObjectCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + std::string CheckedTypes; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDNTDOBJECTCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.cpp @@ -0,0 +1,169 @@ +//===--- UnusedNtdObjectCheck.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 "UnusedNtdObjectCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +using namespace clang::ast_matchers; + +namespace { +AST_MATCHER(VarDecl, isLocal) { return Node.isLocalVarDecl(); } +AST_MATCHER_P(DeclStmt, containsAnyDeclaration, + ast_matchers::internal::Matcher, InnerMatcher) { + return ast_matchers::internal::matchesFirstInPointerRange( + InnerMatcher, Node.decl_begin(), Node.decl_end(), Finder, + Builder) != Node.decl_end(); +} +} // namespace + +UnusedNtdObjectCheck::UnusedNtdObjectCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + CheckedTypes(Options.get("CheckedTypes", "::absl::Status")) {} + +void UnusedNtdObjectCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckedTypes", CheckedTypes); +} + +void UnusedNtdObjectCheck::registerMatchers(MatchFinder *Finder) { + auto TypesVec = utils::options::parseStringList(CheckedTypes); + auto LocalValDecl = + varDecl(allOf(isLocal(), hasType(recordDecl(hasAnyName(TypesVec))))); + const auto FunctionScope = functionDecl(hasBody( + compoundStmt( + forEachDescendant( + declStmt(containsAnyDeclaration(LocalValDecl.bind("local-value")), + unless(has(decompositionDecl()))) + .bind("decl-stmt"))) + .bind("scope"))); + Finder->addMatcher(FunctionScope, this); +} + +/// Traverses AST looking for variable reads after each write. +/// If at least once the variable has not been read IsUnused() returns true. +class UnusedVariableVisitor + : public RecursiveASTVisitor { +public: + /// Initializes UnusedVariableVisitor + /// \param VariableName the variable name to look for + UnusedVariableVisitor(StringRef VariableName) : VariableName(VariableName) {} + + bool TraverseStmt(Stmt *Stmt) { + if (Stmt == nullptr) { + return true; + } + // If a class does not declare operator=, assignments will be + // BinaryOperators. + if (Stmt->getStmtClass() == Stmt::BinaryOperatorClass) { + auto AsBinaryOperator = static_cast(Stmt); + if (AsBinaryOperator->getOpcode() == clang::BO_Assign) { + auto LHS = AsBinaryOperator->getLHS(); + auto RHS = AsBinaryOperator->getRHS(); + auto ProcessingResult = ProcessAssignmentOperator(LHS, RHS); + if (ProcessingResult.has_value()) { + return ProcessingResult.value(); + } + } + } + + // If a class does declare operator=, assignments will be + // CXXOperatorCallExpr. + if (Stmt->getStmtClass() == Stmt::CXXOperatorCallExprClass) { + auto AsCxxOperator = (CXXOperatorCallExpr *)Stmt; + if (AsCxxOperator->isAssignmentOp() && AsCxxOperator->getNumArgs() == 2) { + auto LHS = AsCxxOperator->getArg(0); + auto RHS = AsCxxOperator->getArg(1); + auto ProcessingResult = ProcessAssignmentOperator(LHS, RHS); + if (ProcessingResult.has_value()) { + return ProcessingResult.value(); + } + } + } + + // If a value is used, we treat it as a read operation. + if (Stmt->getStmtClass() == Stmt::DeclRefExprClass) { + auto AsDeclRef = static_cast(Stmt); + auto Identifier = AsDeclRef->getDecl()->getIdentifier(); + if (Identifier != nullptr && Identifier->getName() == VariableName) { + FoundUsage = true; + } + } + RecursiveASTVisitor::TraverseStmt(Stmt); + + return true; + } + + /// After traversing the AST this returns whether VariableName was unused in + /// AST \return true if the variable was unused + bool IsUnused() { return UnusedInAssign || (!FoundUsage); } + +private: + StringRef VariableName; + bool FoundUsage = false; + + // If we had at least one assignment before which the value was not read. + bool UnusedInAssign = false; + + /// Processes an assignment operator. If lhs is the `varname` variable, it + /// constitues a write operation, and the value must have been used before. + /// \param LHS the left-hand side of the operator + /// \param RHS the right-hand side of the operator + /// \return false if an unused scenario was found; true if processing of this + /// AST node is finished; nullopt if this node needs further processing. + std::optional ProcessAssignmentOperator(Stmt *LHS, Stmt *RHS) { + if (LHS->getStmtClass() == Stmt::DeclRefExprClass) { + auto LHSAsDeclRef = static_cast(LHS); + if (LHSAsDeclRef->getDecl()->getIdentifier() != nullptr && + LHSAsDeclRef->getDecl()->getIdentifier()->getName() == VariableName) { + if (!FoundUsage) { + UnusedInAssign = true; + return std::make_optional(false); + } + // Variable was assigned to, need to find another usage. + FoundUsage = false; + } + RecursiveASTVisitor::TraverseStmt(RHS); + return std::make_optional(true); + } + return std::nullopt; + } +}; + +void UnusedNtdObjectCheck::check(const MatchFinder::MatchResult &Result) { + const auto *LocalScope = Result.Nodes.getNodeAs("scope"); + const auto *Variable = Result.Nodes.getNodeAs("local-value"); + + // unused attribute suppresses the warning + if (Variable->hasAttr()) { + return; + } + if (!Variable->getIdentifier()) { + return; + } + + UnusedVariableVisitor Visitor(Variable->getIdentifier()->getName().str()); + Visitor.TraverseCompoundStmt(const_cast(LocalScope), nullptr); + if (!Visitor.IsUnused()) { + return; + } + + diag(Variable->getLocation(), + "%0 is unlikely to be RAII and is potentially unused") + << Variable; +} + +} // 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 @@ -105,6 +105,11 @@ Finds usages of ``realloc`` where the return value is assigned to the same expression as passed to the first argument. +- New :doc:`bugprone-unused-ntd-object + ` check. + + Finds unused variables with nontrivial destructors that are unlikely to be used as RAII. + - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-ntd-object.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-ntd-object.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-ntd-object.rst @@ -0,0 +1,33 @@ +.. title:: clang-tidy - bugprone-unused-ntd-object + +bugprone-unused-ntd-object +========================== + +Finds unused variables with nontrivial destructors that are unlikely to be used +as RAII. + +Options +------- + +.. option:: CheckedTypes + + Semicolon-separated list of types to check. The variable is checked if + the its type is from ``CheckedTypes``. + By default the following types are checked: + ``absl::Status`` + +By default unused variables are reported only if they have trivial destructors, +otherwise the destructor call is a usage of the variable, a pattern used in RAII. + +Some objects however have destructors because of internal state management, not +to enable RAII. One such examle is ``absl::Status``. This class has reference counting (and thus a non-trivial destructor), but it is a "meaningless" usage. Consider the following code. + +.. code-block:: c++ + + { + absl::Status status = call_some_function(); + } // status.~Status() is called here + +This does not cause unused variable warning, but is likely to contain a bug. + +This check is not absolutely precise and aims to capture the most common scenarios like the one above. \ No newline at end of file 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 @@ -136,6 +136,7 @@ `bugprone-undelegated-constructor `_, `bugprone-unhandled-exception-at-new `_, `bugprone-unhandled-self-assignment `_, + `bugprone-unused-ntd-object `_, `bugprone-unused-raii `_, "Yes" `bugprone-unused-return-value `_, `bugprone-use-after-move `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-ntd-object.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-ntd-object.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-ntd-object.cpp @@ -0,0 +1,64 @@ +// RUN: %check_clang_tidy %s bugprone-unused-ntd-object %t +namespace absl { +class Status { +public: + bool ok() {return true;} +}; +} +bool simple_used_value() { + absl::Status status; + return status.ok(); +} + +bool if_used_value() { + absl::Status status; + if (status.ok()) { + return true; + } + return false; +} + +void accepts_status(absl::Status status) { +} + +void used_by_function() { + absl::Status status; + accepts_status(status); +} + +int value; +int& accepts_status_returns_ref(absl::Status status) { + return value; +} + +int* accepts_status_returns_ptr(absl::Status status) { + return &value; +} + + +void used_assign_lhs() { + absl::Status for_ref; + accepts_status_returns_ref(for_ref) = 7; + absl::Status for_ptr; + *accepts_status_returns_ptr(for_ptr) = 42; +} + +void unused_simple() { + absl::Status unused; +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'unused' is unlikely to be RAII and is potentially unused [bugprone-unused-ntd-object] +} + +void unused_reassigned() { + absl::Status unused; +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'unused' is unlikely to be RAII and is potentially unused [bugprone-unused-ntd-object] + unused = absl::Status(); +} + +void unused_checked_reassigned() { + absl::Status unused; +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'unused' is unlikely to be RAII and is potentially unused [bugprone-unused-ntd-object] + if (!unused.ok()) { + return; + } + unused = absl::Status(); +}