Enhance clang-tidy readability-simplify-boolean-expr to handle 'if (e) return true; return false;' and improve replacement expressions.

[LegalizeAdulthood]

Summary

This changeset extends the simplify boolean expression check in clang-tidy to simplify if (e) return true; return false; to return e; (note the lack of an else clause on the if statement.) By default, chained conditional assignment is left unchanged, unless a configuration parameter is set to non-zero to override this behavior.

It also improves the handling of replacement expressions to apply static_cast<bool>(expr) when expr is not of type bool.

[LegalizeAdulthood]

It probably makes sense for me to rebase this on top of the other enhancement to simplify boolean after that is pushed.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

224–239

I think this would make a good matcher, but I am unsure how to best express it using the matcher infrastructure. I could find no way in the existing matcher lexicon to say "match compound statements where this statement follows that statement"

[LegalizeAdulthood]

Rebase on latest changes

[LegalizeAdulthood]

Testing this on LLVM/clang indicates that it turns valid code into invalid code, so I will need to address this first. The problem appears that isFollowedBy is reporting true when it should not.

[LegalizeAdulthood]

In D9810#173989, @LegalizeAdulthood wrote:

Testing this on LLVM/clang indicates that it turns valid code into invalid code, so I will need to address this first. The problem appears that isFollowedBy is reporting true when it should not.

This appears to be a regression somewhere outside of my changes.

[alexfh]

In D9810#173999, @LegalizeAdulthood wrote:

In D9810#173989, @LegalizeAdulthood wrote:

Testing this on LLVM/clang indicates that it turns valid code into invalid code, so I will need to address this first. The problem appears that isFollowedBy is reporting true when it should not.

This appears to be a regression somewhere outside of my changes.

So does LLVM/Clang compile and pass all tests after applying all fixes from this check?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

78–80

Why not make the default value "ignored" and remove the ternary below?

[LegalizeAdulthood]

In D9810#174211, @alexfh wrote:

In D9810#173999, @LegalizeAdulthood wrote:

In D9810#173989, @LegalizeAdulthood wrote:

Testing this on LLVM/clang indicates that it turns valid code into invalid code, so I will need to address this first. The problem appears that isFollowedBy is reporting true when it should not.

This appears to be a regression somewhere outside of my changes.

So does LLVM/Clang compile and pass all tests after applying all fixes from this check?

It all compiles, but manual testing with clang-query shows the problem. See https://llvm.org/bugs/show_bug.cgi?id=23552 for details.

[LegalizeAdulthood]

In D9810#174276, @LegalizeAdulthood wrote:

It all compiles, but manual testing with clang-query shows the problem. See https://llvm.org/bugs/show_bug.cgi?id=23552 for details.

There's also the possibility that it's something I messed up in my tree. I'd like to see if anyone else can reproduce the clang-query session in the attachment on that bug.

[alexfh]

In D9810#174289, @LegalizeAdulthood wrote:

In D9810#174276, @LegalizeAdulthood wrote:

It all compiles, but manual testing with clang-query shows the problem. See https://llvm.org/bugs/show_bug.cgi?id=23552 for details.

There's also the possibility that it's something I messed up in my tree. I'd like to see if anyone else can reproduce the clang-query session in the attachment on that bug.

I've replied on the bug.

[LegalizeAdulthood]

After sorting out some things with the clang tree for testing, I discovered that the code had errors. This diff corrects them.

Specifically compoundStmt(hasAnySubstatement(ifStmt())) will always match the first if statement in a compound statement. The matcher will not be invoked for each if statement in the block. So when a block contains chained if statements like:

if (i < 5) return false;
if (i > 10) return true;
return false;

The first if statement would have been matched, but then determined that it did not immediately precede the return statement and the entire block would be left alone. The consequence is that I can use a matcher that finds all compound statements containing both an if statement and a return statement, but I then have to iterate over the compound statement manually to locate the if immediately preceding the return. Test cases have been added to cover this situation. Running the check on clang/LLVM code now does the sensible thing.

[LegalizeAdulthood]

This comment was removed by LegalizeAdulthood.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

78–80

Fixed.

[LegalizeAdulthood]

When the replacement text is already a negated rexpression, !expr, and we're negating it again, use expr, not !!expr as the replacement.

[alexfh]

I'd make an exception for chained conditional returns:

if (X)
  return true;
if (Y)
  return true;
if (Z)
  return true;
return false;

It's arguable whether replacing the last piece with return Z; is an improvement, so I'd better be conservative here.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

