Page MenuHomePhabricator

[clang-tidy] Add 'readability-useless-return-value' check
Needs ReviewPublic

Authored by LukasHanel on Feb 4 2021, 3:07 PM.

Details

Summary

When we try to satisfy the checker bugprone-unused-return-value we sometimes see that we call functions that are unworthy of their return values being checked.
Such functions should be refactored first.
Also, when we need to document all the possible return values of a function, it feels strange to have only 1 possible return value.

This check looks for functions that always return 0.
The fix-it proposes to make such functions void and removes the expression from the return statement.

This checker is similar in spirit to libreoffice's should-return-bool and should-be-a-constant:

Diff Detail

Event Timeline

LukasHanel created this revision.Feb 4 2021, 3:07 PM
LukasHanel requested review of this revision.Feb 4 2021, 3:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2021, 3:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
LukasHanel updated this revision to Diff 321580.Feb 4 2021, 3:08 PM
LukasHanel edited projects, added Restricted Project; removed Restricted Project.

Add the real changes

Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2021, 3:08 PM
LukasHanel edited the summary of this revision. (Show Details)Feb 4 2021, 3:18 PM
LukasHanel edited the summary of this revision. (Show Details)
LukasHanel edited the summary of this revision. (Show Details)Feb 4 2021, 3:23 PM

New check must be mentioned in Release Notes.

clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
2

Please merge two lines. See other files as example.

65

Please don't use auto unless type is explicitly stated in statement or iterator.

73

Please don't use auto unless type is explicitly stated in statement or iterator.

75

Please don't use auto unless type is explicitly stated in statement or iterator.

76

Please don't use auto unless type is explicitly stated in statement or iterator.

clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst
7

Please use single back-tick for value.

17

Please remove one empty line.

21

Please use single back-tick for value.

39

Return is redundant. See readability-redundant-control-flow.

44

Return is redundant. See readability-redundant-control-flow.

46

Please remove one empty line.

LukasHanel updated this revision to Diff 321715.Feb 5 2021, 5:04 AM

Address review comments, fix C++ unit tests

LukasHanel marked 9 inline comments as done.Feb 5 2021, 5:10 AM

Thanks for the review!

clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst
44

My new checker will not remove the return statement. I added a new comment to the readme to recommend usage of the other checker as well.
Should I extend the fix-it to remove also the return statement?

@Eugene.Zelenko Why did you remove me from the reviewers list?

njames93 requested changes to this revision.Feb 5 2021, 5:54 AM

This check, more specifically the fixes, seem quite dangerous. Changing function signatures is highly likely to cause compilation issues.
For a starters this check doesn't appear to examine call sites of the function to ensure they don't use the return value.
There's also the case where the function is used as a callback, this can't change the signature as the callback type would then mismatch.
From these 2 issues, this shouldn't trigger on functions with external linkage.

There's the case where the return value is based on some preprocessor constant. You are showing how it handles NULL and EXIT_SUCCESS, however what if the macro is called BUILT_WITH_FEATURE_X, that's likely a build setting and shouldn't be triggered upon. Obviously there's no hard and fast way to determine what's a dependent macro and what isn't, so either never trigger on returns where the value is a macro, or have a list of allowed macros, defaulted to containing the likes of NULL.

With all being said, I feel that while the diagnosing of these functions can have some value, an automated fix probably isn't a good idea.

This revision now requires changes to proceed.Feb 5 2021, 5:54 AM

@Eugene.Zelenko Why did you remove me from the reviewers list?

Only project's maintainers should be reviewers and approve patch. Sure, nobody prevents you from making comments.

@Eugene.Zelenko Why did you remove me from the reviewers list?

Only project's maintainers should be reviewers and approve patch. Sure, nobody prevents you from making comments.

I think that's true for accepting a patch, but not so true for just generic reviewers. For instance, it's common to add reviewers based on who recently modified the code, which often is not a maintainer.

@Eugene.Zelenko Why did you remove me from the reviewers list?

Only project's maintainers should be reviewers and approve patch. Sure, nobody prevents you from making comments.

And what determines list of reviewers? Why do you choose some people to be on it and not others?

Anyway, I think you get the point that you shouldn't do that.

LukasHanel added a comment.EditedFeb 5 2021, 11:43 AM

Hi, thanks for discussing my proposal!
Although I think it can stand as is, I was looking for feedback:

  • Is the name good?
  • Is the readability group good? Or better in misc?
  • too slow, too fast?
  • More precision required?
  • Usefulness of the fix-it's
  • Should I add options?

Anyhow, last year I was refactoring our companies code base and often manually searched for functions that would be identified by such checker. And as you say, I did not implement the corner cases yet.
Clang-tidy is a really easy environment to write such checkers, Kudos to all the contributors and maintainers.

