Add clang-tidy readability-redundant-control-flow check

[LegalizeAdulthood]

Summary

This check looks for return statements at the end of a function returning void:

void f()
{
  return;
}

becomes

void f()
{
}

It looks for redundant continue statements at the end of loop constructs:

void f() {
   for (int i = 0; i < 10; ++i) {
    g();
    continue;
  }
}

becomes

void f() {
  for (int i = 0; i < 10; ++i) {
    g();
  }
}

[LegalizeAdulthood]

clang-tidy/readability/RedundantReturnCheck.cpp

46–49

This was a great trick I stole from the unused namespace alias check. I could really use some method for trimming off preceding whitespace for a statement as well. Right now, the return statement gets removed, but when it appears on a line by itself, it's indentation whitespace is left in place and I'd like to be able to remove that as well. I tried a couple things, but for this first pass simply decided to let the test cases allow leading whitespace before the closing brace of the function. One can always use artistic style or clang-format to reformat the results anyway, so it's not that big a deal I suppose.

[Eugene.Zelenko]

Thank you for this check! PR21984 waited to be implemented for so looong time :-)

By the word, other return related readability check idea: PR22416. May be both could be combined in single check?

[LegalizeAdulthood]

Extend this check to handle redundant continue statements in loop bodies.

With this extension, I'm wondering if the check should be renamed redundant-control-flow instead of redundant-return.

Comments?

[LegalizeAdulthood]

I didn't know about the bug reports. I created this check because I have encountered such redundant control flow in real code bases.

This would close 21984, but not 22416. 22416 feels more like a check to eliminate redundant parenthesis, whereas this check is concerned with the entire control flow statement being redundant.

[Eugene.Zelenko]

In D16259#328834, @LegalizeAdulthood wrote:

I didn't know about the bug reports. I created this check because I have encountered such redundant control flow in real code bases.

Bug report was reflection on my code base, but I also spotted similar patterns in LLVM.

There are a lot of Clang-tidy enhancement ideas in Bugzilla, so may be you could find some interesting enough to be implemented?

[LegalizeAdulthood]

I'm starting with stuff that I see in code bases where I work; chances are that other people see them too. In the future, I'll look in the bug database for stuff that is similar or identical to what I'm doing.

[aaron.ballman]

I would like to see some additional tests to ensure that this does not turn dead code into live code with the fix. e.g.,

void g();
void f() {
  return;
  g(); // This would become live code if return were removed.
}

A similar test for continue would be appreciated as well. Tests for range-based for loops also.

clang-tidy/readability/RedundantReturnCheck.cpp

24

Would be better written as: returns(voidType()) instead of using asString().

25

Would be best to restrict this to a return statement that has no expression if we don't want to diagnose this:

void g();

void f() {
  return g();
}

Either way, it would be good to have a test that ensures this isn't mangled.

29

I think you also want to match cxxForRangeStmt() in the same way.

35

Elide braces for these if-else statements.

76

No need for ast_matchers.

78

Please consolidate this duplicated code.

clang-tidy/readability/RedundantReturnCheck.h

27

Since this also handling continue, I think this would be SpuriousFlowControlCheck instead?

[alexfh]

clang-tidy/readability/RedundantReturnCheck.h

27

Maybe RedundantControlFlowStatementsCheck?

docs/clang-tidy/checks/readability-redundant-return.rst

8

Please add an example for return and one more for continue.

[aaron.ballman]

clang-tidy/readability/RedundantReturnCheck.h

27

I like that name better. :-)

[alexfh]

clang-tidy/readability/RedundantReturnCheck.cpp

37

Maybe just bind the compound statement and get rid of these ifs?

test/clang-tidy/readability-redundant-return.cpp

2

Please add tests with macros and templates that ensure:

