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

[LukasHanel]

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:

• https://git.libreoffice.org/core/+/HEAD/compilerplugins/clang/returnconstant.cxx
• https://git.libreoffice.org/core/+/HEAD/compilerplugins/clang/shouldreturnbool.cxx

[LukasHanel]

Add the real changes

[Eugene.Zelenko]

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]

Address review comments, fix C++ unit tests

[LukasHanel]

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?

[steveire]

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

[njames93]

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.

[Eugene.Zelenko]

In D96082#2544778, @steveire wrote:

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

[aaron.ballman]

In D96082#2545106, @Eugene.Zelenko wrote:

In D96082#2544778, @steveire wrote:

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

[steveire]

In D96082#2545106, @Eugene.Zelenko wrote:

In D96082#2544778, @steveire wrote:

@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]

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:

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

[steveire]

In 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, @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.

[LukasHanel]

In D96082#2549943, @steveire wrote:

In D96082#2545807, @LukasHanel wrote:

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:

• https://github.com/llvm/llvm-project/blob/main/llvm/lib/Demangle/MicrosoftDemangle.cpp#L923 : assignment to std:tie()
• e.g. https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/FastISel.h#L473 virtual methods

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.

[steveire]

In D96082#2550468, @LukasHanel wrote:

In D96082#2549943, @steveire wrote:

In D96082#2545807, @LukasHanel wrote:

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?

[aaron.ballman]

In D96082#2561806, @steveire wrote:

In D96082#2550468, @LukasHanel wrote:

In D96082#2549943, @steveire wrote:

In D96082#2545807, @LukasHanel wrote:

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]

In D96082#2565339, @aaron.ballman wrote:

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?

[LukasHanel]

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

[LukasHanel]

In D96082#2566984, @steveire wrote:

In D96082#2565339, @aaron.ballman wrote:

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.

[aaron.ballman]

In D96082#2566984, @steveire wrote:

In D96082#2565339, @aaron.ballman wrote:

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.

In D96082#2567143, @LukasHanel wrote:

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.

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   SimplifyBooleanExprCheck.cpp
   SimplifySubscriptExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
   UppercaseLiteralSuffixCheck.cpp
   UseAnyOfAllOfCheck.cpp
+  UselessReturnValueCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyUtils
 
   DEPENDS
   omp_gen
   )

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 #include "SimplifyBooleanExprCheck.h"
 #include "SimplifySubscriptExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "StringCompareCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
+#include "UselessReturnValueCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SimplifySubscriptExprCheck>(
         "readability-simplify-subscript-expr");
     CheckFactories.registerCheck<StaticAccessedThroughInstanceCheck>(
         "readability-static-accessed-through-instance");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<StringCompareCheck>(
         "readability-string-compare");
+    CheckFactories.registerCheck<UselessReturnValueCheck>(
+        "readability-useless-return-value");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<NonConstParameterCheck>(
         "readability-non-const-parameter");
     CheckFactories.registerCheck<RedundantControlFlowCheck>(
         "readability-redundant-control-flow");
     CheckFactories.registerCheck<RedundantDeclarationCheck>(
         "readability-redundant-declaration");

clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.h