On your comments:

This check, more specifically the fixes, seem quite dangerous. Changing function signatures is highly likely to cause compilation issues.

This seems to be a general issue with clang-tidy's fixes.
It seems some fixes are more of a gimmick and shouldn't be used without supervision.
I did not find any other checker that changed the function signature, is there any guidance against this?

For a starters this check doesn't appear to examine call sites of the function to ensure they don't use the return value.

I have considered this, but it would double the complexity of the checker, so I did not work on it yet.
Yes, it is a good safety net.
However, it could also hide two issues:

  1. Inconsistent use (or not use) of the return value - it is actually a sign of the return value being useless (Note: this is a common checker for commercial static analyzers).
  2. Propagation of the useless return value, let's say b checks useless return value of a, and returns it, but c does not check return value of b.
int a() { return 0; } // "useless"
int b() { return a(); } // "checked"
int c() {
    b();                      // not checked
   // or even
   int ret = b();        // "checked"
   if (ret) {
       printf("something went wrong"); // or maybe the return value was useless in the first place
   }
   //...
}

Regarding the propagation, anyhow, such refactoring needs to be done in steps: you fix one function, then the linter will highlight the next function.

There's also the case where the function is used as a callback, this can't change the signature as the callback type would then mismatch.

Yes, we had this case as well.
I wonder how I could detect this.

  1. I guess I need the call sites.
  2. Or a configuration for this checker of functions to ignore.
  3. Or somehow annotate functions to ignore this checker.

From these 2 issues, this shouldn't trigger on functions with external linkage.

From my experience, this is actually more a problem for external functions.
Static functions tend to less have such issues because the deveoloper can more easily verify the callers.

There's the case where the return value is based on some preprocessor constant. You are showing how it handles NULL and EXIT_SUCCESS, however what if the macro is called BUILT_WITH_FEATURE_X, that's likely a build setting and shouldn't be triggered upon. Obviously there's no hard and fast way to determine what's a dependent macro and what isn't, so either never trigger on returns where the value is a macro, or have a list of allowed macros, defaulted to containing the likes of NULL.

Curious case of the command-line defined return-value macro. :)
When playing with this part, I found that clang-tidy sees the code after preprocessing.
So I never saw that I was handling NULL or EXIT_SUCCESS, I only saw 0.
Clang-tidy must know about build-time macros from the compile_commands.json data base, otherwise it would scream.
Hmm, I start to see what you mean.

With all being said, I feel that while the diagnosing of these functions can have some value, an automated fix probably isn't a good idea.

Thanks.

Will the CI run my checker on the llvm code base?
I found several issues locally, I will figure out how to best share those findings.

Hi, thanks for discussing my proposal!
Although I think it can stand as is, I was looking for feedback:

  • Is the name good?
  • Is the readability group good? Or better in misc?
  • too slow, too fast?
  • More precision required?
  • Usefulness of the fix-it's

I agree with @njames93 that this check is dangerous. Even if you extended it to port callExprs, that would only work in translation units which can see the definition.

I have made checks to do that kind of thing (port from raw pointers to unique_ptr), but it involves running a check to generate a list of functions that will be ported in one step and doing the actual change (including callExprs across all TUs) in a second step.

A tool like this would have to something similar to be useful, but I don't think such a check should be upstream at this point. We don't have good infrastructure to record the signatures that will be changed. What I did was create/touch files to the filesystem named as function signatures in the first step, and in the second step read the list of files to know what signatures should be ported. Using the filesystem meant uniqueness and that a concurrently-running instances didn't encounter syncronization issues. Perhaps something like that could be made generic and upstreamed to support this kind of case.

In addition to that practical problem, there are some functions which deliberately always return the same value and you don't have a way to distinguish them other than by adding an option to ignore them. It might make the checker a bit of a burden. The kinds of functions I'm referring to include functions which give a name to an otherwise-magic number (int sentinalValue() { return 0; }), virtual methods (I see you already handle those), Visitors, dummy implementations of APIs which may not be implemented on a platform, CRTP interfaces, there may be others.

Am I missing something?

  • Should I add options?

Anyhow, last year I was refactoring our companies code base and often manually searched for functions that would be identified by such checker. And as you say, I did not implement the corner cases yet.

Does this mean that you didn't try to run this tool on a real codebase? (ie your company code is already changed and doesn't need it anymore). You may run into the cross-TU issue if you run it on a codebase.

Clang-tidy is a really easy environment to write such checkers, Kudos to all the contributors and maintainers.

Glad it's useful to you.

On your comments:

This check, more specifically the fixes, seem quite dangerous. Changing function signatures is highly likely to cause compilation issues.

