[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D96082#2545807 <https://reviews.llvm.org/D96082#2545807>, @LukasHanel wrote:

> 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:
>
> In D96082#2544836 <https://reviews.llvm.org/D96082#2544836>, @njames93 wrote:
>
>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96082/new/

https://reviews.llvm.org/D96082