Create unused non-trivially-destructible check in clang-tidy

[ankineri]

Summary

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();
}

[njames93]

Clang already has a warning -Wunused-variable that is designed for this specific purpose. So unless this is bringing anything enhanced functionality I don't see the need for this check.

[ankineri]

-Wunused-variable does not detect unused non-trivially-destructible objects because they may be used for RAII, i.e.

{
  scoped_lock lock(&global_mutex);
  critical_section();
}

lock here is not an unused variable because its destructor side-effects are its usage. However if replaced with say absl::Status or even std::string it will be more or less obvious that the variable is unused, but the warning will still not be triggered (as both absl::Status and std::string have nontrivial destructors). This is where this checks is useful: for non-trivially-destructible types known not to be used in RAII.

[sammccall]

Extending --Wunused-variable to other types seems preferable in a few ways:

• clang warnings are more accessible than clang-tidy checks
• avoids duplicating implementation
• places description of the types along with the types, instead of in clang-tidy config
• avoids divergent ux between sources of warnings

In fact it looks like an attribute for this already exists: it can be spelled __attribute__((warn_unused)) or [[gnu::warn_unused]]

struct __attribute__((warn_unused)) S { ~S(); };
void foo() { S s; }

$ clang -fsyntax-only ~/test.cc -Wall
/usr/local/google/home/sammccall/test.cc:4:5: warning: unused variable 's' [-Wunused-variable]
  S s;

[ankineri]

I do agree that having it as part of -Wunused-variable is better on many aspects, but I have a few concerns about it.
The proposed check can be applied to any type including STL ones such as std::string, all as per codebase owner's choice. Adding this attribute to STL classes will probably be really hard as it will break many builds.
Having warn_unused absl::Status (and a couple of others) is probably a good idea anyway, but is it feasible to apply it to STL?

In other words, this check is:

• Configurable per build (which types to check for)
• Applicable to types of libraries not owned by the developer
• Likely breaking far fewer build targets
• Is decoupled from trivially-destructible objects (i.e. with -Wunused-variable types like int fall into the same category as [[warn_unused]]-marked ones).

[sammccall]

In D138583#3949604, @ankineri wrote:

I do agree that having it as part of -Wunused-variable is better on many aspects, but I have a few concerns about it.
The proposed check can be applied to any type including STL ones such as std::string, all as per codebase owner's choice. Adding this attribute to STL classes will probably be really hard as it will break many builds.
Having warn_unused absl::Status (and a couple of others) is probably a good idea anyway, but is it feasible to apply it to STL?

My guess is that libc++ and libstdc++ would accept such a patch *where it's safe*, since clang and gcc respectively support the attribute, but you'd have to check with them.

---

Since the standard library is mostly templates, one issue you're going to run into with say std::vector<int> is that it's only warn_unused because int is warn_unused, and this is hard to express (with attributes or otherwise).
std::string is actually the same since it's an alias for std::basic_string<char, ...>. While maybe nobody uses std::basic_string<std::mutex>, it's a bit of a grey area.

Fundamentally this is all stuff that such a clang-tidy check would have to address as well, and IMO the tools you have to address it with clang + attributes are much better than in a clang-tidy check.
(If it solves std::string and std::vector, then that might even be enough to justify adding some hacky warn_unused_recursive attribute that is equivalent to warn_unused if all type template parameters are warn-unused).

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

 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
 #include "UncheckedOptionalAccessCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnhandledExceptionAtNewCheck.h"
 #include "UnhandledSelfAssignmentCheck.h"
+#include "UnusedNtdObjectCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
         "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");
     CheckFactories.registerCheck<UseAfterMoveCheck>("bugprone-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "bugprone-virtual-near-miss");
   }
 };

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

add_clang_library(clangTidyBugproneModule
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp
   UncheckedOptionalAccessCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnhandledExceptionAtNewCheck.cpp
   UnhandledSelfAssignmentCheck.cpp
+  UnusedNtdObjectCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyCppCoreGuidelinesModule

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

+//===--- 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

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

+//===--- 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

clang-tools-extra/docs/ReleaseNotes.rst

 ^^^^^^^^^^
 
 - New :doc:`bugprone-suspicious-realloc-usage
   <clang-tidy/checks/bugprone/suspicious-realloc-usage>` check.
 
   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.
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
 - New :doc:`cppcoreguidelines-avoid-do-while
   <clang-tidy/checks/cppcoreguidelines/avoid-do-while>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-ntd-object.rst

+.. 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

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

.. csv-table::
    `bugprone-terminating-continue <bugprone/terminating-continue.html>`_, "Yes"
    `bugprone-throw-keyword-missing <bugprone/throw-keyword-missing.html>`_,
    `bugprone-too-small-loop-variable <bugprone/too-small-loop-variable.html>`_,
    `bugprone-unchecked-optional-access <bugprone/unchecked-optional-access.html>`_,
    `bugprone-undefined-memory-manipulation <bugprone/undefined-memory-manipulation.html>`_,
    `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>`_,
    `bugprone-virtual-near-miss <bugprone/virtual-near-miss.html>`_, "Yes"
    `cert-dcl21-cpp <cert/dcl21-cpp.html>`_, "Yes"
    `cert-dcl50-cpp <cert/dcl50-cpp.html>`_,
    `cert-dcl58-cpp <cert/dcl58-cpp.html>`_,
    `cert-env33-c <cert/env33-c.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-ntd-object.cpp

+// 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();
+}