• no replacements in macro bodies
• correct replacements in macro arguments (that are not inside another macro's body)
• replacements in template bodies with multiple instantiations are applied only once (adding unless(isInTemplateInstantiation()) filter to the matcher is a usual way to handle this).

[LegalizeAdulthood]

clang-tidy/readability/RedundantReturnCheck.cpp

25

How about transforming this odd looking duck into

void g();

void f() {
  g();
}

?

[aaron.ballman]

clang-tidy/readability/RedundantReturnCheck.cpp

25

I think in the context of this check, that would be fine.

[kimgr]

Came up with another test case.

test/clang-tidy/readability-redundant-return.cpp

22–25

What happens to guard clauses invoking void functions?

void h() {
}

void g(int i) {
  if(i < 0) {
    return h();
  }
}

[LegalizeAdulthood]

test/clang-tidy/readability-redundant-return.cpp

22–25

Nothing because the last statement of the compoundStmt that is the function body is an if statement and not a return statement.

That is exactly why lines 21-24 are in the test suite :).

[kimgr]

test/clang-tidy/readability-redundant-return.cpp

22–25

Ah, I hadn't understood the mechanics of the check. I read the docs, and now I do! Don't mind me :-)

[LegalizeAdulthood]

test/clang-tidy/readability-redundant-return.cpp

22–25

I had thought about doing a deeper analysis of the control flow to look for such cases, but I will leave that for later.

For instance, the following code may plausibly appear in a code base:

void f() {
  do_stuff();
  {
    lock l(mutex);
    do_locked_stuff();
    return;
  }
}

I haven't tried this on this patch, but I suspect it would do nothing; I will add some examples of these more complicated cases to the test suite to show that the implementation doesn't yet handle more advanced flow analysis.

In this case, the return is similarly redundant, as well as a return as the last statement of an if as you mentioned. However, I wanted to start with something simple and get feedback on that before attempting to do more advanced cases.

[aaron.ballman]

test/clang-tidy/readability-redundant-return.cpp

22–25

I think that starting simple and expanding later is the right approach.

[LegalizeAdulthood]

clang-tidy/readability/RedundantReturnCheck.cpp

25

For now I have opted to simply ignore such odd ducks.

test/clang-tidy/readability-redundant-return.cpp

2

When I added template test cases, I wasn't sure how to write a test case for what you requested. I wrote test code that multiply instantiated a template and only see my check triggering on the template body as written in the code.

[LegalizeAdulthood]

Update from review comments

[LegalizeAdulthood]

Improve matcher to simplify callback

[aaron.ballman]

clang-tidy/readability/RedundantControlFlowCheck.cpp

24 ↗