268

Please use const auto * to make the constness explicit.

275

Instead of using the return value and additionally return a literal by reference, I'd suggest making the function return llvm::Optional<const CXXBoolLiteralExpr*>.

Same above.

281

Please use const auto * to make the constness explicit. Also two times below.

283

nit: No else after return please.

557

Please use const auto * to make the constness explicit.

test/clang-tidy/readability-simplify-bool-expr.cpp

704

How about making it a single regexp?

{{^  }$}}

Same applies for other similar lines.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

268

Fixed.

275

Any reason to not return just the pointer and compare against nulltpr?

281

Fixed.

283

Fixed.

557

Fixed.

test/clang-tidy/readability-simplify-bool-expr.cpp

704

Fixed.

[LegalizeAdulthood]

Update from review comments and handle chained condition returns of the form

if (expr)
  return true;
if (expr2)
  return true;
return false;

and variants.

[LegalizeAdulthood]

Improve handling of replacement expressions to apply explicit casts to bool where necessary.

[LegalizeAdulthood]

Document transformations applied to replacement expression.

Replace implicit pointer to boolean conversions with explicit comparison to nullptr.

[LegalizeAdulthood]

Remove unnecessary includes.

[alexfh]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

111

Can we use braced initialization instead of std::make_pair ({{OO_EqualEqual, "=="}, ....})? Same below.

141

In clang-tidy code (and in LLVM in general) it's more common to omit braces around single-line if bodies. One exception is when any branch of the if-else chain uses braces, we put them to all branches.

Please fix here and in other places in this CL.

142

This will return a StringRef pointing to a memory consumed by a temporary string. Make the return type std::string to avoid the problem.

254

No else after return, please.

275

If it's enough, sure.

556

Can the body be empty? If not, I'd document this (e.g. using an assert()).

clang-tidy/readability/SimplifyBooleanExprCheck.h

41

nit: parentheses? I didn't find what "parenthesese" is.

53

ditto

54

Did you intentionally skip a number for this paragraph? Sam below after 3.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

556

The body can't be empty because the matcher ensures that it must contain at least two statements:

• A return statement returning a boolean literal false or true
• An if statement with no else clause and a then clause that consists of a single return statement returning the opposite boolean literal true or false

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

141

CERT coding standards require braces around all control structures, so I've routinely adopted that habit. Considering that existing security vulnerabilities were traced to the omission of the braces, I don't simply consider it a matter of style.

However, I'm not in charge of llvm code and if it is really that important, I can change it.

Sometimes these code reviews are simply exhausting because of all this little fiddling about with formatting instead of function.

[alexfh]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

141

CERT coding standards require braces around all control structures, so
I've routinely adopted that habit. Considering that existing security
vulnerabilities were traced to the omission of the braces, I don't simply
consider it a matter of style.

This concern doesn't apply to the code that is routinely automatically formatted, as there's little to no chance of a line with incorrect indentation being considered a part of a branch when it's actually outside.

... if it is really that important ...

Consistency and readability are *really* that important. IMO, braces around single-line bodies of conditions/loops don't hurt readability as long as they are used consistently. Given that the code in this file is going to be modified mostly by you, I'm fine with putting braces around single-line if/for/while bodies as long as you commit to follow this style consistently in the files authored by you (and only there).

Sometimes these code reviews are simply exhausting because of all this
little fiddling about with formatting instead of function.

Luckily, we have clang-format that takes care of the most of formatting. And what's left is not "little fiddling about with formatting" in most cases, but something more important. For example, here we're talking about a style preference "existing security vulnerabilities were traced to" in your own words. Also, I think, most style rules should be easy to internalize, at which point they won't require any special attention.

556

Thanks for the explanation! Would you mind putting an assert then?

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

111

Good idea. Fixed.

141

Fixed.

142

Fixed.

254

FIxed.

556

Fixed.

clang-tidy/readability/SimplifyBooleanExprCheck.h

41

Fixed.

53

Fixed.

54

This should have been associated with the paragraph giving examples of implicit conversion to bool. Fixed.

[LegalizeAdulthood]

Update from comments

[alexfh]

A few minor issues, otherwise looks good.

I'm going to fix the code and commit the patch for you.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

52

This variable and the one below are not used.

141

Thanks for fixing this, but I was talking only about *single-line* bodies. Luckily, the braces that shouldn't have been removed can easily be added back by clang-tidy itself:

clang-tidy -fix -checks=readability-braces-around-statements \
  -config="{CheckOptions: [{key: 'readability-braces-around-statements.ShortStatementLines', value: 2}]}" \
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp

test/clang-tidy/readability-simplify-bool-expr.cpp

604

nit: capitalization, trailing period

[alexfh]

Committed revision 241155.

clang-tidy/readability/SimplifyBooleanExprCheck.h

 /// `e ? true : false`                         becomes `e`
 /// `e ? false : true`                         becomes `!e`
 /// `if (true) t(); else f();`                 becomes `t();`
 /// `if (false) t(); else f();`                becomes `f();`
 /// `if (e) return true; else return false;`   becomes `return e;`
 /// `if (e) return false; else return true;`   becomes `return !e;`
 /// `if (e) b = true; else b = false;`         becomes `b = e;`
 /// `if (e) b = false; else b = true;`         becomes `b = !e;`
+/// `if (e) return true; return false;`        becomes `return e;`
+/// `if (e) return false; return true;`        becomes `return !e;`
 ///
-/// Parenthesis from the resulting expression `e` are removed whenever
-/// possible.
+/// The resulting expression `e` is modified as follows:
+/// 1. Unnecessary parenthesese around the expression are removed.
+/// 2. Negated applications of `!` are eliminated.
+/// 3. Negated applications of comparison operators are changed to use the
+///    opposite condition.
+/// 4. Implicit conversions of pointer to `bool` are replaced with explicit
+///    comparisons to `nullptr`.
+/// 5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
+/// 6. Object expressions with `explicit operator bool` conversion operators
+///    are replaced with explicit casts to `bool`.
+///
+/// Examples:
+/// 1. The ternary assignment `bool b = (i < 0) ? true : false;` has redundant
+/// parenthesese and becomes `bool b = i < 0;`.
+/// The ternary assignment `bool b = (i & 1) ? true : false;` has an implicit
+/// conversion of `i & 1` to `bool` and becomes
+/// `bool b = static_cast<bool>(i & 1);`.
+///
+/// 2. The conditional return `if (!b) return false; return true;` has an
+/// implied double negation and becomes `return b;`.
+///
+/// 3. The conditional return `if (i < 0) return false; return true;` becomes
+/// `return i >= 0;`.
+/// The conditional return `if (i != 0) return false; return true;` becomes
+/// `return i == 0;`.
+///
+/// 4. The conditional return `if (p) return true; return false;` has an
+/// implicit conversion of a pointer to `bool` and becomes
+/// `return p != nullptr;`.
+///
+/// 5. The conditional return `if (i & 1) return true; else return false;` has
+/// an implicit conversion of an integer quantity `i & 1` to `bool` and becomes
+/// `return static_cast<bool>(i & 1);`
+///
+/// 6. Given `struct X { explicit operator bool(); };`, and an instance `x` of
+/// `struct X`, the conditional return `if (x) return true; return false;`
+/// becomes `return static_cast<bool>(x);`
 ///
 /// When a conditional boolean return or assignment appears at the end of a
 /// chain of `if`, `else if` statements, the conditional statement is left
 /// unchanged unless the option `ChainedConditionalReturn` or
 /// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
 /// The default value for both options is zero.
 ///
 class SimplifyBooleanExprCheck : public ClangTidyCheck {
void matchTernaryResult(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef TernaryId);
 
   void matchIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef Id);
 
   void matchIfAssignsBool(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef Id);
 
+  void matchCompoundIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+                                  StringRef Id);
+
   void
   replaceWithExpression(const ast_matchers::MatchFinder::MatchResult &Result,
                         const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS,
                         bool Negated = false);
 
   void
   replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result,
                            const CXXBoolLiteralExpr *BoolLiteral);
private:
   void replaceWithReturnCondition(
       const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If,
       bool Negated = false);
 
   void
   replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
                         const IfStmt *If, bool Negated = false);
 
+  void replaceCompoundReturnWithCondition(
+      const ast_matchers::MatchFinder::MatchResult &Result,
+      const CompoundStmt *Compound, bool Negated = false);
+
   const bool ChainedConditionalReturn;
   const bool ChainedConditionalAssignment;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

 //===--- SimplifyBooleanExpr.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 "SimplifyBooleanExprCheck.h"
 #include "clang/Lex/Lexer.h"
 