+//===--- UselessReturnValueCheck.h - clang-tidy -----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USELESSRETURNVALUECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USELESSRETURNVALUECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Looks for function that only and always return 0 and
+/// proposes to make them void.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-useless-return-value.html
+class UselessReturnValueCheck : public ClangTidyCheck {
+public:
+  UselessReturnValueCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void replaceWithVoid(const FunctionDecl *MatchedDecl);
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USELESSRETURNVALUECHECK_H

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

+//===--- UselessReturnValueCheck.cpp - clang-tidy -------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UselessReturnValueCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void UselessReturnValueCheck::registerMatchers(MatchFinder *Finder) {
+  auto Int0 = integerLiteral(equals(0));
+  auto IgnInt0 = ignoringParenCasts(anyOf(Int0, parenExpr(has(Int0))));
+  auto Int0Var =
+      anyOf(IgnInt0,
+            ignoringImplicit(declRefExpr(
+                to(varDecl(hasInitializer(IgnInt0), unless(hasGlobalStorage()))
+                       .bind("retvar")))));
+  Finder->addMatcher(
+      functionDecl(
+          isDefinition(), unless(returns(voidType())), unless(hasName("main")),
+          unless(cxxMethodDecl(isVirtual())),
+          forEachDescendant(
+              returnStmt(hasReturnValue(Int0Var)).bind("return-to-void")),
+          unless(hasDescendant(returnStmt(unless(hasReturnValue(Int0Var))))),
+          unless(hasDescendant(binaryOperator(
+              isAssignmentOperator(),
+              hasLHS(declRefExpr(to(varDecl(equalsBoundNode("retvar")))))))),
+          unless(hasDescendant(cxxOperatorCallExpr(
+              isAssignmentOperator(),
+              hasLHS(hasDescendant(
+                  declRefExpr(to(varDecl(equalsBoundNode("retvar"))))))))),
+          unless(hasDescendant(
+              unaryOperator(hasAnyOperatorName("++", "--", "&"),
+                            hasUnaryOperand(ignoringImplicit(declRefExpr(
+                                to(varDecl(equalsBoundNode("retvar"))))))))))
+          .bind("function-to-void"),
+      this);
+}
+
+void UselessReturnValueCheck::replaceWithVoid(const FunctionDecl *MatchedDecl) {
+  diag(MatchedDecl->getLocation(), "function %0 returns always the same value")
+      << MatchedDecl;
+
+  diag(MatchedDecl->getLocation(), "make function void", DiagnosticIDs::Note)
+      << FixItHint::CreateReplacement(MatchedDecl->getReturnTypeSourceRange(),
+                                      "void");
+}
+
+void UselessReturnValueCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl =
+      Result.Nodes.getNodeAs<FunctionDecl>("function-to-void");
+  if (!MatchedDecl->getIdentifier())
+    return;
+  replaceWithVoid(MatchedDecl);
+
+  const FunctionDecl *ProtoDecl = MatchedDecl->getCanonicalDecl();
+  if (ProtoDecl)
+    replaceWithVoid(ProtoDecl);
+
+  const auto *MatchedReturn =
+      Result.Nodes.getNodeAs<ReturnStmt>("return-to-void");
+  diag(MatchedReturn->getBeginLoc(), "returns 0 here");
+  SourceLocation RemovalStartLocation =
+      MatchedReturn->getBeginLoc().getLocWithOffset(6);
+  SourceRange RemovalRange =
+      SourceRange(RemovalStartLocation, MatchedReturn->getEndLoc());
+  diag(RemovalStartLocation, "remove return value", DiagnosticIDs::Note)
+      << FixItHint::CreateRemoval(RemovalRange);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 The improvements are...
 
+ - New :doc:`readability-useless-return-value <clang-tidy/checks/readability-useless-return-value>` check.
+
+ Finds functions that always return `0`, such functions could be ``void``.
+
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `readability-simplify-boolean-expr <readability-simplify-boolean-expr.html>`_, "Yes"
    `readability-simplify-subscript-expr <readability-simplify-subscript-expr.html>`_, "Yes"
    `readability-static-accessed-through-instance <readability-static-accessed-through-instance.html>`_, "Yes"
    `readability-static-definition-in-anonymous-namespace <readability-static-definition-in-anonymous-namespace.html>`_, "Yes"
    `readability-string-compare <readability-string-compare.html>`_, "Yes"
    `readability-uniqueptr-delete-release <readability-uniqueptr-delete-release.html>`_, "Yes"
    `readability-uppercase-literal-suffix <readability-uppercase-literal-suffix.html>`_, "Yes"
    `readability-use-anyofallof <readability-use-anyofallof.html>`_,
+   `readability-useless-return-value <readability-useless-return-value.html>`_, "Yes"
    `zircon-temporary-objects <zircon-temporary-objects.html>`_,
 
 
 .. csv-table:: Aliases..
    :header: "Name", "Redirect", "Offers fixes"
 
    `cert-con36-c <cert-con36-c.html>`_, `bugprone-spuriously-wake-up-functions <bugprone-spuriously-wake-up-functions.html>`_,
    `cert-con54-cpp <cert-con54-cpp.html>`_, `bugprone-spuriously-wake-up-functions <bugprone-spuriously-wake-up-functions.html>`_,

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

+.. title:: clang-tidy - readability-useless-return-value
+
+readability-useless-return-value
+================================
+
+This check looks for functions that always return `0`.
+Such functions could be ``void``.
+
+When we try to satisfy the checker
+`bugprone-unused-return-value <bugprone-unused-return-value.html>`_,
+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.
+
+Examples:
+
+The following function `f` and `f2` return always `0`:
+
+.. code-block:: c
+
+  int f() {
+    return 0;
+  }
+
+  int f2() {
+    int ret = 0;
+    return ret;
+  }
+
+becomes
+
+.. code-block:: c
+
+  void f() {
+    return;
+  }
+
+  void f2() {
+    int ret = 0;
+    return;
+  }
+
+Note: in above samples, the final `return` statement should be removed as well,
+using `readability-redundant-control-flow <readability-redundant-control-flow.html>`_.
+
+This is useful in following cleanup:
+.. code-block:: c
+
+  int g(int i) {
+    f();
+    // ^ Warning: unused return value of f()
+    if (i > 0)
+      return 1;
+    return 0;
+  }

clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.h

