[clang-tidy] Improve bugprone-unused-return-value check

Summary:
Add support for checking class template member functions.

Also add the following functions to be checked by default:

- std::unique_ptr::release
- std::basic_string::empty
- std::vector::empty

Reviewers: alexfh, hokein, aaron.ballman, ilya-biryukov

Reviewed By: aaron.ballman

Subscribers: jbcoe, xazax.hun, cfe-commits

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D45891

Patch by khuttun (Kalle Huttunen)

llvm-svn: 330772

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

@@ -19,14 +19,32 @@ namespace clang {
 namespace tidy {
 namespace bugprone {
 
+namespace {
+
+// Matches functions that are instantiated from a class template member function
+// matching InnerMatcher. Functions not instantiated from a class template
+// member function are matched directly with InnerMatcher.
+AST_MATCHER_P(FunctionDecl, isInstantiatedFrom, Matcher<FunctionDecl>,
+              InnerMatcher) {
+  FunctionDecl *InstantiatedFrom = Node.getInstantiatedFromMemberFunction();
+  return InnerMatcher.matches(InstantiatedFrom ? *InstantiatedFrom : Node,
+                              Finder, Builder);
+}
+
+} // namespace
+
 UnusedReturnValueCheck::UnusedReturnValueCheck(llvm::StringRef Name,
                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      CheckedFunctions(Options.get("CheckedFunctions", "::std::async;"
+      CheckedFunctions(Options.get("CheckedFunctions",
+                                   "::std::async;"
                                    "::std::launder;"
                                    "::std::remove;"
                                    "::std::remove_if;"
-                                                       "::std::unique")) {}
+                                   "::std::unique;"
+                                   "::std::unique_ptr::release;"
+                                   "::std::basic_string::empty;"
+                                   "::std::vector::empty")) {}
 
 void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckedFunctions", CheckedFunctions);
@@ -35,11 +53,11 @@ void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
 void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
   auto FunVec = utils::options::parseStringList(CheckedFunctions);
   auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts(
-      callExpr(
+      callExpr(callee(functionDecl(
-          callee(functionDecl(
                    // Don't match void overloads of checked functions.
-              unless(returns(voidType())), hasAnyName(std::vector<StringRef>(
+                   unless(returns(voidType())),
-                                               FunVec.begin(), FunVec.end())))))
+                   isInstantiatedFrom(hasAnyName(
+                       std::vector<StringRef>(FunVec.begin(), FunVec.end()))))))
           .bind("match"))));
 
   auto UnusedInCompoundStmt =

clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst

@@ -11,7 +11,7 @@ Options
 .. option:: CheckedFunctions
 
    Semicolon-separated list of functions to check. Defaults to
-   ``::std::async;::std::launder;::std::remove;::std::remove_if;::std::unique``.
+   ``::std::async;::std::launder;::std::remove;::std::remove_if;::std::unique;::std::unique_ptr::release;::std::basic_string::empty;::std::vector::empty``.
    This means that the calls to following functions are checked by default:
 
    - ``std::async()``. Not using the return value makes the call synchronous.
@@ -22,3 +22,10 @@ Options
      iterator indicates the boundary between elements to keep and elements to be
      removed. Not using the return value means that the information about which
      elements to remove is lost.
+   - ``std::unique_ptr::release()``. Not using the return value can lead to
+     resource leaks if the same pointer isn't stored anywhere else. Often,
+     ignoring the ``release()`` return value indicates that the programmer
+     confused the function with ``reset()``.
+   - ``std::basic_string::empty()`` and ``std::vector::empty()``. Not using the
+     return value often indicates that the programmer confused the function with
+     ``clear()``.

clang-tools-extra/test/clang-tidy/bugprone-unused-return-value-custom.cpp

@@ -1,7 +1,7 @@
 // RUN: %check_clang_tidy %s bugprone-unused-return-value %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: bugprone-unused-return-value.CheckedFunctions, \
-// RUN:    value: "::fun;::ns::Outer::Inner::memFun;::ns::Type::staticFun"}]}' \
+// RUN:    value: "::fun;::ns::Outer::Inner::memFun;::ns::Type::staticFun;::ns::ClassTemplate::memFun;::ns::ClassTemplate::staticFun"}]}' \
 // RUN: --
 
 namespace std {
@@ -34,6 +34,12 @@ struct Type {
   static Retval staticFun();
 };
 
+template <typename T>
+struct ClassTemplate {
+  Retval memFun();
+  static Retval staticFun();
+};
+
 } // namespace ns
 
 int fun();
@@ -60,6 +66,13 @@ void warning() {
 
   ns::Type::staticFun();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::ClassTemplate<int> ObjA4;
+  ObjA4.memFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::ClassTemplate<int>::staticFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 }
 
 void noWarning() {
@@ -70,6 +83,11 @@ void noWarning() {
 
   auto R3 = ns::Type::staticFun();
 
+  ns::ClassTemplate<int> ObjB2;
+  auto R4 = ObjB2.memFun();
+
+  auto R5 = ns::ClassTemplate<int>::staticFun();
+
   // test calling a void overload of a checked function
   fun(5);
 
@@ -77,6 +95,6 @@ void noWarning() {
   int I = 1;
   std::launder(&I);
 
-  ns::Type ObjB2;
+  ns::Type ObjB3;
-  ObjB2.memFun();
+  ObjB3.memFun();
 }

clang-tools-extra/test/clang-tidy/bugprone-unused-return-value.cpp

@@ -24,6 +24,34 @@ ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
 template <typename ForwardIt>
 ForwardIt unique(ForwardIt, ForwardIt);
 
+template <typename T>
+struct default_delete;
+
+template <typename T, typename Deleter = std::default_delete<T>>
+struct unique_ptr {
+  T *release() noexcept;
+};
+
+template <typename T>
+struct char_traits;
+
+template <typename T>
+struct allocator;
+
+template <typename CharT,
+          typename Traits = char_traits<CharT>,
+          typename Allocator = allocator<CharT>>
+struct basic_string {
+  bool empty() const;
+};
+
+typedef basic_string<char> string;
+
+template <typename T, typename Allocator = std::allocator<T>>
+struct vector {
+  bool empty() const noexcept;
+};
+
 // the check should be able to match std lib calls even if the functions are
 // declared inside inline namespaces
 inline namespace v1 {
@@ -64,6 +92,18 @@ void warning() {
   std::unique(nullptr, nullptr);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 
+  std::unique_ptr<Foo> UPtr;
+  UPtr.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::string Str;
+  Str.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::vector<Foo> Vec;
+  Vec.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
   // test discarding return values inside different kinds of statements
 
   auto Lambda = [] { std::remove(nullptr, nullptr, 1); };
@@ -137,6 +177,15 @@ void noWarning() {
 
   auto UniqueRetval = std::unique(nullptr, nullptr);
 
+  std::unique_ptr<Foo> UPtrNoWarning;
+  auto ReleaseRetval = UPtrNoWarning.release();
+
+  std::string StrNoWarning;
+  auto StrEmptyRetval = StrNoWarning.empty();
+
+  std::vector<Foo> VecNoWarning;
+  auto VecEmptyRetval = VecNoWarning.empty();
+
   // test using the return value in different kinds of expressions
   useFuture(std::async(increment, 42));
   std::launder(&FNoWarning)->f();