-#include <cassert>
+#include <algorithm>
+#include <iterator>
 #include <string>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 

 const char IfReturnsBoolId[] = "if-return";
 const char IfReturnsNotBoolId[] = "if-not-return";
 const char ThenLiteralId[] = "then-literal";
 const char IfAssignVariableId[] = "if-assign-lvalue";
 const char IfAssignLocId[] = "if-assign-loc";
 const char IfAssignBoolId[] = "if-assign";
 const char IfAssignNotBoolId[] = "if-assign-not";
 const char IfAssignObjId[] = "if-assign-obj";
+const char CompoundId[] = "compound";
+const char CompoundIfReturnsBoolId[] = "compound-if";
+const char CompoundReturnId[] = "compound-return";
+const char CompoundBoolId[] = "compound-bool";
+const char CompoundNotBoolId[] = "compound-bool-not";
 
 const char IfStmtId[] = "if";
 const char LHSId[] = "lhs-expr";
 const char RHSId[] = "rhs-expr";
 
 const char SimplifyOperatorDiagnostic[] =
     "redundant boolean literal supplied to boolean operator";
 const char SimplifyConditionDiagnostic[] =
     "redundant boolean literal in if statement condition";
+const char SimplifyConditionalReturnDiagnostic[] =
+    "redundant boolean literal in conditional return statement";
 
 const CXXBoolLiteralExpr *getBoolLiteral(const MatchFinder::MatchResult &Result,
                                          StringRef Id) {
   const auto *Literal = Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id);
   return (Literal &&
           Result.SourceManager->isMacroBodyExpansion(Literal->getLocStart()))
              ? nullptr
              : Literal;
 }
 
-internal::Matcher<Stmt> ReturnsBool(bool Value, StringRef Id = "") {
-  auto SimpleReturnsBool = returnStmt(
-      has(boolLiteral(equals(Value)).bind(Id.empty() ? "ignored" : Id)));
+internal::Matcher<Stmt> returnsBool(bool Value, StringRef Id = "ignored") {
+  auto SimpleReturnsBool =
+      returnStmt(has(boolLiteral(equals(Value)).bind(Id))).bind("returns-bool");
   return anyOf(SimpleReturnsBool,
                compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
 }
 
 bool needsParensAfterUnaryNegation(const Expr *E) {
+  E = E->IgnoreImpCasts();
   if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
     return true;
   if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
     return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
            Op->getOperator() != OO_Subscript;
   return false;
 }
 
 std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
     std::make_pair(BO_LT, BO_GE), std::make_pair(BO_GT, BO_LE),
     std::make_pair(BO_EQ, BO_NE)};
 
 StringRef negatedOperator(const BinaryOperator *BinOp) {
   const BinaryOperatorKind Opcode = BinOp->getOpcode();
   for (auto NegatableOp : Opposites) {
     if (Opcode == NegatableOp.first)
       return BinOp->getOpcodeStr(NegatableOp.second);
     if (Opcode == NegatableOp.second)
       return BinOp->getOpcodeStr(NegatableOp.first);
   }
   return StringRef();
 }
 
+std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
+    std::make_pair(OO_EqualEqual, "=="), std::make_pair(OO_ExclaimEqual, "!="),
+    std::make_pair(OO_Less, "<"),        std::make_pair(OO_GreaterEqual, ">="),
+    std::make_pair(OO_Greater, ">"),     std::make_pair(OO_LessEqual, "<=")};
+
+StringRef getOperatorName(OverloadedOperatorKind OpKind) {
+  for (auto Name : OperatorNames) {
+    if (Name.first == OpKind) {
+      return Name.second;
+    }
+  }
+  return StringRef();
+}
+
+std::pair<OverloadedOperatorKind, OverloadedOperatorKind> OppositeOverloads[] =
+    {std::make_pair(OO_EqualEqual, OO_ExclaimEqual),
+     std::make_pair(OO_Less, OO_GreaterEqual),
+     std::make_pair(OO_Greater, OO_LessEqual)};
+
+StringRef negatedOperator(const CXXOperatorCallExpr *OpCall) {
+  const OverloadedOperatorKind Opcode = OpCall->getOperator();
+  for (auto NegatableOp : OppositeOverloads) {
+    if (Opcode == NegatableOp.first)
+      return getOperatorName(NegatableOp.second);
+    if (Opcode == NegatableOp.second)
+      return getOperatorName(NegatableOp.first);
+  }
+  return StringRef();
+}
+
+StringRef asBool(StringRef text, bool NeedsStaticCast) {
+  if (NeedsStaticCast) {
+    return ("static_cast<bool>(" + text + ")").str();
+  }
+
+  return text;
+}
+
+bool needsNullPtrComparison(const Expr *E) {
+  if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
+    return ImpCast->getCastKind() == CK_PointerToBoolean;
+  }
+  return false;
+}
+
+bool needsStaticCast(const Expr *E) {
+  if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
+    if (ImpCast->getCastKind() == CK_UserDefinedConversion &&
+        ImpCast->getSubExpr()->getType()->isBooleanType()) {
+      if (const auto *MemCall =
+              dyn_cast<CXXMemberCallExpr>(ImpCast->getSubExpr())) {
+        if (const auto *MemDecl =
+                dyn_cast<CXXConversionDecl>(MemCall->getMethodDecl())) {
+          if (MemDecl->isExplicit()) {
+            return true;
+          }
+        }
+      }
+    }
+  }
+  E = E->IgnoreImpCasts();
+  return !E->getType()->isBooleanType();
+}
+
 std::string replacementExpression(const MatchFinder::MatchResult &Result,
                                   bool Negated, const Expr *E) {
-  while (const auto *Parenthesized = dyn_cast<ParenExpr>(E)) {
-    E = Parenthesized->getSubExpr();
-  }
+  E = E->ignoreParenBaseCasts();
+  const bool NeedsStaticCast = needsStaticCast(E);
   if (Negated) {
+    if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
+      if (UnOp->getOpcode() == UO_LNot) {
+        if (needsNullPtrComparison(UnOp->getSubExpr())) {
+          return (getText(Result, *UnOp->getSubExpr()) + " != nullptr").str();
+        }
+        return replacementExpression(Result, false, UnOp->getSubExpr());
+      }
+    } else if (needsNullPtrComparison(E)) {
+      return (getText(Result, *E) + " == nullptr").str();
+    }
+
+    StringRef NegatedOperator;
+    const Expr *LHS = nullptr;
+    const Expr *RHS = nullptr;
     if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
-      StringRef NegatedOperator = negatedOperator(BinOp);
-      if (!NegatedOperator.empty()) {
-        return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator +
-                " " + getText(Result, *BinOp->getRHS())).str();
+      NegatedOperator = negatedOperator(BinOp);
+      LHS = BinOp->getLHS();
+      RHS = BinOp->getRHS();
+    } else if (const auto *OpExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
+      if (OpExpr->getNumArgs() == 2) {
+        NegatedOperator = negatedOperator(OpExpr);
+        LHS = OpExpr->getArg(0);
+        RHS = OpExpr->getArg(1);
       }
     }
