[clang-tidy] New check misc-uniqueptr-release-unused-retval

[khuttun]

Summary

Detects calls to std::unique_ptr::release where the return value is unused.

[Eugene.Zelenko]

May be bugprone is better module then misc?

docs/ReleaseNotes.rst

60

Please put in list of new checks in alphabetical order.

63

Please copy first statement from documentation.

[khuttun]

In D41056#950570, @Eugene.Zelenko wrote:

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

[JonasToth]

In D41056#950605, @khuttun wrote:

In D41056#950570, @Eugene.Zelenko wrote:

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

As not reviewer I agree for it to be in bugprone.

[alexfh]

In D41056#950605, @khuttun wrote:

In D41056#950570, @Eugene.Zelenko wrote:

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

Yup, bugprone- should be a better category for this, IMO.

I wonder whether libc++ folks are interested in marking unique_ptr::release() with __attribute__ ((warn_unused_result)). A compiler warning (with -Werror) would be a better protection against this kind of a bug.

[aaron.ballman]

In D41056#951083, @alexfh wrote:

In D41056#950605, @khuttun wrote:

In D41056#950570, @Eugene.Zelenko wrote:

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

Yup, bugprone- should be a better category for this, IMO.

I wonder whether libc++ folks are interested in marking unique_ptr::release() with __attribute__ ((warn_unused_result)). A compiler warning (with -Werror) would be a better protection against this kind of a bug.

There's a push in WG21 to mark more of the library with [[nodiscard]]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0600r1.pdf

If we have a check for this, I do not think it should be specific to unique_ptr::release(), but instead be more broadly applicable to APIs that should be marked [[nodiscard]] but are not (currently). P0600R1 is a good place to start, but I'm guessing there are POSIX APIs (among others) that would also qualify.

[khuttun]

In D41056#951145, @aaron.ballman wrote:

In D41056#951083, @alexfh wrote:

In D41056#950605, @khuttun wrote:

In D41056#950570, @Eugene.Zelenko wrote:

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

Yup, bugprone- should be a better category for this, IMO.

I wonder whether libc++ folks are interested in marking unique_ptr::release() with __attribute__ ((warn_unused_result)). A compiler warning (with -Werror) would be a better protection against this kind of a bug.

There's a push in WG21 to mark more of the library with [[nodiscard]]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0600r1.pdf

If we have a check for this, I do not think it should be specific to unique_ptr::release(), but instead be more broadly applicable to APIs that should be marked [[nodiscard]] but are not (currently). P0600R1 is a good place to start, but I'm guessing there are POSIX APIs (among others) that would also qualify.

P0600R1 seems to suggest quite conservative approach (and rightly so, IMO) in taking [[nodiscard]] in use in std lib. For example it proposes *not* to use it with unique_ptr::release. I think there could still be room for static unused ret val checking for parts of std lib not covered by P0600R1, but also for boost, POSIX APIs etc.

What do you think about making this check fully configurable through the check options? The options could list the functions to check. The documentation could suggest some candidate functions from std lib to configure checks for.

[JonasToth]

What do you think about making this check fully configurable through the check options? The options could list the functions to check. The documentation could suggest some candidate functions from std lib to configure checks for.

That sound reasonable. A similar approach has been used for cppcoreguidelines-no-malloc already.

[aaron.ballman]

In D41056#951507, @khuttun wrote:

P0600R1 seems to suggest quite conservative approach (and rightly so, IMO) in taking [[nodiscard]] in use in std lib. For example it proposes *not* to use it with unique_ptr::release. I think there could still be room for static unused ret val checking for parts of std lib not covered by P0600R1, but also for boost, POSIX APIs etc.

What do you think about making this check fully configurable through the check options? The options could list the functions to check. The documentation could suggest some candidate functions from std lib to configure checks for.

Agreed; that's largely what I was suggesting with my previous comment. I think this should be a generic, configurable check that is prepopulated with some APIs that make sense. You can use P0600R1 as a starting point, but I'd encourage you to find a pretty decent (but still conservative) list of APIs.

[khuttun]

Closing this as more general check is being reviewed here: https://reviews.llvm.org/D41655

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
+  UniqueptrReleaseUnusedRetvalCheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UnusedUsingDeclsCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/misc/MiscTidyModule.cpp

 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UndelegatedConstructor.h"
