This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by chaitanyav on May 24 2023, 3:26 PM.

Details

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

Diff Detail

Event Timeline

chaitanyav created this revision.May 24 2023, 3:26 PM
Herald added a project: Restricted Project. · View Herald Transcript
chaitanyav requested review of this revision.May 24 2023, 3:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 3:26 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

chaitanyav edited the summary of this revision. (Show Details)May 24 2023, 3:30 PM
Eugene.Zelenko retitled this revision from Check for specific return types on all functions to [clang-tidy] Check for specific return types on all functions.May 24 2023, 3:33 PM

Please extend test case.

Reuse diag code

PiotrZSL requested changes to this revision.May 25 2023, 4:38 AM
  • 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.

This revision now requires changes to proceed.May 25 2023, 4:38 AM

Add tests, update docs and extend matchcallexpr

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 added inline comments.May 25 2023, 4:08 PM
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 marked an inline comment as done.

Use single backticks for options

Eugene.Zelenko added inline comments.May 25 2023, 9:15 PM
clang-tools-extra/docs/ReleaseNotes.rst
394

Excessive newline.

PiotrZSL added inline comments.May 26 2023, 7:32 AM
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 marked 4 inline comments as done.

Make changes as per comments

PiotrZSL accepted this revision.May 26 2023, 12:47 PM
PiotrZSL added inline comments.
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...

This revision is now accepted and ready to land.May 26 2023, 12:47 PM
chaitanyav marked an inline comment as done.

use fully qualified name in code and documentation

PiotrZSL accepted this revision.May 26 2023, 1:29 PM

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).

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.