+    if (!NegatedOperator.empty() && LHS && RHS) {
+      return (asBool((getText(Result, *LHS) + " " + NegatedOperator + " " +
+                      getText(Result, *RHS)).str(),
+                     NeedsStaticCast)).str();
     }
+
     StringRef Text = getText(Result, *E);
-  return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")"
-                                                      : "!" + Text)
-                  : Text).str();
+    if (!NeedsStaticCast && needsParensAfterUnaryNegation(E)) {
+      return ("!(" + Text + ")").str();
+    }
+
+    if (needsNullPtrComparison(E)) {
+      return (getText(Result, *E) + " == nullptr").str();
+    }
+
+    return ("!" + asBool(Text, NeedsStaticCast)).str();
+  }
+
+  if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
+    if (UnOp->getOpcode() == UO_LNot) {
+      if (needsNullPtrComparison(UnOp->getSubExpr())) {
+        return (getText(Result, *UnOp->getSubExpr()) + " == nullptr").str();
+      }
+    }
+  }
+
+  if (needsNullPtrComparison(E)) {
+    return (getText(Result, *E) + " != nullptr").str();
+  }
+
+  return asBool(getText(Result, *E), NeedsStaticCast).str();
+}
+
+const CXXBoolLiteralExpr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
+  if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
+    if (Bool->getValue() == !Negated) {
+      return Bool;
+    }
+  }
+
+  return nullptr;
+}
+
+const CXXBoolLiteralExpr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
+  if (IfRet->getElse() != nullptr) {
+    return nullptr;
+  }
+
+  if (const auto *Ret = dyn_cast<ReturnStmt>(IfRet->getThen())) {
+    return stmtReturnsBool(Ret, Negated);
+  } else if (const auto *Compound = dyn_cast<CompoundStmt>(IfRet->getThen())) {
+    if (Compound->size() == 1) {
+      if (const auto *CompoundRet =
+              dyn_cast<ReturnStmt>(Compound->body_back())) {
+        return stmtReturnsBool(CompoundRet, Negated);
+      }
+    }
+  }
+
+  return nullptr;
 }
 
 } // namespace
 
 SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       ChainedConditionalReturn(Options.get("ChainedConditionalReturn", 0U)),
       ChainedConditionalAssignment(
           Options.get("ChainedConditionalAssignment", 0U)) {}
 
 void SimplifyBooleanExprCheck::matchBoolBinOpExpr(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef OperatorName,
                                                   StringRef BooleanId) {
   Finder->addMatcher(
       binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName),
                      hasLHS(allOf(expr().bind(LHSId),
                                   boolLiteral(equals(Value)).bind(BooleanId))),
                      hasRHS(expr().bind(RHSId)),
                      unless(hasRHS(hasDescendant(boolLiteral())))),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchExprBinOpBool(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef OperatorName,
                                                   StringRef BooleanId) {
Finder->addMatcher(
           .bind(TernaryId),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   if (ChainedConditionalReturn) {
     Finder->addMatcher(ifStmt(isExpansionInMainFile(),
-                              hasThen(ReturnsBool(Value, ThenLiteralId)),
-                              hasElse(ReturnsBool(!Value))).bind(Id),
+                              hasThen(returnsBool(Value, ThenLiteralId)),
+                              hasElse(returnsBool(!Value))).bind(Id),
                        this);
   } else {
     Finder->addMatcher(ifStmt(isExpansionInMainFile(),
                               unless(hasParent(ifStmt())),
-                              hasThen(ReturnsBool(Value, ThenLiteralId)),
-                              hasElse(ReturnsBool(!Value))).bind(Id),
+                              hasThen(returnsBool(Value, ThenLiteralId)),
+                              hasElse(returnsBool(!Value))).bind(Id),
                        this);
   }
 }
 
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   auto SimpleThen = binaryOperator(
       hasOperatorName("="),
void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
   } else {
     Finder->addMatcher(ifStmt(isExpansionInMainFile(),
                               unless(hasParent(ifStmt())), hasThen(Then),
                               hasElse(Else)).bind(Id),
                        this);
   }
 }
 