+int f11(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f11' returns always the same value [readability-useless-return-value]
+// CHECK-FIXES: {{^}}void f11(void);{{$}}

clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.c

+// RUN: %check_clang_tidy %s readability-useless-return-value %t
+
+int f() {
+  return 0;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+unsigned int f2() {
+  return 0U;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: function 'f2' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f2() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+typedef unsigned int mytype_t;
+mytype_t f3() {
+  return 0U;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:10: warning: function 'f3' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f3() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+const int f4() {
+  return 0;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: function 'f4' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}const void f4() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+static int f5() {
+  return 0;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: function 'f5' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}static void f5() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+#define EXIT_SUCCESS 0
+
+int f6() {
+  return EXIT_SUCCESS; //EXIT_SUCCESS
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f6' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f6() {{{$}}
+// CHECK-FIXES: {{^}}  return; //EXIT_SUCCESS{{$}}
+
+#define NULL 0
+
+int f7() {
+  return NULL; //NULL
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f7' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f7() {{{$}}
+// CHECK-FIXES: {{^}}  return; //NULL{{$}}
+
+int f8(int i) {
+  if (i < 0) {
+    f7();
+    return 0; //#1
+  } else {
+    return 0; //#2
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-8]]:5: warning: function 'f8' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-6]]:5: warning: returns 0 here [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-5]]:5: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f8(int i) {{{$}}
+// CHECK-FIXES: {{^}}    return; //#1{{$}}
+// CHECK-FIXES: {{^}}    return; //#2{{$}}
+
+int f9() {
+  return (0); //(0)
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f9' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f9() {{{$}}
+// CHECK-FIXES: {{^}}  return; //(0){{$}}
+
+int f10(void);
+int f10() {
+  return 0;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: function 'f10' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: function 'f10' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f10(void);{{$}}
+// CHECK-FIXES: {{^}}void f10() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+// fix-it works in the header file, but not handled by check_clang_tidy.py
+// #include "readability-useless-return-value.h"
+int f11() {
+  return 0;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f11' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f11() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+static __attribute__((section("abc"))) int f12() {
+  return 0;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:44: warning: function 'f12' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}static __attribute__((section("abc"))) void f12() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+int f13() {
+  int ret = 0;
+  return ret;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: function 'f13' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f13() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+int f14(int i) {
+  int ret = 0;
+  if (i > 0)
+    return 0; //(f14-1)
+  if (i > 1)
+    return ret; //(f14-2)
+  int ret1 = (0);
+  if (i > 2)
+    return ret1; //(f14-3)
+  return ret;    //(f14-4)
+}
+// CHECK-MESSAGES: :[[@LINE-11]]:5: warning: function 'f14' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-9]]:5: warning: returns 0 here [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-8]]:5: warning: returns 0 here [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-6]]:5: warning: returns 0 here [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-6]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f14(int i) {{{$}}
+// CHECK-FIXES: {{^}}    return; //(f14-1){{$}}
+// CHECK-FIXES: {{^}}    return; //(f14-2){{$}}
+// CHECK-FIXES: {{^}}    return; //(f14-3){{$}}
+// CHECK-FIXES: {{^}}  return;    //(f14-4){{$}}
+
+int f15() {
+  return (int)0; //(f15)
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f15' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f15() {{{$}}
+// CHECK-FIXES: {{^}}  return; //(f15){{$}}
+
+void g();
+
+void g1() {
+}
+
+int g2(int i) {
+  if (i < 0)
+    return 0;
+  else
+    return 1;
+}
+
+int g3() {
+  return (0 + 1);
+}
+
+int g4() {
+  return (0) + 1;
+}
+
+int g5() {
+  return g4();
+}
+
+int main() {
+  return 0;
+}
+
+int g6() {
+  int ret = 1;
+  return ret;
+}
+
+int g7() {
+  int ret = 0;
+  ret = 1;
+  return ret;
+}
+
+int g8() {
+  int ret = 0;
+  ret = g7();
+  return ret;
+}
+
+int g9() {
+  int ret = 0;
+  ret = 0; /* Should we add match for this case? */
+  return ret;
+}
+
+int g10(int i) {
+  int ret = 0;
+  if (i > 0)
+    return 1;
+  if (i > 1)
+    return ret;
+  int ret1 = 0;
+  if (i > 2)
+    return ret1;
+  return ret;
+}
+
+int g11(int i) {
+  return i;
+}
+
+int g12() {
+  int ret = 0;
+  ret++;
+  return ret;
+}
+
+int g13() {
+  int ret = 0;
+  g11(&ret);
+  return ret;
+}
+
+int ret = 0;
+int g14() {
+  return ret;
+}

clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.cpp

+// RUN: %check_clang_tidy %s readability-useless-return-value %t
+
+int f() {
+  return 0;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f' returns always the same value [readability-useless-return-value]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value]
+
+// CHECK-FIXES: {{^}}void f() {{{$}}
+// CHECK-FIXES: {{^}}  return;{{$}}
+
+struct Foo {
+  Foo(int i);
+  Foo &operator=(int &&other);
+};
+Foo tie(int i) {
+  return Foo(i);
+}
+
+int demangleUnsigned() {
+  int Number = 0;
+  tie(Number) = 1;
+  return Number;
+}
+
+class Foo1 {
+  virtual unsigned g(void) const;
+};
+
+unsigned Foo1::g(void) const {
+  return 0;
+}