[clang-tidy] Readability check for redundant parenthesis in return expression.

[adek05]

Summary

Follows rough guidelines from PR22416. So far, I use very simple measure for expressions's simplicity. If it doesn't have > 1 children it is a simple one. Probably it also makes sense to check how deep is the tree if there is just one immediate child. This would catch situations like return (!(*foo));

Depends on D16278 which adds ParenExpr matcher.

[adek05]

drop -check in the registered name for the checker

[omtcyf0]

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

50

Both warnings correspond to the same issue. Therefore I'd suggest creating one warning only.

[LegalizeAdulthood]

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

31

Current AST matcher reference doesn't list parenExpr. That's what I use to find all the matchers. Why isn't this one documented?

41–44

Rather than using an ad-hoc heuristic, I'd rather the check just do it.

If we must have an ad-hoc heuristic, then we should have a configuration parameter that configures the metric ChildrenCountThreshold, where some value like 0 means "always do it" and this value should be the default value for this parameter.

If your return expression is so huge that you think you need extra parentheses to clarify something, then the problem isn't made any worse by removing the outer-most set of parentheses. The real problem of readability is your huge expression. Extracting subexpressions into a variable with an intention-revealing name is a better approach to readability. This is beyond the scope of this check.

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

24

I don't know if it makes a difference, but I added a test case for:

return(1);

as well. In that case we need to remove the parens but add a space between the return and it's expression.

[LegalizeAdulthood]

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

31

Oh, I missed in your description that this depends on another changeset that adds the matcher. Great!

[adek05]

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

31

I added it: D16278. Waiting to have it committed.

41–44

So you prefer moving this logic into check function? I separated that for the sake of readability. I do not mean that this is what we need. But for example we could expand the isComplexExpression to allow for return (!*foo); to be in parens (deep expression tree). I agree, that configuration param makes sense in such case. Which way would you prefer it?

50

I'll try to merge them.

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

24

Yeah... that's an odd case. Ideally I would defer to clang-format here which could first add space between return and (1), but since it is easy enough I could just always do s/)/ / since clang-format is expected to be run after clang-tidy anyway. Good catch!

[Eugene.Zelenko]

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

24

I encountered such statements in my work code base. Unfortunately I could not use Clang-format, since we standardized on Artistic Style, and its default didn't fix excessive spacing :-(

[LegalizeAdulthood]

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

41–44

I don't see return (!*foo); as any clearer than return !*foo;. It's the !* that is the source of the complexity and adding parens just makes it look even more complicated, so I would rather that clang-tidy just remove the parentheses always.

In this case, I would "do the simplest thing that could possibly work" and always remove the parentheses.

Specifically, in this changeset I'm proposing that the function isComplexExpression be removed and the if statement on lines 41-44 also be removed and that the check simply always removes the parentheses.

50

What I would do is replace this with a single diagnostic:

diag(LParenLoc, "redundant parentheses for expression in return statement")
      << FixItHint::CreateReplacement(LParenLoc, "")
      << FixitHint::CreateReplacement(RParenLoc, "");

Also, I think the FixitHint stuff has a way of expressing a deletion directly instead of replace some text with empty string.

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

24

In general, I wouldn't want to rely on clang-format to pretty up the input to clang-tidy for exactly the reasons that there are many code bases where it is unacceptable to run clang-format on the code base. At least not until clang-format subsumes all the functionality of other existing formatters (and it is far from that currently).

IMO, clang-tidy should be agnostic as to input formatting for what it can handle.

[adek05]

@LegalizeAdulthood, @Eugene.Zelenko and @omtcyf0 comments

[aaron.ballman]

I think this check is a bit too aggressive about suggesting removal of parens. For instance:

return (foo && (bar || baz));

It seems reasonable that with more complex subexpressions involving operators of differing precedence that a user may want to keep the outer parens for clarity and that removing them actually reduces readability. Perhaps an option is in order (uncertain what to default its value to) to support that?

Also, how well does this interact with macro expansions, where parens are like candy?

#define FOO(x) (x)

void f() {
  return FOO(12)
}

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

27

No need for ast_matchers.

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

25

IMO, clang-tidy should be agnostic as to input formatting for what it can handle.

Strongly agreed.

[alexfh]

A high-level comment: why do we want to limit this check to only remove parentheses from return ()? Anything wrong with removing unnecessary parentheses everywhere (except for macro bodies)?

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

29

Please use const auto *: repeating the type name doesn't make anything clearer.

32

SourceLocations are small and trivially copyable, so they should be passed by value.

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

2

Please add tests with macros:

