[clang-tidy] Add a check for RAII temporaries.

[bkramer]

Summary

This tries to find code similar that immediately destroys
an object that looks like it's trying to follow RAII.

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

This checker will have false positives if someone uses this pattern
to legitimately invoke a destructor immediately (or the statement is
at the end of a scope anyway). To reduce the number we ignore this
pattern in macros (this is heavily used by gtest) and ignore objects
with no user-defined destructor.

[djasper]

clang-tidy/misc/UnusedRAII.cpp

51 ↗

(On Diff #11741)

Maybe: "object destroyed immediately after creation; did .."?

Should we suggestion a fix? E.g. insert "give_me_a_name"?

test/clang-tidy/misc-unused-raii.cpp

21 ↗

(On Diff #11741)

Add a test with a templated RAII class.

Also, make this "void main()" (or "void f()") or return something. We should try to get these tests warning free.

[bkramer]

• Reworded warning message.
• Added a test case with a templated RAII object, renamed main method.
• No FixItHint because we don't have a source location for the place where the name would go :(

[bkramer]

• Added FixIts. This is tricky because we have to remove the parens when encountering Foo(); // Foo is a type. -> Foo give_me_a_name;

otherwise it'll parse as a function decl.

• Added another way of reducing false positives: If the expression is the last one in

the compoundStmt we don't warn, as it doesn't change behavior.

[djasper]

I think, this looks good as a first version.

As discussed offline, follow-ups could:

• Ignore objects that are known to have an empty destructor.
• Ignore constructors with more than one argument (reducing false positives at the cost of false negatives).
• Distinguish the case of attempted delegated constructors (consider adding a test now).

clang-tidy/misc/UnusedRAII.cpp

53 ↗

(On Diff #11747)

Add a test for this code path.

72 ↗

(On Diff #11747)

Hm. Fix-It-wise, wouldn't it be better to create a single replacement instead of an insertion and a removal?

clang-tidy/misc/UnusedRAII.h

36 ↗

(On Diff #11747)

nit: missing period at the end of the sentence.

[bkramer]

Closed by commit rL213647 (authored by d0k).

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   BoolPointerImplicitConversion.cpp
   MiscTidyModule.cpp
   RedundantSmartptrGet.cpp
   SwappedArgumentsCheck.cpp
+  UnusedRAII.cpp
   UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "BoolPointerImplicitConversion.h"
 #include "RedundantSmartptrGet.h"
 #include "SwappedArgumentsCheck.h"
+#include "UnusedRAII.h"
 #include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "misc-argument-comment",
         new ClangTidyCheckFactory<ArgumentCommentCheck>());
     CheckFactories.addCheckFactory(
         "misc-bool-pointer-implicit-conversion",
         new ClangTidyCheckFactory<BoolPointerImplicitConversion>());
     CheckFactories.addCheckFactory(
         "misc-redundant-smartptr-get",
         new ClangTidyCheckFactory<RedundantSmartptrGet>());
     CheckFactories.addCheckFactory(
         "misc-swapped-arguments",
         new ClangTidyCheckFactory<SwappedArgumentsCheck>());
     CheckFactories.addCheckFactory(
+        "misc-unused-raii",
+        new ClangTidyCheckFactory<UnusedRAIICheck>());
+    CheckFactories.addCheckFactory(
         "misc-use-override",
         new ClangTidyCheckFactory<UseOverride>());
   }
 };
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<MiscModule>
 X("misc-module", "Adds miscellaneous lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the MiscModule.
 volatile int MiscModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h

+//===--- UnusedRAII.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_UNUSED_RAII_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_RAII_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Finds temporaries that look like RAII objects.
+///
+/// The canonical example for this is a scoped lock.
+/// \code
+///   {
+///     scoped_lock(&global_mutex);
+///     critical_section();
+///   }
+/// \endcode
+/// The destructor of the scoped_lock is called before the critical_section is
+/// entered, leaving it unprotected.
+///
+/// We apply a number of heuristics to reduce the false positive count of this
+/// check:
+///   - Ignore code expanded from macros. Testing frameworks make heavy use of
+///     this.
+///   - Ignore types with no user-declared constructor. Those are very unlikely
+///     to be RAII objects.
+///   - Ignore objects at the end of a compound statement (doesn't change behavior).
+///   - Ignore objects returned from a call.
+class UnusedRAIICheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_RAII_H

clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp

+//===--- UnusedRAII.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 "UnusedRAII.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace ast_matchers {
+AST_MATCHER(CXXRecordDecl, hasUserDeclaredDestructor) {
+  // TODO: If the dtor is there but empty we don't want to warn either.
+  return Node.hasUserDeclaredDestructor();
+}
+} // namespace ast_matchers
+
+namespace tidy {
+
+void UnusedRAIICheck::registerMatchers(MatchFinder *Finder) {
+  // Look for temporaries that are constructed in-place and immediately
+  // destroyed. Look for temporaries created by a functional cast but not for
+  // those returned from a call.
+  auto BindTemp = bindTemporaryExpr(unless(has(callExpr()))).bind("temp");
+  Finder->addMatcher(
+      exprWithCleanups(
+          unless(hasAncestor(decl(
+              anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),
+                    functionDecl(ast_matchers::isTemplateInstantiation()))))),
+          hasParent(compoundStmt().bind("compound")),
+          hasDescendant(typeLoc().bind("typeloc")),
+          hasType(recordDecl(hasUserDeclaredDestructor())),
+          anyOf(has(BindTemp), has(functionalCastExpr(has(BindTemp)))))
+          .bind("expr"),
+      this);
+}
+
+void UnusedRAIICheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *E = Result.Nodes.getStmtAs<Expr>("expr");
+
+  // We ignore code expanded from macros to reduce the number of false
+  // positives.
+  if (E->getLocStart().isMacroID())
+    return;
+
+  // Don't emit a warning for the last statement in the surrounding compund
+  // statement.
+  const auto *CS = Result.Nodes.getStmtAs<CompoundStmt>("compound");
+  if (E == CS->body_back())
+    return;
+
+  // Emit a warning.
+  auto D = diag(E->getLocStart(), "object destroyed immediately after "
+                                  "creation; did you mean to name the object?");
+  const char *Replacement = " give_me_a_name";
+
+  // If this is a default ctor we have to remove the parens or we'll introduce a
+  // most vexing parse.
+  const auto *BTE = Result.Nodes.getStmtAs<CXXBindTemporaryExpr>("temp");
+  if (const auto *TOE = dyn_cast<CXXTemporaryObjectExpr>(BTE->getSubExpr()))
+    if (TOE->getNumArgs() == 0) {
+      D << FixItHint::CreateReplacement(
+          CharSourceRange::getTokenRange(TOE->getParenOrBraceRange()),
+          Replacement);
+      return;
+    }
+
+  // Otherwise just suggest adding a name.
+  const auto *TL = Result.Nodes.getNodeAs<TypeLoc>("typeloc");
+  D << FixItHint::CreateInsertion(
+      Lexer::getLocForEndOfToken(TL->getLocEnd(), 0, *Result.SourceManager,
+                                 Result.Context->getLangOpts()),
+      Replacement);
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/test/clang-tidy/misc-unused-raii.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-unused-raii %t
+// REQUIRES: shell
+
+struct Foo {
+  Foo();
+  Foo(int);
+  Foo(int, int);
+  ~Foo();
+};
+
+struct Bar {
+  Bar();
+  Foo f;
+};
+
+template <typename T>
+void qux() {
+  T(42);
+}
+
+template <typename T>
+struct TFoo {
+  TFoo(T);
+  ~TFoo();
+};
+
+Foo f();
+
+struct Ctor {
+  Ctor(int);
+  Ctor() {
+    Ctor(0); // TODO: warn here.
+  }
+};
+
+void test() {
+  Foo(42);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+// CHECK-FIXES: Foo give_me_a_name(42);
+  Foo(23, 42);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+// CHECK-FIXES: Foo give_me_a_name(23, 42);
+  Foo();
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+// CHECK-FIXES: Foo give_me_a_name;
+  TFoo<int>(23);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+// CHECK-FIXES: TFoo<int> give_me_a_name(23);
+
+  Bar();
+  f();
+  qux<Foo>();
+
+#define M Foo();
+  M
+
+  {
+    Foo();
+  }
+  Foo();
+}