(On Diff #45842)

void function -> function with a void return type.

45 ↗

(On Diff #45842)

Instead of adding four different matchers, it would be better to add one matcher with anyOf(). e.g.,

Finder->addMatcher(stmt(anyOf(forStmt(), whileStmt(), doStmt(), cxxForRangeStmt()), has(compoundStmt(hasAnySubstatement(continueStmt())))), this);

59 ↗

(On Diff #45842)

Elide braces.

68 ↗

(On Diff #45842)

Elide braces

[Eugene.Zelenko]

Just encountered next situation in my work code base:

void Function() {
...

  if (Condition) {
    ...
  }
  else
    return;
}

[LegalizeAdulthood]

clang-tidy/readability/RedundantControlFlowCheck.cpp

59 ↗

(On Diff #45842)

This keeps coming up. My fingers really want to type braces around bodies of control structures.

If omitting the braces is the "LLVM/clang style", then we should have a clang-tidy check that fixes this :-).

[LegalizeAdulthood]

Update from review comments

[aaron.ballman]

With one small nit to the diagnostic wording for consistency, LGTM! I will go ahead and make that change, then commit on your behalf. You may want to talk to Chris Lattner about getting commit privileges, if you'd like them.

clang-tidy/readability/RedundantControlFlowCheck.cpp

25 ↗

(On Diff #46334)

Our usual diagnostic terminology for this is "void return type" instead of "returning void".

60 ↗

(On Diff #46334)

It is the LLVM coding style (though I don't personally care for it), and you are right -- a clang-tidy check in the LLVM module wouldn't be amiss. :-)

[aaron.ballman]

I have commit in r259362. Thank you!

[alexfh]

Thank you for adding more test cases! Could you address one more comment in a follow-up, please?

test/clang-tidy/readability-redundant-control-flow.cpp

193 ↗

(On Diff #46334)

This test doesn't verify whether the second return is removed or not. Whatever the behavior is, it would be good to make it obvious in the test.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
+  RedundantReturnCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   SimplifyBooleanExprCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "BracesAroundStatementsCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
+#include "RedundantReturnCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "readability-implicit-bool-cast");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<RedundantReturnCheck>(
+        "readability-redundant-return");
     CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
         "readability-uniqueptr-delete-release");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");

clang-tidy/readability/RedundantReturnCheck.h

+//===--- RedundantReturnCheck.h - clang-tidy---------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_RETURN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_RETURN_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Eliminates redundant `return` statements at the end of a function that
+/// returns `void`.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-return.html
+class RedundantReturnCheck : public ClangTidyCheck {
+public:
+  RedundantReturnCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_RETURN_H

clang-tidy/readability/RedundantReturnCheck.cpp

+//===--- RedundantReturnCheck.cpp - clang-tidy-----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantReturnCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void RedundantReturnCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      functionDecl(isDefinition(), returns(asString("void")),
+                   has(compoundStmt(hasAnySubstatement(returnStmt()))))
+          .bind("fn"),
+      this);
+}
+
+void RedundantReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Fn = Result.Nodes.getNodeAs<FunctionDecl>("fn");
+  if (const auto *Block = dyn_cast<CompoundStmt>(Fn->getBody())) {
+    CompoundStmt::const_reverse_body_iterator last = Block->body_rbegin();
+    if (const auto *Return = dyn_cast<ReturnStmt>(*last)) {
+      CompoundStmt::const_reverse_body_iterator Previous =
+          ++Block->body_rbegin();
+      SourceLocation Start;
+      if (Previous != Block->body_rend()) {
+        Start = Lexer::findLocationAfterToken(
+            dyn_cast<Stmt>(*Previous)->getLocEnd(), tok::semi,
+            *Result.SourceManager, Result.Context->getLangOpts(),
+            /*SkipTrailingWhitespaceAndNewLine=*/true);
+      } else {
+        Start = Return->getLocStart();
+      }
+      auto ReturnRange = CharSourceRange::getCharRange(
+          Start, Lexer::findLocationAfterToken(
+                     Return->getLocEnd(), tok::semi, *Result.SourceManager,
+                     Result.Context->getLangOpts(),
+                     /*SkipTrailingWhitespaceAndNewLine=*/true));
+
+      diag(Return->getLocStart(),
+           "redundant return statement at the end of void function")
+          << FixItHint::CreateRemoval(ReturnRange);
+    }
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-braces-around-statements
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
+   readability-redundant-return
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-simplify-boolean-expr
    readability-uniqueptr-delete-release

docs/clang-tidy/checks/readability-redundant-return.rst

+.. title:: clang-tidy - readability-redundant-return
+
+readability-redundant-return
+============================
+
+This check looks for procedures (functions returning no value) with return
+statements at the end of the function.  Such return statements are redundant.

test/clang-tidy/readability-redundant-return.cpp

+// RUN: %check_clang_tidy %s readability-redundant-return %t
+
+void g(int i);
+void j();
+
+void f()
+{
+  return;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: redundant return statement at the end of void function [readability-redundant-return]
+// CHECK-FIXES: {{^}}void f(){{$}}
+// CHECK-FIXES-NEXT: {{^{$}}
+// CHECK-FIXES-NEXT: {{^ *}$}}
+
+void g()
+{
+  f();
+  return;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: redundant return statement
+// CHECK-FIXES: {{^  }}f();{{$}}
+// CHECK-FIXES-NEXT: {{^ *}$}}
+
+void g(int i)
+{
+  if (i < 0) {
+    return;
+  }
+  if (i < 10) {
+    f();
+  }
+}
+
+int h()
+{
+  return 1;
+}
+
+void j()
+{
+}