[clang-tidy] Add a check for constructing non-trivial types without assigning to a variable

[pfultz2]

Summary

Diagnoses when a non-trivial type is not assigned to a variable. This is useful to check for problems like unnamed lock guards.

For example, if you had code written like this:

int g_i = 0;
std::mutex g_i_mutex;  // protects g_i
 
void safe_increment()
{
  std::lock_guard<std::mutex>{g_i_mutex};
  // The lock is locked and then unlocked before reaching here
  ++g_i;
}

This will warn that a variable should be created instead:

int g_i = 0;
std::mutex g_i_mutex;  // protects g_i
 
void safe_increment()
{
  std::lock_guard<std::mutex> lock{g_i_mutex};
  // The lock is locked and then will be unlocked when exiting scope
  ++g_i;
}

[lebedev.ri]

I'd like to see some more tests

void valid() {
  g(NonTrivial{}); // Should not warn
}

Have you already tried running it on some C++ codebase (llvm, qt, boost, ...) ?
What are the results?

[JonasToth]

The check is related to: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#cp44-remember-to-name-your-lock_guards-and-unique_locks

If you want, you can add an alias into the cppcoreguideline module.

[alexfh]

How does this differ from misc-unused-raii?

[pfultz2]

How does this differ from misc-unused-raii?

Actually, I was unaware of this check. This seems like a better place to start. Whats missing from the misc-unused-raii is that it doesn't catch it when constructing with curly braces(ie NonTrivial{}). Let me work on updating that check instead.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   MisplacedConstCheck.cpp
+  OwnerlessResourceCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   DefinitionsInHeadersCheck.cpp
   MacroParenthesesCheck.cpp
   MiscTidyModule.cpp
   NewDeleteOverloadsCheck.cpp
   NonCopyableObjects.cpp
   RedundantExpressionCheck.cpp
   SizeofContainerCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NonCopyableObjects.h"
+#include "OwnerlessResourceCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 #include "UnusedUsingDeclsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
+    CheckFactories.registerCheck<OwnerlessResourceCheck>(
+        "misc-ownerless-resource");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "misc-macro-parentheses");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");

clang-tidy/misc/OwnerlessResourceCheck.h

+//===--- OwnerlessResourceCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OWNERLESSRESOURCECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OWNERLESSRESOURCECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Diagnoses when a non-trivial type is not assigned to a variable.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-ownerless-resource.html
+class OwnerlessResourceCheck : public ClangTidyCheck {
+public:
+  OwnerlessResourceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OWNERLESSRESOURCECHECK_H

clang-tidy/misc/OwnerlessResourceCheck.cpp

+//===--- OwnerlessResourceCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "OwnerlessResourceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void OwnerlessResourceCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      exprWithCleanups(unless(hasType(voidType())),
+                       has(cxxBindTemporaryExpr(has(cxxTemporaryObjectExpr()))))
+          .bind("ownerless-resource"),
+      this);
+}
+
+void OwnerlessResourceCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *E = Result.Nodes.getNodeAs<Expr>("ownerless-resource")) {
+    diag(E->getLocStart(), "Resource has no owner; Did you mean to declare it "
+                           "with a variable name?");
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New `misc-ownerless-resource
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-ownerless-resource.html>`_ check
+
+  Diagnoses when a non-trivial type is not assigned to a variable. This is
+  useful to check for problems like unamed lock guards.
+
 - New `bugprone-throw-keyword-missing
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-throw-keyword-missing.html>`_ check
 
   Diagnoses when a temporary object that appears to be an exception is
   constructed but not thrown.
 
 - New `cppcoreguidelines-avoid-goto
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ check

docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-definitions-in-headers
    misc-macro-parentheses
    misc-misplaced-const
    misc-new-delete-overloads
    misc-non-copyable-objects
+   misc-ownerless-resource
    misc-redundant-expression
    misc-sizeof-container
    misc-sizeof-expression
    misc-static-assert
    misc-throw-by-value-catch-by-reference
    misc-unconventional-assign-operator
    misc-uniqueptr-reset-release
    misc-unused-alias-decls

docs/clang-tidy/checks/misc-ownerless-resource.rst

+.. title:: clang-tidy - misc-ownerless-resource
+
+misc-ownerless-resource
+=======================
+
+Diagnoses when a non-trivial type is not assigned to a variable. This is useful to check for problems like unamed lock guards.
+
+For example, if you had code written like this:
+
+.. code-block:: c++
+
+  int g_i = 0;
+  std::mutex g_i_mutex;  // protects g_i
+   
+  void safe_increment()
+  {
+    std::lock_guard<std::mutex>{g_i_mutex};
+    // The lock is locked and then unlocked before reaching here
+    ++g_i;
+  }
+
+This will warn that a variable should be created instead:
+
+.. code-block:: c++
+
+  int g_i = 0;
+  std::mutex g_i_mutex;  // protects g_i
+   
+  void safe_increment()
+  {
+    std::lock_guard<std::mutex> lock{g_i_mutex};
+    // The lock is locked and then will be unlocked when exiting scope
+    ++g_i;
+  }

test/clang-tidy/misc-ownerless-resource.cpp

+// RUN: %check_clang_tidy %s misc-ownerless-resource %t
+
+struct Trivial {};
+
+struct NonTrivial {
+  int *mem;
+  NonTrivial()
+      : mem(new int()) {}
+  NonTrivial(int)
+      : mem(new int()) {}
+  ~NonTrivial() {
+    delete mem;
+  }
+};
+
+NonTrivial f() { return {}; }
+void g(NonTrivial) {}
+
+void fail() {
+  NonTrivial{};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Resource has no owner; Did you mean to declare it with a variable name? [misc-ownerless-resource]
+  NonTrivial{1};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Resource has no owner; Did you mean to declare it with a variable name? [misc-ownerless-resource]
+}
+
+void valid() {
+  NonTrivial a{};
+  auto b = NonTrivial{};
+
+  g(f());
+  f();
+
+  auto c = f();
+  Trivial{};
+  Trivial x{};
+}