This seems to be a general issue with clang-tidy's fixes.
It seems some fixes are more of a gimmick and shouldn't be used without supervision.

I think you might be right. Do you have a list of such checks? Maybe they should be removed.

I did not find any other checker that changed the function signature, is there any guidance against this?

I don't think there are any because they don't work across TUs. If we don't have guidance on this, we should probably create some. My position is that the guidance should say that we shouldn't have such checks until we have the required infrastructure to implement them correctly.

For a starters this check doesn't appear to examine call sites of the function to ensure they don't use the return value.

I have considered this, but it would double the complexity of the checker, so I did not work on it yet.
Yes, it is a good safety net.
However, it could also hide two issues:

  1. Inconsistent use (or not use) of the return value - it is actually a sign of the return value being useless (Note: this is a common checker for commercial static analyzers).

Indeed, this doesn't seem to be an issue. If the function returns a constant, then the behavior at the call site is known and can be simplified to operate the same way without checking the return value.

  • Propagation of the useless return value, let's say b checks useless return value of a, and returns it, but c does not check return value of b.
int a() { return 0; } // "useless"
int b() { return a(); } // "checked"
int c() {
    b();                      // not checked
   // or even
   int ret = b();        // "checked"
   if (ret) {
       printf("something went wrong"); // or maybe the return value was useless in the first place
   }
   //...
}

Regarding the propagation, anyhow, such refactoring needs to be done in steps: you fix one function, then the linter will highlight the next function.

Yes.

steveire requested changes to this revision.Feb 8 2021, 4:24 PM

Hi, thanks for discussing my proposal!

  • Usefulness of the fix-it's

I agree with @njames93 that this check is dangerous. Even if you extended it to port callExprs, that would only work in translation units which can see the definition.

Are you saying I should just remove the fix-its altogether?
Or, put them under some option that is off by default?

In addition to that practical problem, there are some functions which deliberately always return the same value and you don't have a way to distinguish them other than by adding an option to ignore them. It might make the checker a bit of a burden. The kinds of functions I'm referring to include functions which give a name to an otherwise-magic number (int sentinalValue() { return 0; }), virtual methods (I see you already handle those), Visitors, dummy implementations of APIs which may not be implemented on a platform, CRTP interfaces, there may be others.

Anyhow, last year I was refactoring our companies code base and often manually searched for functions that would be identified by such checker. And as you say, I did not implement the corner cases yet.

Does this mean that you didn't try to run this tool on a real codebase? (ie your company code is already changed and doesn't need it anymore). You may run into the cross-TU issue if you run it on a codebase.

Running on my own code base is the next step. clang-tidy is not well integrated there yet.
Running on llvm-project, it got me to implement several filters:

In addition, I saw the following:
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/Support/regcomp.c#L1180
Clearly not used most of the time, but the macro REQUIRE uses seterr() in an expression. However, a void function call creates a statement, not an expression.

doRegionSplit:
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/RegAllocGreedy.cpp#L1965
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/RegAllocGreedy.cpp#L1862
^ Replace here with call(); return 0;.
Like here:
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/RegAllocGreedy.cpp#L2826

https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6887
Used 4 times in a private class, with an assert that warns about the single use. I guess it is a magic number with an assert, so somewhat useful, but still 6 years that nobody added more transitions.

It seems some fixes are more of a gimmick and shouldn't be used without supervision.

I think you might be right. Do you have a list of such checks? Maybe they should be removed.

https://clang.llvm.org/extra/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.html

changing the parameter names, but its declaration in header file is not updated.

For us it fixed things in the wrong order. The header file were polished doxygen resembling the user facing documentation or API standard.
In the function definition, we used parameter names that helped in the implementation. Those should be changed, not the ones in the header file. This should be an option IMHO.

https://clang.llvm.org/docs/DiagnosticsReference.html#wasm-operand-widths
Can be missing context, it changes % to %w, which is not always what you want.

https://clang.llvm.org/docs/DiagnosticsReference.html#wformat-type-confusion (not sure which one actually)
%08lx, --> %08llx, but then does it compile in 32-bit 64bit? Maybe should be replaced by PRIx64.

Well, mostly, I was set back by the broken formatting after fix-its, and also the sheer number of fix-its and related options.

Hi, thanks for discussing my proposal!

  • Usefulness of the fix-it's

I agree with @njames93 that this check is dangerous. Even if you extended it to port callExprs, that would only work in translation units which can see the definition.

Are you saying I should just remove the fix-its altogether?
Or, put them under some option that is off by default?

I'm not sure this check meets the general applicability and usefulness threshold to be part of clang-tidy. The warning alone isn't actionable. Someone wanting to take action on the warning would have to write their own clang tidy check to make the change across their codebase. Add that to the false-positive issues I mentioned before.

