ankineri created this revision.
ankineri added a reviewer: sammccall.
Herald added a subscriber: carlosgalvezp.
Herald added a reviewer: njames93.
Herald added a project: All.
ankineri requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Adds a check to clang-tidy to ensure that some (`absl::Status` by default)
non-trivially-destructible objects are used.
Example code that should trigger a warning:
{
absl::Status status = call_some_function();
}
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D138583
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.cpp
clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-ntd-object.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-ntd-object.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-ntd-object.cpp
===================================================================
--- /dev/null
+++ 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();
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -136,6 +136,7 @@
`bugprone-undelegated-constructor <bugprone/undelegated-constructor.html>`_,
`bugprone-unhandled-exception-at-new <bugprone/unhandled-exception-at-new.html>`_,
`bugprone-unhandled-self-assignment <bugprone/unhandled-self-assignment.html>`_,
+ `bugprone-unused-ntd-object <bugprone/unused-ntd-object.html>`_,
`bugprone-unused-raii <bugprone/unused-raii.html>`_, "Yes"
`bugprone-unused-return-value <bugprone/unused-return-value.html>`_,
`bugprone-use-after-move <bugprone/use-after-move.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-ntd-object.rst
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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
+ <clang-tidy/checks/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
<clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
Index: clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.h
===================================================================
--- /dev/null
+++ 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 <clang/Analysis/Analyses/ExprMutationAnalyzer.h>
+
+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
Index: clang-tools-extra/clang-tidy/bugprone/UnusedNtdObjectCheck.cpp
===================================================================
--- /dev/null
+++ 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<Decl>, 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<UnusedVariableVisitor> {
+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<BinaryOperator *>(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<DeclRefExpr *>(Stmt);
+ auto Identifier = AsDeclRef->getDecl()->getIdentifier();
+ if (Identifier != nullptr && Identifier->getName() == VariableName) {
+ FoundUsage = true;
+ }
+ }
+ RecursiveASTVisitor<UnusedVariableVisitor>::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<bool> ProcessAssignmentOperator(Stmt *LHS, Stmt *RHS) {
+ if (LHS->getStmtClass() == Stmt::DeclRefExprClass) {
+ auto LHSAsDeclRef = static_cast<DeclRefExpr *>(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<UnusedVariableVisitor>::TraverseStmt(RHS);
+ return std::make_optional(true);
+ }
+ return std::nullopt;
+ }
+};
+
+void UnusedNtdObjectCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *LocalScope = Result.Nodes.getNodeAs<CompoundStmt>("scope");
+ const auto *Variable = Result.Nodes.getNodeAs<VarDecl>("local-value");
+
+ // unused attribute suppresses the warning
+ if (Variable->hasAttr<UnusedAttr>()) {
+ return;
+ }
+ if (!Variable->getIdentifier()) {
+ return;
+ }
+
+ UnusedVariableVisitor Visitor(Variable->getIdentifier()->getName().str());
+ Visitor.TraverseCompoundStmt(const_cast<CompoundStmt *>(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
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ 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
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ 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<UnhandledExceptionAtNewCheck>(
"bugprone-unhandled-exception-at-new");
+ CheckFactories.registerCheck<UnusedNtdObjectCheck>(
+ "bugprone-unused-ntd-object");
CheckFactories.registerCheck<UnusedRaiiCheck>("bugprone-unused-raii");
CheckFactories.registerCheck<UnusedReturnValueCheck>(
"bugprone-unused-return-value");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
