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

[khuttun]

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

[aaron.ballman]

LGTM!

[khuttun]

Could someone help getting this merged? I don't have commit access to the repo.

[jbcoe]

I can merge this for you. Congratulations on the commit!

[khuttun]

Thank you!

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

 
 using namespace clang::ast_matchers;
 using namespace clang::ast_matchers::internal;
 
 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;"
-                                                       "::std::launder;"
-                                                       "::std::remove;"
-                                                       "::std::remove_if;"
-                                                       "::std::unique")) {}
+      CheckedFunctions(Options.get("CheckedFunctions",
+                                   "::std::async;"
+                                   "::std::unique;"
+                                   "::std::unique_ptr::release;"
+                                   "::std::basic_string::empty;"
+                                   "::std::vector::empty")) {}
 
 void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckedFunctions", CheckedFunctions);
 }
 
 void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
   auto FunVec = utils::options::parseStringList(CheckedFunctions);
   auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts(
-      callExpr(
-          callee(functionDecl(
+      callExpr(callee(functionDecl(
                    // Don't match void overloads of checked functions.
-              unless(returns(voidType())), hasAnyName(std::vector<StringRef>(
-                                               FunVec.begin(), FunVec.end())))))
+                   unless(returns(voidType())),
+                   isInstantiatedFrom(hasAnyName(
+                       std::vector<StringRef>(FunVec.begin(), FunVec.end()))))))
           .bind("match"))));
 
   auto UnusedInCompoundStmt =
       compoundStmt(forEach(MatchedCallExpr),
                    // The checker can't currently differentiate between the
                    // return statement and other statements inside GNU statement
                    // expressions, so disable the checker inside them to avoid
                    // false positives.

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

 .. title:: clang-tidy - bugprone-unused-return-value
 
 bugprone-unused-return-value
 ============================
 
 Warns on unused function return values. The checked funtions can be configured.
 
 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.
    - ``std::launder()``. Not using the return value usually means that the
      function interface was misunderstood by the programmer. Only the returned
      pointer is "laundered", not the argument.
    - ``std::remove()``, ``std::remove_if()`` and ``std::unique()``. The returned
      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()``.

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

 // 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 {
 
 template <typename T>
 T *launder(T *);
 
 } // namespace std
struct Retval {
   ~Retval() { delete P; }
 };
 
 struct Type {
   Retval memFun();
   static Retval staticFun();
 };
 
+template <typename T>
+struct ClassTemplate {
+  Retval memFun();
+  static Retval staticFun();
+};
+
 } // namespace ns
 
 int fun();
 void fun(int);
 
 void warning() {
   fun();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
void warning() {
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 
   ns::Derived ObjA3;
   ObjA3.memFun();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 
   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() {
   auto R1 = fun();
 
   ns::Outer::Inner ObjB1;
   auto R2 = ObjB1.memFun();
 
   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);
 
   // test discarding return value of functions that are not configured to be checked
   int I = 1;
   std::launder(&I);
 
-  ns::Type ObjB2;
-  ObjB2.memFun();
+  ns::Type ObjB3;
+  ObjB3.memFun();
 }

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

 ForwardIt remove(ForwardIt, ForwardIt, const T &);
 
 template <typename ForwardIt, typename UnaryPredicate>
 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 {
 
 template <typename T>
 T *launder(T *);
 
 } // namespace v1
void warning() {
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 
   std::remove_if(nullptr, nullptr, nullptr);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 
   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); };
   // CHECK-MESSAGES: [[@LINE-1]]:22: warning: the value returned by this function should be used [bugprone-unused-return-value]
 
   if (true)
     std::remove(nullptr, nullptr, 1);
   // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
void noWarning() {
   auto LaunderRetval = std::launder(&FNoWarning);
 
   auto RemoveRetval = std::remove(nullptr, nullptr, 1);
 
   auto RemoveIfRetval = std::remove_if(nullptr, nullptr, nullptr);
 
   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();
   delete std::launder(&FNoWarning);
 
   if (std::launder(&FNoWarning))
     ;
   for (; std::launder(&FNoWarning);)