Does anyone have any other thoughts?

Hi, thanks for discussing my proposal!

  • Usefulness of the fix-it's

I agree with @njames93 that this check is dangerous. Even if you extended it to port callExprs, that would only work in translation units which can see the definition.

Are you saying I should just remove the fix-its altogether?
Or, put them under some option that is off by default?

I'm not sure this check meets the general applicability and usefulness threshold to be part of clang-tidy. The warning alone isn't actionable. Someone wanting to take action on the warning would have to write their own clang tidy check to make the change across their codebase. Add that to the false-positive issues I mentioned before.

Does anyone have any other thoughts?

I agree. I'm not certain such a check could ever be generally applicable. For example, some functions return zero always because it's part of the API design. e.g.,

struct Base {
  virtual int foo() { return 0; /* Degenerate case */ }
};

struct Derived : Base {
  int foo() override; // More useful implementation
};

// Or
namespace std {
template <>
class numeric_limits<MyAwesomeType> {
public:
  ...
  static constexpr MyAwesomeType lowest() noexcept { return 0; /* implicit conversion to the return type */ }
  ...
};

Also, there's nothing particularly interesting about 0 as opposed to any other constant return value.

A somewhat similar check that would be interesting is a function that returns the same value on all control paths, as that may signify a logical error on the part of the programmer. e.g.,

int foo() {
  if (whatever)
    return 0;
 ... code without return statements ...
  return 0;
}

// Or
int bar() {
  if (whatever)
    return foo();
  else
    ...
  ...
  return foo();
}

but I'd want this check to ignore functions that only have a single return statement in them in order to reduce noise.

steveire added a comment.EditedFeb 16 2021, 3:37 PM

A somewhat similar check that would be interesting is a function that returns the same value on all control paths

I think we shouldn't try to design a new, different check in the comments of this MR.

I think it would be better to finalize what to do with this one.

Request further work to also change expressions in all affected TUs? Or close?

Handle the case of a global variable being the "return value"

A somewhat similar check that would be interesting is a function that returns the same value on all control paths

I think we shouldn't try to design a new, different check in the comments of this MR.

I think it would be better to finalize what to do with this one.

Request further work to also change expressions in all affected TUs? Or close?

Hi,
I finally managed to run this checker on my own code base and I run into all the above problems:

  1. There are some sloppy areas in the code that this checker nicely highlights.
  2. Many false-positives for example with callbacks or involving preprocessor
  3. Some questionable suggestions, like where it is breaking code-symmetry with a set of handlers that have the same signatures but some always return 0.

In conclusion for this checker:

  • it's a good way to spot sloppy areas in the code.
  • You couldn't run it in -Werror mode to enforce clean code.
  • Yes, I could filter more FP/noise, e.g. functions that have a single return statement in the body; plus maybe assert.
  • However, if you have a policy to use the return value of all functions, this checker can be a good way to clean the code first.

I am ok to close this one, I will park it somewhere.

In the meantime, I try to spin up some fixes for the false positives that I see with other checkers, as mentioned above.

As a follow-up to this checker, clang -Wdocumentation does not understand the @retval command.
I was thinking of adding a new clang-tidy checker for this to verify/complete the list of documented return values.
In this case where the documentation does not say @retval 0 always, than I will come back to the checker here, suggest make the function void or add the "always" to the text :).
The work with the useless-return-value was a study towards this new @retval checker.

A somewhat similar check that would be interesting is a function that returns the same value on all control paths

I think we shouldn't try to design a new, different check in the comments of this MR.

I wasn't. I was giving an example of what is similar and I would find acceptable so that the code author has more data from which to make decisions about their patch.

I think it would be better to finalize what to do with this one.

Request further work to also change expressions in all affected TUs? Or close?

I don't think requesting further work would solve any of my concerns short of making a rather different check, so my preference is to close.

As a follow-up to this checker, clang -Wdocumentation does not understand the @retval command.
I was thinking of adding a new clang-tidy checker for this to verify/complete the list of documented return values.
In this case where the documentation does not say @retval 0 always, than I will come back to the checker here, suggest make the function void or add the "always" to the text :).
The work with the useless-return-value was a study towards this new @retval checker.

That's an interesting idea -- we don't have any clang-tidy checks related to documentation comments currently, but the doxygen-style comments are the only form of comment to be retained in the AST. So it seems plausible to write clang-tidy checkers for documentation comments. However, you should know that there are limitations to this given that clang-tidy works most effectively with ASTs and not the user's original source -- it's trivially easy to lose some or all of the AST nodes for the comments if the formatting gets mucked up: https://godbolt.org/z/nWcMsM So it is possible that it's easier to do this work in the frontend instead.