+void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
+                                                          bool Value,
+                                                          StringRef Id) {
+  Finder->addMatcher(
+      compoundStmt(
+          allOf(hasAnySubstatement(ifStmt(hasThen(returnsBool(Value)),
+                                          unless(hasElse(stmt())))),
+                hasAnySubstatement(returnStmt(has(boolLiteral(equals(!Value))))
+                                       .bind(CompoundReturnId)))).bind(Id),
+      this);
+}
+
 void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
   Options.store(Opts, "ChainedConditionalAssignment",
                 ChainedConditionalAssignment);
 }
 
 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
   matchBoolBinOpExpr(Finder, true, "&&", RightExpressionId);
void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
   matchTernaryResult(Finder, true, TernaryId);
   matchTernaryResult(Finder, false, TernaryNegatedId);
 
   matchIfReturnsBool(Finder, true, IfReturnsBoolId);
   matchIfReturnsBool(Finder, false, IfReturnsNotBoolId);
 
   matchIfAssignsBool(Finder, true, IfAssignBoolId);
   matchIfAssignsBool(Finder, false, IfAssignNotBoolId);
+
+  matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
+  matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
 }
 
 void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
   if (const CXXBoolLiteralExpr *LeftRemoved =
           getBoolLiteral(Result, RightExpressionId)) {
     replaceWithExpression(Result, LeftRemoved, false);
   } else if (const CXXBoolLiteralExpr *RightRemoved =
                  getBoolLiteral(Result, LeftExpressionId)) {
replaceWithReturnCondition(Result, If);
                  Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId)) {
     replaceWithReturnCondition(Result, IfNot, true);
   } else if (const auto *IfAssign =
                  Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId)) {
     replaceWithAssignment(Result, IfAssign);
   } else if (const auto *IfAssignNot =
                  Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId)) {
     replaceWithAssignment(Result, IfAssignNot, true);
+  } else if (const auto *Compound =
+                 Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId)) {
+    replaceCompoundReturnWithCondition(Result, Compound);
+  } else if (const auto *Compound =
+                 Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId)) {
+    replaceCompoundReturnWithCondition(Result, Compound, true);
   }
 }
 
 void SimplifyBooleanExprCheck::replaceWithExpression(
     const ast_matchers::MatchFinder::MatchResult &Result,
     const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS, bool Negated) {
   const auto *LHS = Result.Nodes.getNodeAs<Expr>(LHSId);
   const auto *RHS = Result.Nodes.getNodeAs<Expr>(RHSId);

 
 void SimplifyBooleanExprCheck::replaceWithReturnCondition(
     const MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated) {
   StringRef Terminator = isa<CompoundStmt>(If->getElse()) ? ";" : "";
   std::string Condition = replacementExpression(Result, Negated, If->getCond());
   std::string Replacement = ("return " + Condition + Terminator).str();
   SourceLocation Start =
       Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getLocStart();
-  diag(Start, "redundant boolean literal in conditional return statement")
+  diag(Start, SimplifyConditionalReturnDiagnostic)
       << FixItHint::CreateReplacement(If->getSourceRange(), Replacement);
 }
 
+void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
+    const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+    bool Negated) {
+  const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
+
+  const IfStmt *BeforeIf = nullptr;
+  CompoundStmt::const_body_iterator Current = Compound->body_begin();
+  CompoundStmt::const_body_iterator After = Compound->body_begin();
+  for (++After; After != Compound->body_end() && *Current != Ret;
+       ++Current, ++After) {
+    if (const auto *If = dyn_cast<IfStmt>(*Current)) {
+      if (const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated)) {
+        if (*After == Ret) {
+          if (!ChainedConditionalReturn && BeforeIf) {
+            continue;
+          }
+
+          const Expr *Condition = If->getCond();
+          std::string Replacement =
+              "return " + replacementExpression(Result, Negated, Condition);
+          diag(Lit->getLocStart(), SimplifyConditionalReturnDiagnostic)
+              << FixItHint::CreateReplacement(
+                  SourceRange(If->getLocStart(), Ret->getLocEnd()),
+                  Replacement);
+          return;
+        }
+
+        BeforeIf = If;
+      }
+    } else {
+      BeforeIf = nullptr;
+    }
+  }
+}
+
 void SimplifyBooleanExprCheck::replaceWithAssignment(
     const MatchFinder::MatchResult &Result, const IfStmt *IfAssign,
     bool Negated) {
   SourceRange Range = IfAssign->getSourceRange();
   StringRef VariableName =
       getText(Result, *Result.Nodes.getNodeAs<Expr>(IfAssignVariableId));
   StringRef Terminator = isa<CompoundStmt>(IfAssign->getElse()) ? ";" : "";
   std::string Condition =

test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp

else if (i > 20)
     return true;
   else
     return false;
   // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement
   // CHECK-FIXES:      {{^}}  else if (i < 10)
   // CHECK-FIXES-NEXT: {{^}}    return false;
   // CHECK-FIXES-NEXT: {{^}}  else return i > 20;
 }