1. where superfluous parentheses are in the macro body (and they usually shouldn't be removed);
2. where superfluous parentheses are in a macro argument (which is not itself located in a macro body (e.g. EXPECT_EQ(x, (y + z));) - I'd say that in this case we usually can remove parentheses;

Please add a test to ensure replacements are only applied once in templates with multiple instantiations.

[LegalizeAdulthood]

With readability checks, there are always people who disagree about what makes for more readable code. I think I have seen this back-and-forth discussion with every readability check since I've been paying attention. It simply isn't possible to make everyone happy and we shouldn't even try.

As I stated earlier on this review, IMO the way to deal with complex expressions is not to shotgun blast extra parentheses into the expression, but to extract variables or functions with intention revealing names that break out meaningful subexpressions.

For checks relating to readability or style, I think it is fair to simply assume that the results are subjective. At the risk of sounding tautological, those who don't want the transformation shouldn't ask clang-tidy to perform the transformation.

There are already tons of transformations that clang-tidy performs that aren't to my personal taste, but are there because of a particular style guide (LLVM, google, etc.) or because the author of the check desires the transformation that is implemented.

I don't think such transformations should be denied from clang-tidy simply because of a difference of opinion.

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

[LegalizeAdulthood]

@alexfh: The check and the bug report originate in the preferences of the google style guide.

[aaron.ballman]

In D16286#330360, @LegalizeAdulthood wrote:

With readability checks, there are always people who disagree about what makes for more readable code. I think I have seen this back-and-forth discussion with every readability check since I've been paying attention. It simply isn't possible to make everyone happy and we shouldn't even try.

It is perfectly reasonable for people to disagree about what makes code more or less readable. Some readability issues are more obvious than others, and the threshold for "readable" is going to change from check to check. However, that is not a reason to not attempt to make a check that makes everyone reasonably (un)happy.

As I stated earlier on this review, IMO the way to deal with complex expressions is not to shotgun blast extra parentheses into the expression, but to extract variables or functions with intention revealing names that break out meaningful subexpressions.

That seems reasonable to me, but this checker is *removing* parens, not adding them. The point I was making before is that when a subexpression is sufficiently complex, parens are a quick-and-dirty way to reduce cognitive load for understanding operator precedence. This is an approach you see relatively frequently -- a quick look at the LLVM source base shows that. For a check that claims it will make something more readable, removing code the user wrote had better have a pretty good chance of improving readability or else the check will quickly get disabled.

A good way to empirically test this is to run this check over the LLVM source base (and/or a few other large source bases) and see how often the diagnostic is triggered, and take a random sampling to see how often the removal of parens involves an expression with multiple operators of differing precedence to see if perhaps an option or design change is warranted or not. These sort of metrics are something we usually look for with any clang-tidy check that may have a high false-positive rate. For stylistic checks, I suppose just about anything could qualify as a false-positive (unless it's part of a style guideline that's being followed).

For checks relating to readability or style, I think it is fair to simply assume that the results are subjective.

Definitely agreed.

At the risk of sounding tautological, those who don't want the transformation shouldn't ask clang-tidy to perform the transformation.

That presumes the user knows what transformations this check will apply. If this was misc-remove-redundant-parens or some-style-guide-remove-redundant-parens, I would agree with your statement. However, with readability-* I think the user is likely to ask clang-tidy to perform the check and apply changes on a case-by-case basis. So we may want to start with the smallest set of cases where the check might increase readability and then expand from there to cover more cases as uses are requested.

There are already tons of transformations that clang-tidy performs that aren't to my personal taste, but are there because of a particular style guide (LLVM, google, etc.) or because the author of the check desires the transformation that is implemented.

I don't think such transformations should be denied from clang-tidy simply because of a difference of opinion.

No one is recommending denying this transformation; just discussing the particulars of it to ensure we have reasonable semantics for the check.

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

If it is meant to work for a particular style guide, then it should absolutely go under that style guide. But for something more generally under the readability-* umbrella, I think it doesn't hurt to discuss where to draw the line, or what options may be valuable.

[LegalizeAdulthood]

As a general comment, I am continually fascinated by the huge variety of opinion as to matters of readability within the C++ community. I don't know why we have so many differing opinions compared to the communities built around other programming languages, but I enjoy hearing all of them. I enjoy having my opinions on readability challenged because it forces me to think why I find one form preferable over another and that is a good thing.

In D16286#330390, @aaron.ballman wrote:

In D16286#330360, @LegalizeAdulthood wrote:

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

If it is meant to work for a particular style guide, then it should absolutely go under that style guide. But for something more generally under the readability-* umbrella, I think it doesn't hurt to discuss where to draw the line, or what options may be valuable.

I think that is the correct conclusion here; according to the bug report this check is motivated by the Google style guide which explicitly prohibits the parens for return expressions.

[aaron.ballman]

In D16286#333911, @LegalizeAdulthood wrote:

As a general comment, I am continually fascinated by the huge variety of opinion as to matters of readability within the C++ community. I don't know why we have so many differing opinions compared to the communities built around other programming languages, but I enjoy hearing all of them. I enjoy having my opinions on readability challenged because it forces me to think why I find one form preferable over another and that is a good thing.

Likewise! I think these discussions have been great (and I apologize if they are a source of frustration while we wrestle with the details).

In D16286#330390, @aaron.ballman wrote:

In D16286#330360, @LegalizeAdulthood wrote:

One could argue that this check should be part of the google module. The google style guide specifically forbids writing return with extra parenthesis.

If it is meant to work for a particular style guide, then it should absolutely go under that style guide. But for something more generally under the readability-* umbrella, I think it doesn't hurt to discuss where to draw the line, or what options may be valuable.

I think that is the correct conclusion here; according to the bug report this check is motivated by the Google style guide which explicitly prohibits the parens for return expressions.

Then perhaps this should go under google-* instead of readability-*? However, I posed an idea in a different review about a more general solution to the problem that may make sense under readability-* with specific options for an alias under google-*. Perhaps it would make sense to make this a general "strip *all* parens that don't have an effect on the order of evaluation in all expressions" check, which leaves room for an additional (different) check in the future that says "put parens in anywhere based on this heuristic we can debate in the future". I would say that these two checks should be off-by-default since they will definitely conflict, but I think the alias under google-* should be on by default and only focus on return statements (or whatever else the google style guide requires).

(Not saying that you have to implement this -- I'm just exploring the design space. The initial implementation of this idea could simply be to take your existing check and put it under google-* and we can refactor some day in the future for a more generalized readability-* check.)

[owenpan]

clang-format can remove some types of redundant parens now. See D154484.

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
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
+  ReturnWithRedundantParensCheck.cpp
   SimplifyBooleanExprCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
+#include "ReturnWithRedundantParensCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");
     CheckFactories.registerCheck<SimplifyBooleanExprCheck>(
         "readability-simplify-boolean-expr");
+    CheckFactories.registerCheck<ReturnWithRedundantParensCheck>(
+        "readability-return-with-redundant-parens");
   }
 };
 
 // Register the ReadabilityModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ReadabilityModule>
     X("readability-module", "Adds readability-related checks.");
 
 } // namespace readability
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ReadabilityModule.
 volatile int ReadabilityModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/readability/ReturnWithRedundantParensCheck.h

