[clang-tidy] Check for specific return types on all functions

[chaitanyav]

Summary

Extend the check to all functions with return types like

std::error_code, std::expected, boost::system::error_code, abseil::Status...

Resolves issue https://github.com/llvm/llvm-project/issues/62884

Code example:

#include <algorithm>
#include <iostream>
#include <system_error>
#include <vector>

std::error_code foo() {
  return std::error_code(errno, std::generic_category());
};

int main() {
  std::vector<int> v = {1, 2, 3, 4};
  std::remove_if(std::begin(v), std::end(v), [](int &x) {
    if (x == 1) {
      return true;
    }
    return false;
  });

  foo();
}

output:

/home/nvellanki/scratch/llvm_test_ground/test.cpp:12:3: error: the value returned by this function should be used [bugprone-unused-return-value,-warnings-as-errors]
  std::remove_if(std::begin(v), std::end(v), [](int &x) {
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/nvellanki/scratch/llvm_test_ground/test.cpp:12:3: note: cast the expression to void to silence this warning
/home/nvellanki/scratch/llvm_test_ground/test.cpp:19:3: error: the value returned by this function should be used [bugprone-unused-return-value,-warnings-as-errors]
  foo();
  ^~~~~
/home/nvellanki/scratch/llvm_test_ground/test.cpp:19:3: note: cast the expression to void to silence this warning
2 warnings treated as errors

[chaitanyav]

@royjacobson Please list other types that must be included here. The tests are coming...

[Eugene.Zelenko]

Please extend test case.

[chaitanyav]

Reuse diag code

[PiotrZSL]

• Missing tests
• Missing release notes
• Missing check documentation update

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

179

This may cause check method to be executed twice for same AST node.
Instead of adding new matcher you should just extend MatchedCallExpr to include all methods that isInstantiatedFrom(hasAnyName(FunVec)) or returns one of those specific types.

181

Put those return types into some configuration option instead of hardcoding it.

182

check if this work with typedefs/aliases...

193–194

this for is not needed, just use same "bind" name, and you wont need to change this method at all.

[chaitanyav]

Add tests, update docs and extend matchcallexpr

[chaitanyav]

please let me know if there is better way to do the matching hasAnyReturnType. We could also sort then do binarysearch instead of looping through the list of ret types.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

394

Please keep alphabetical order in section (by check name).

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

53

Please use single back-tick for option values.

[chaitanyav]

Use single backticks for options

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

394

Excessive newline.

[PiotrZSL]

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

159

Consider:

hasAnyReturnType -> returns(hasCanonicalType(hasDeclaration(namedDecl(matchers::matchesAnyListedName(RetTypeVec))))
Note that to use matchesAnyListedName, you should save RetTypeVec as class member, just to be safe.

[chaitanyav]

Make changes as per comments

[PiotrZSL]

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

140

good but put those all as fully qualified names (starting with ::) like in Checkedfunctions, and align documentation

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

53

split this, into ::std::error_code, ::std::expected, and so on...

[chaitanyav]

use fully qualified name in code and documentation

[PiotrZSL]

LGTM, you may consider reducing commit message (aka review description by removing example and error from it, and leaving just plain description of a change).

[chaitanyav]

In D151383#4377043, @PiotrZSL wrote:

LGTM, you may consider reducing commit message (aka review description by removing example and error from it, and leaving just plain description of a change).

got it, will make sure to remove them from the arc generated commit message.

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

159

I followed the code in ForRangeCopyCheck.cpp and made the changes accordingly.

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

 public:
   UnusedReturnValueCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   std::string CheckedFunctions;
+  std::string CheckedReturnTypes;
 };
 
 } // namespace clang::tidy::bugprone
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDRETURNVALUECHECK_H

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

 // 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);
 }
 
+// Checks if the function return type is in CheckedReturnedTypes
+AST_MATCHER_P(FunctionDecl, hasAnyReturnType, std::vector<StringRef>,
+              RetTypes) {
+  auto FuncRetType = Node.getReturnType().getAsString();
+  for (const auto RetType : RetTypes) {
+    if (RetType == FuncRetType) {
+      return true;
+    }
+  }
+  return false;
+}
 } // namespace
 
 UnusedReturnValueCheck::UnusedReturnValueCheck(llvm::StringRef Name,
                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       CheckedFunctions(Options.get("CheckedFunctions",
                                    "::std::async;"
                                    "::std::launder;"
: ClangTidyCheck(Name, Context),
                                    "::scandir;"
                                    "::semget;"
                                    "::setjmp;"
                                    "::shm_open;"
                                    "::shmget;"
                                    "::sigismember;"
                                    "::strcasecmp;"
                                    "::strsignal;"
-                                   "::ttyname")) {}
+                                   "::ttyname")),
+      CheckedReturnTypes(Options.get("CheckedReturnTypes",
+                                     "std::error_code;"
+                                     "std::expected;"
+                                     "boost::system::error_code;"
+                                     "abseil::Status")) {}
 
 void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckedFunctions", CheckedFunctions);
