[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

159

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.

161

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

162

check if this work with typedefs/aliases...

176–177

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 ↗

(On Diff #525855)

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

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

53 ↗

(On Diff #525855)

Please use single back-tick for option values.

[chaitanyav]

Use single backticks for options

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

398 ↗

(On Diff #525926)

Excessive newline.

[PiotrZSL]

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

140

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

129

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 ↗

(On Diff #526159)

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

140

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

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

: ClangTidyCheck(Name, Context),
                                    "::setjmp;"
                                    "::shm_open;"
                                    "::shmget;"
                                    "::sigismember;"
                                    "::strcasecmp;"
                                    "::strsignal;"
                                    "::ttyname")) {}
 
 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(
                    // Don't match void overloads of checked functions.
                    unless(returns(voidType())),
                    isInstantiatedFrom(hasAnyName(FunVec)))))
           .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(
+      callExpr(callee(functionDecl(anyOf(
+                   hasReturnTypeLoc(loc(asString("std::error_code"))),
+                   hasReturnTypeLoc(loc(asString("std::expected"))),
+                   hasReturnTypeLoc(loc(asString("boost::system::error_code"))),
+                   hasReturnTypeLoc(loc(asString("abseil::Status")))))))
+          .bind("return-types"),
+      this);
+
+  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")) {
+  const char *callExprBindingNames[] = {"return-types", "match"};
+  for (const char *callExprBindingName : callExprBindingNames) {
+    if (const auto *Matched =
+            Result.Nodes.getNodeAs<CallExpr>(callExprBindingName)) {
       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