+
+bool chained_simple_if_return(int i) {
+  if (i < 5)
+    return true;
+  if (i > 10)
+    return true;
+  return false;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool chained_simple_if_return(int i) {{{$}}
+// CHECK-FIXES: {{^}}  if (i < 5){{$}}
+// CHECK-FIXES: {{^    return true;$}}
+// CHECK-FIXES: {{^  return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool chained_simple_if_return_negated(int i) {
+  if (i < 5)
+    return false;
+  if (i > 10)
+    return false;
+  return true;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool chained_simple_if_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^}}  if (i < 5){{$}}
+// CHECK-FIXES: {{^    return false;$}}
+// CHECK-FIXES: {{^  return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_chained_if_return_return(int i) {
+  if (i < 5) {
+    return true;
+  }
+  if (i > 10) {
+    return true;
+  }
+  return false;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_chained_if_return_return(int i) {{{$}}
+// CHECK-FIXES: {{^}}  if (i < 5) {{{$}}
+// CHECK-FIXES: {{^}}    return true;{{$}}
+// CHECK-FIXES: {{^}}  }{{$}}
+// CHECK-FIXES: {{^  return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_chained_if_return_return_negated(int i) {
+  if (i < 5) {
+    return false;
+  }
+  if (i > 10) {
+    return false;
+  }
+  return true;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_chained_if_return_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^}}  if (i < 5) {{{$}}
+// CHECK-FIXES: {{^}}    return false;{{$}}
+// CHECK-FIXES: {{^}}  }{{$}}
+// CHECK-FIXES: {{^  return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}

test/clang-tidy/readability-simplify-bool-expr.cpp

 
 bool negated_conditional_return_statements(int i) {
   if (i == 0) return false; else return true;
 }
 // CHECK-MESSAGES: :[[@LINE-2]]:22: warning: {{.*}} in conditional return statement
 // CHECK-FIXES:      {{^}}  return i != 0;{{$}}
 // CHECK-FIXES-NEXT: {{^}$}}
 
+bool negative_condition_conditional_return_statement(int i) {
+  if (!(i == 0)) return false; else return true;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: {{.*}} in conditional return statement
+// CHECK-FIXES:      {{^}}  return i == 0;{{$}}
+// CHECK-FIXES-NEXT: {{^}$}}
+
 bool conditional_compound_return_statements(int i) {
   if (i == 1) {
     return true;
   } else {
     return false;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return statement
if (i < 0)
     b = true;
   else if (i < 10)
     b = false;
   else if (i > 20)
     b = true;
   else
     b = false;
 }
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_simple_if_return_negated(int i) {
+  if (i < 5)
+    return false;
+  if (i > 10)
+    return false;
+  return true;
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool complex_chained_if_return_return(int i) {
+  if (i < 5) {
+    return true;
+  }
+  if (i > 10) {
+    return true;
+  }
+  return false;
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool complex_chained_if_return_return_negated(int i) {
+  if (i < 5) {
+    return false;
+  }
+  if (i > 10) {
+    return false;
+  }
+  return true;
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_simple_if_return(int i) {
+  if (i < 5)
+    return true;
+  if (i > 10)
+    return true;
+  return false;
+}
+
+bool simple_if_return_return(int i) {
+  if (i > 10)
+    return true;
+  return false;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool simple_if_return_return(int i) {{{$}}
+// CHECK-FIXES: {{^  return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool simple_if_return_return_negated(int i) {
+  if (i > 10)
+    return false;
+  return true;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool simple_if_return_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^  return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_if_return_return(int i) {
+  if (i > 10) {
+    return true;
+  }
+  return false;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_if_return_return(int i) {{{$}}
+// CHECK-FIXES: {{^  return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_if_return_return_negated(int i) {
+  if (i > 10) {
+    return false;
+  }
+  return true;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_if_return_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^  return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool if_implicit_bool_expr(int i) {
+  if (i & 1) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return static_cast<bool>(i & 1);{{$}}
+
+bool negated_if_implicit_bool_expr(int i) {
+  if (i - 1) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return !static_cast<bool>(i - 1);{{$}}
+
+bool implicit_int(int i) {
+  if (i) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return static_cast<bool>(i);{{$}}
+
+bool explicit_bool(bool b) {
+  if (b) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return b;{{$}}
+
+class Implicit {
+public:
+  operator bool() {
+    return true;
+  }
+};
+
+bool object_bool_implicit_conversion(Implicit O) {
+  if (O) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return O;{{$}}
+
+bool negated_explicit_bool(bool b) {
+  if (!b) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return !b;{{$}}
+
+bool bitwise_complement_conversion(int i) {
+  if (~i) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return static_cast<bool>(~i);{{$}}
+
+bool logical_or(bool a, bool b) {
+  if (a || b) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return a || b;{{$}}
+
+bool logical_and(bool a, bool b) {
+  if (a && b) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return a && b;{{$}}
+
+class Comparable
+{
+public:
+  bool operator==(Comparable const &rhs) { return true; }
+  bool operator!=(Comparable const &rhs) { return false; }
+};
+
+bool comparable_objects() {
+  Comparable c;
+  Comparable d;
+  if (c == d) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return c == d;{{$}}
+
+bool negated_comparable_objects() {
+  Comparable c;
+  Comparable d;
+  if (c == d) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return c != d;{{$}}
+
+struct X {
+  explicit operator bool();
+};
+
+void explicit_conversion_assignment(X x) {
+  bool y;
+  if (x) {
+    y = true;
+  } else {
+    y = false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{^  bool y;$}}
+// CHECK-FIXES: {{^}}  y = static_cast<bool>(x);{{$}}
+
+void ternary_integer_condition(int i) {
+  bool b = i ? true : false;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: bool b = static_cast<bool>(i);{{$}}
+
+bool non_null_pointer_condition(int *p1) {
+  if (p1) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p1 != nullptr;{{$}}
+
+bool null_pointer_condition(int *p2) {
+  if (!p2) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p2 == nullptr;{{$}}
+
+bool negated_non_null_pointer_condition(int *p3) {
+  if (p3) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p3 == nullptr;{{$}}
+
+bool negated_null_pointer_condition(int *p4) {
+  if (!p4) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p4 != nullptr;{{$}}