+  Options.store(Opts, "CheckedReturnTypes", CheckedReturnTypes);
 }
 
 void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
   auto FunVec = utils::options::parseStringList(CheckedFunctions);
+  auto RetTypeVec = utils::options::parseStringList(CheckedReturnTypes);
+
   auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts(
       callExpr(callee(functionDecl(
                    // Don't match void overloads of checked functions.
                    unless(returns(voidType())),
-                   isInstantiatedFrom(hasAnyName(FunVec)))))
+                   anyOf(isInstantiatedFrom(hasAnyName(FunVec)),
+                         hasAnyReturnType(RetTypeVec)))))
           .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.
                    unless(hasParent(stmtExpr())));
   auto UnusedInIfStmt =
       ifStmt(eachOf(hasThen(MatchedCallExpr), hasElse(MatchedCallExpr)));
   auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr));
   auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr));
   auto UnusedInForStmt =
       forStmt(eachOf(hasLoopInit(MatchedCallExpr),
                      hasIncrement(MatchedCallExpr), hasBody(MatchedCallExpr)));
   auto UnusedInRangeForStmt = cxxForRangeStmt(hasBody(MatchedCallExpr));
   auto UnusedInCaseStmt = switchCase(forEach(MatchedCallExpr));
 
   Finder->addMatcher(
       stmt(anyOf(UnusedInCompoundStmt, UnusedInIfStmt, UnusedInWhileStmt,
                  UnusedInDoStmt, UnusedInForStmt, UnusedInRangeForStmt,
                  UnusedInCaseStmt)),
       this);
 }
 
 void UnusedReturnValueCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Matched = Result.Nodes.getNodeAs<CallExpr>("match")) {
     diag(Matched->getBeginLoc(),
          "the value returned by this function should be used")
         << Matched->getSourceRange();
     diag(Matched->getBeginLoc(),
          "cast the expression to void to silence this warning",
          DiagnosticIDs::Note);
   }
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
 - Improved :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>`: warn on ``const &&``
   constructors.
 
+- Extend ``bugprone-unused-return-value`` check to check for all functions
+  with specified return types using the ``CheckedReturnTypes`` option.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

.. option:: CheckedFunctions
    - ``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()``.
 
+.. option:: CheckedReturnTypes
+
+   Semicolon-separated list of function return types to check.
+   By default the following function return types are checked:
+   ``std::error_code std::expected boost::system::error_code abseil::Status``
+
 `cert-err33-c <../cert/err33-c.html>`_ is an alias of this check that checks a
 fixed and large set of standard library functions.

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

 
 typedef basic_string<char> string;
 
 template <typename T, typename Allocator = std::allocator<T>>
 struct vector {
   bool empty() const noexcept;
 };
 
+class error_code {
+};
+
 // 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
 } // namespace std
 
 struct Foo {
   void f();
 };
 
 int increment(int i) {
   return i + 1;
 }
 
 void useFuture(const std::future &fut);
 
+std::error_code errorFunc() {
+    return std::error_code();
+}
+
 void warning() {
   std::async(increment, 42);
   // CHECK-NOTES: [[@LINE-1]]:3: warning: the value returned by this function should be used
   // CHECK-NOTES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning
 
   std::async(std::launch::async, increment, 42);
   // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used
   // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning
try {
     std::remove(nullptr, nullptr, 1);
     // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used
     // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning
   } catch (...) {
     std::remove(nullptr, nullptr, 1);
     // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used
     // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning
   }
+
+  errorFunc();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used
+  // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning
 }
 
 void noWarning() {
   auto AsyncRetval1 = std::async(increment, 42);
   auto AsyncRetval2 = std::async(std::launch::async, increment, 42);
 
   Foo FNoWarning;
   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();
 
+  (void) errorFunc();
+
   // 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);)