+//===--- ReturnWithRedundantParensCheck.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_RETURNBRACKETSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant parenthesis surrounding returned expression.
+///
+/// Examples:
+/// \code
+///   void f() { return (1); } ==> void f() { return 1; }
+/// \endcode
+class ReturnWithRedundantParensCheck : public ClangTidyCheck {
+public:
+  ReturnWithRedundantParensCheck(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_RETURNBRACKETSCHECK_H

clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

+//===--- ReturnWithRedundantParensCheck.cpp - clang-tidy --------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ReturnWithRedundantParensCheck.h"
+
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+
+// Ad-hoc measure of complexity of the expression.
+bool isComplexExpression(const Expr *Expr) {
+  const int ChildrenCountThreshold = 1;
+  auto ChildrenNum = std::distance(Expr->child_begin(), Expr->child_end());
+  return ChildrenNum > ChildrenCountThreshold;
+}
+}
+
+void ReturnWithRedundantParensCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(returnStmt(hasDescendant(parenExpr().bind("paren_expr"))),
+                     this);
+}
+
+void ReturnWithRedundantParensCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+
+  const ParenExpr *ParenExpression =
+      Result.Nodes.getNodeAs<ParenExpr>("paren_expr");
+
+  // Complex expression can be surrounded by parenthesis for readability
+  if (isComplexExpression(ParenExpression->getSubExpr())) {
+    return;
+  }
+
+  const SourceLocation &LParenLoc = ParenExpression->getLParen();
+  const SourceLocation &RParenLoc = ParenExpression->getRParen();
+  diag(LParenLoc, "redundant '(' for expression in return statement")
+      << FixItHint::CreateReplacement(LParenLoc, "");
+  diag(RParenLoc, "redundant ')' for expression in return statement")
+      << FixItHint::CreateReplacement(RParenLoc, "");
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

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

+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int no_parens() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: redundant '(' for expression in return statement [readability-return-with-redundant-parens]
+  // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: redundant ')' for expression in return statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int simple2() {
+  int a = 0;
+  return (a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: redundant '(' for expression in return statement [readability-return-with-redundant-parens]
+  // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: redundant ')' for expression in return statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a;{{$}}
+}
+
+int complex1() {
+  return (1 + 2);
+}