+#include "UniqueptrReleaseUnusedRetvalCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 #include "UnusedUsingDeclsCheck.h"
 
 namespace clang {
 namespace tidy {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousStringCompareCheck>(
         "misc-suspicious-string-compare");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
         "misc-throw-by-value-catch-by-reference");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
+    CheckFactories.registerCheck<UniqueptrReleaseUnusedRetvalCheck>(
+        "misc-uniqueptr-release-unused-retval");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
         "misc-uniqueptr-reset-release");
     CheckFactories.registerCheck<UnusedAliasDeclsCheck>(
         "misc-unused-alias-decls");
     CheckFactories.registerCheck<UnusedParametersCheck>(
         "misc-unused-parameters");
     CheckFactories.registerCheck<UnusedRAIICheck>("misc-unused-raii");
     CheckFactories.registerCheck<UnusedUsingDeclsCheck>(

clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.h

+//===--- UniqueptrReleaseUnusedRetvalCheck.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_UNIQUEPTR_RELEASE_UNUSED_RETVAL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNIQUEPTR_RELEASE_UNUSED_RETVAL_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This check flags calls to std::unique_ptr::release with unused return value.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-uniqueptr-release-unused-retval.html
+class UniqueptrReleaseUnusedRetvalCheck : public ClangTidyCheck {
+public:
+  UniqueptrReleaseUnusedRetvalCheck(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_UNIQUEPTR_RELEASE_UNUSED_RETVAL_H

clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.cpp

+//===--- UniqueptrReleaseUnusedRetvalCheck.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 "UniqueptrReleaseUnusedRetvalCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void UniqueptrReleaseUnusedRetvalCheck::registerMatchers(MatchFinder *Finder) {
+  // match on release() calls with CompoundStmt parent (= unused)
+  auto UniquePtrType = hasType(hasCanonicalType(
+      hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom("::std::unique_ptr")))));
+  auto ReleaseMethod = cxxMethodDecl(hasName("release"));
+  auto UnusedRetVal = hasParent(compoundStmt());
+  Finder->addMatcher(
+      cxxMemberCallExpr(on(UniquePtrType), callee(ReleaseMethod), UnusedRetVal)
+          .bind("match"),
+      this);
+}
+
+void UniqueptrReleaseUnusedRetvalCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto Matched = Result.Nodes.getNodeAs<Stmt>("match")) {
+    diag(Matched->getLocStart(),
+         "unused std::unique_ptr::release return value");
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New `misc-uniqueptr-release-unused-retval
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-uniqueptr-release-unused-retval.html>`_ check
+
+  Detects calls to std::unique_ptr::release where the return value is unused.
+
 - New module `fuchsia` for Fuchsia style checks.
 
 - New module `objc` for Objective-C style checks.
 
 - New `android-cloexec-accept
   <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept.html>`_ check
 
   Detects usage of ``accept()``.

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-suspicious-enum-usage
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-suspicious-string-compare
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-unconventional-assign-operator
    misc-undelegated-constructor
+   misc-uniqueptr-release-unused-retval
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    misc-unused-using-decls
    modernize-avoid-bind
    modernize-deprecated-headers
    modernize-loop-convert

docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst

+.. title:: clang-tidy - misc-uniqueptr-release-unused-retval
+
+misc-uniqueptr-release-unused-retval
+====================================
+
+Warns if the return value of ``std::unique_ptr::release()`` is not used.
+
+Discarding the return value results in leaking the managed object, if the
+pointer isn't stored anywhere else. This can happen for example when
+``release()`` is incorrectly used instead of ``reset()``:
+
+.. code-block:: c++
+
+  void deleteObject() {
+    MyUniquePtr.release();
+  }
+
+The check will warn about this. The fix is to replace the ``release()`` call
+with ``reset()``.
+
+Discarding the ``release()`` return value doesn't necessary result in a leak if
+the pointer is also stored somewhere else:
+
+.. code-block:: c++
+
+  void f(std::unique_ptr<Foo> p) {
+    // store the raw pointer
+    storePointer(p.get());
+
+    // prevent destroying the Foo object when the unique_ptr is destructed
+    p.release();
+  }
+
+The check warns also here. Although there's no leak here, the code can still be
+improved by using the ``release()`` return value:
+
+.. code-block:: c++
+
+  void f(std::unique_ptr<Foo> p) {
+    storePointer(p.release());
+  }
+
+This eliminates the possibility that code causing ``f()`` to return, thus
+causing ``p``'s destructor to be called and making the stored raw pointer
+dangle, is added between ``storePointer()`` and ``release()`` calls.

test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp

+// RUN: %check_clang_tidy %s misc-uniqueptr-release-unused-retval %t
+
+namespace std {
+template <typename T>
+struct unique_ptr {
+  T *operator->();
+  T *release();
+};
+} // namespace std
+
+struct Foo {
+  int release();
+};
+
+template <typename T>
+void callRelease(T &t) { t.release(); }
+// CHECK-MESSAGES: [[@LINE-1]]:26: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+
+using FooPtr = std::unique_ptr<Foo>;
+
+template <typename T>
+struct Derived : public std::unique_ptr<T> {
+};
+
+void deleteThis(Foo *pointer) { delete pointer; }
+
+void Warning() {
+  std::unique_ptr<Foo> p1;
+  p1.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  { p1.release(); }
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  callRelease(p1);
+  FooPtr fp;
+  fp.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  Derived<Foo> dp;
+  dp.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+}
+
+void NoWarning() {
+  std::unique_ptr<Foo> p2;
+  auto q = p2.release();
+  delete p2.release();
+  deleteThis(p2.release());
+  p2->release();
+  p2.release()->release();
+}