[clang-tidy] Extend simplify-boolean-expr check

[njames93]

Summary

Extend the check to catch these conditions:

if (e) return true; else return x;  // return e || x;
if (e) return false; else return x; // return !e && x;
if (e) return true; return x;       // return e || x;
if (e) return false; return x;      // return !e && x;

[njames93]

Remove unnecessary includes

[alexfh]

FWIW, I'm not sure this will usually be a simplification of the code. Splitting the condition and using early return is quite a frequent recommendation in code reviews, and I would expect a strong pushback to automated suggestions going in the opposite direction.

[alexfh]

For example, LLVM coding standards recommends using early exits: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

[LegalizeAdulthood]

In D133102#3774151, @alexfh wrote:

For example, LLVM coding standards recommends using early exits: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

How is the proposed replacement not an early exit? It's an unconditional exit: it either returns a bool constant or it returns a variable that is a bool.

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

void replaceWithElseStatement(const ASTContext &Context,
                                 const Expr *BoolLiteral);
 
   void replaceWithCondition(const ASTContext &Context,
                             const ConditionalOperator *Ternary, bool Negated);
 
   void replaceWithReturnCondition(const ASTContext &Context, const IfStmt *If,
                                   const Expr *BoolLiteral, bool Negated);
 
+  void replaceWithReturnComplexCondition(const ASTContext &Context,
+                                         const IfStmt *If, bool Negated,
+                                         const Expr *ReturnValue);
+
   void replaceWithAssignment(const ASTContext &Context, const IfStmt *If,
                              const Expr *Var, SourceLocation Loc, bool Negated);
 
   void replaceCompoundReturnWithCondition(const ASTContext &Context,
                                           const ReturnStmt *Ret, bool Negated,
                                           const IfStmt *If,
                                           const Expr *ThenReturn);
 

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

template <typename Node> struct NodeAndBool {
     bool Bool = false;
 
     operator bool() const { return Item != nullptr; }
   };
 
   using ExprAndBool = NodeAndBool<Expr>;
   using DeclAndBool = NodeAndBool<Decl>;
 
+  static const ReturnStmt *getAsNonVoidReturn(const Stmt *S) {
+    if (const auto *RS = dyn_cast<ReturnStmt>(S); RS && RS->getRetValue())
+      return RS;
+    return nullptr;
+  }
+
   /// Detect's return (true|false|!true|!false);
   static ExprAndBool parseReturnLiteralBool(const Stmt *S) {
-    const auto *RS = dyn_cast<ReturnStmt>(S);
-    if (!RS || !RS->getRetValue())
-      return {};
+    const ReturnStmt *RS = getAsNonVoidReturn(S);
+    if (RS) {
       if (Optional<bool> Ret =
               getAsBoolLiteral(RS->getRetValue()->IgnoreImplicit(), false)) {
         return {RS->getRetValue(), *Ret};
       }
+    }
     return {};
   }
 
   /// If \p S is not a \c CompoundStmt, applies F on \p S, otherwise if there is
   /// only 1 statement in the \c CompoundStmt, applies F on that single
   /// statement.
   template <typename Functor>
   static auto checkSingleStatement(Stmt *S, Functor F) -> decltype(F(S)) {
if (Optional<bool> Bool = getAsBoolLiteral(Cond, true)) {
       else
         Check->replaceWithElseStatement(Context, If, Cond);
     }
 
     if (If->getElse()) {
       /*
        * if (Cond) return true; else return false; -> return Cond;
        * if (Cond) return false; else return true; -> return !Cond;
+       * if (Cond) return true; else return X;     -> return Cond || X;
+       * if (Cond) return false; else return X;    -> return !Cond && X;
        */
       if (ExprAndBool ThenReturnBool =
               checkSingleStatement(If->getThen(), parseReturnLiteralBool)) {
-        ExprAndBool ElseReturnBool =
-            checkSingleStatement(If->getElse(), parseReturnLiteralBool);
-        if (ElseReturnBool && ThenReturnBool.Bool != ElseReturnBool.Bool) {
-          if (Check->ChainedConditionalReturn ||
-              !isa_and_nonnull<IfStmt>(parent())) {
-            Check->replaceWithReturnCondition(Context, If, ThenReturnBool.Item,
-                                              ElseReturnBool.Bool);
+            !isa_and_present<IfStmt>(parent())) {
+          if (const ReturnStmt *ElseReturn =
+                  checkSingleStatement(If->getElse(), getAsNonVoidReturn)) {
+            if (auto RetBool =
+                    getAsBoolLiteral(ElseReturn->getRetValue(), false)) {
+              if (*RetBool != ThenReturnBool.Bool) {
+                Check->replaceWithReturnCondition(
+                    Context, If, ThenReturnBool.Item, *RetBool);
+              }
+            } else {
+              Check->replaceWithReturnComplexCondition(
+                  Context, If, !ThenReturnBool.Bool, ElseReturn->getRetValue());
+            }
           }
         }
       } else {
         /*
          * if (Cond) A = true; else A = false; -> A = Cond;
          * if (Cond) A = false; else A = true; -> A = !Cond;
          */
         Expr *Var = nullptr;
bool VisitCompoundStmt(CompoundStmt *CS) {
     if (CS->size() < 2)
       return true;
     bool CurIf = false, PrevIf = false;
     for (auto First = CS->body_begin(), Second = std::next(First),
               End = CS->body_end();
          Second != End; ++Second, ++First) {
       PrevIf = CurIf;
       CurIf = isa<IfStmt>(*First);
-      ExprAndBool TrailingReturnBool = parseReturnLiteralBool(*Second);
-      if (!TrailingReturnBool)
+      auto *Ret = dyn_cast<ReturnStmt>(*Second);
+      if (!Ret || !Ret->getRetValue())
         continue;
+      auto ReturnBoolValue =
+          getAsBoolLiteral(Ret->getRetValue()->IgnoreImplicit(), false);
 
       if (CurIf) {
         /*
          * if (Cond) return true; return false; -> return Cond;
          * if (Cond) return false; return true; -> return !Cond;
+         * if (Cond) return true; return X;     -> return Cond || X;
+         * if (Cond) return false; return X;    -> return !Cond && X;
          */
         auto *If = cast<IfStmt>(*First);
         if (!If->hasInitStorage() && !If->hasVarStorage()) {
           ExprAndBool ThenReturnBool =
               checkSingleStatement(If->getThen(), parseReturnLiteralBool);
-          if (ThenReturnBool &&
-              ThenReturnBool.Bool != TrailingReturnBool.Bool) {
-            if (Check->ChainedConditionalReturn ||
-                (!PrevIf && If->getElse() == nullptr)) {
+          if (!ThenReturnBool || (!Check->ChainedConditionalReturn &&
+                                  (PrevIf || If->getElse() != nullptr)))
+            continue;
+          if (ReturnBoolValue) {
+            if (ThenReturnBool.Bool != *ReturnBoolValue) {
               Check->replaceCompoundReturnWithCondition(
-                  Context, cast<ReturnStmt>(*Second), TrailingReturnBool.Bool,
-                  If, ThenReturnBool.Item);
+                  Context, cast<ReturnStmt>(*Second), *ReturnBoolValue, If,
+                  ThenReturnBool.Item);
             }
+          } else {
+            Check->replaceWithReturnComplexCondition(
+                Context, If, !ThenReturnBool.Bool, Ret->getRetValue());
           }
         }
+
       } else if (isa<LabelStmt, CaseStmt, DefaultStmt>(*First)) {
         /*
          * (case X|label_X|default): if (Cond) return BoolLiteral;
          *                           return !BoolLiteral
          */
         Stmt *SubStmt =
             isa<LabelStmt>(*First)  ? cast<LabelStmt>(*First)->getSubStmt()
             : isa<CaseStmt>(*First) ? cast<CaseStmt>(*First)->getSubStmt()
                                     : cast<DefaultStmt>(*First)->getSubStmt();
         auto *SubIf = dyn_cast<IfStmt>(SubStmt);
         if (SubIf && !SubIf->getElse() && !SubIf->hasInitStorage() &&
             !SubIf->hasVarStorage()) {
           ExprAndBool ThenReturnBool =
               checkSingleStatement(SubIf->getThen(), parseReturnLiteralBool);
-          if (ThenReturnBool &&
-              ThenReturnBool.Bool != TrailingReturnBool.Bool) {
+          if (!ThenReturnBool)
+            continue;
+          if (ReturnBoolValue) {
+            if (ThenReturnBool.Bool != *ReturnBoolValue) {
               Check->replaceCompoundReturnWithCondition(
-                Context, cast<ReturnStmt>(*Second), TrailingReturnBool.Bool,
-                SubIf, ThenReturnBool.Item);
+                  Context, cast<ReturnStmt>(*Second), *ReturnBoolValue, SubIf,
+                  ThenReturnBool.Item);
+            }
+          } else {
+            Check->replaceWithReturnComplexCondition(
+                Context, SubIf, !ThenReturnBool.Bool, Ret->getRetValue());
           }
         }
       }
     }
     return true;
   }
 
   static bool isUnaryLNot(const Expr *E) {
void SimplifyBooleanExprCheck::replaceWithReturnCondition(
   std::string Condition =
       replacementExpression(Context, Negated, If->getCond());
   std::string Replacement = ("return " + Condition + Terminator).str();
   SourceLocation Start = BoolLiteral->getBeginLoc();
   issueDiag(Context, Start, SimplifyConditionalReturnDiagnostic,
             If->getSourceRange(), Replacement);
 }
 
+static bool flipDemorganSide(SmallVectorImpl<FixItHint> &Fixes,
+                             const ASTContext &Ctx, const Expr *E,
+                             Optional<BinaryOperatorKind> OuterBO);
+
+void SimplifyBooleanExprCheck::replaceWithReturnComplexCondition(
+    const ASTContext &Context, const IfStmt *If, bool Negated,
+    const Expr *ReturnValue) {
+  auto D = diag(If->getBeginLoc(), SimplifyConditionalReturnDiagnostic);
+  SmallVector<FixItHint, 8> Fixes;
+  Fixes.push_back(FixItHint::CreateReplacement(
+      {If->getBeginLoc(), If->getLParenLoc()}, "return "));
+  if (Negated && flipDemorganSide(Fixes, Context, If->getCond(),
+                                  Negated ? BO_LAnd : BO_LOr))
+    return;
+  Fixes.push_back(FixItHint::CreateReplacement(
+      {If->getRParenLoc(), ReturnValue->getBeginLoc().getLocWithOffset(-1)},
+      Negated ? " && " : " || "));
+  if (If->getElse()) {
+    if (const auto *CS = dyn_cast<CompoundStmt>(If->getElse())) {
+      Fixes.push_back(FixItHint::CreateRemoval(CS->getRBracLoc()));
+    }
+  }
+  D << Fixes;
+}
+
 void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
     const ASTContext &Context, const ReturnStmt *Ret, bool Negated,
     const IfStmt *If, const Expr *ThenReturn) {
   const std::string Replacement =
       "return " + replacementExpression(Context, Negated, If->getCond());
   issueDiag(Context, ThenReturn->getBeginLoc(),
             SimplifyConditionalReturnDiagnostic,
             SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
static bool flipDemorganOperator(llvm::SmallVectorImpl<FixItHint> &Output,
   return false;
 }
 
 static BinaryOperatorKind getDemorganFlippedOperator(BinaryOperatorKind BO) {
   assert(BinaryOperator::isLogicalOp(BO));
   return BO == BO_LAnd ? BO_LOr : BO_LAnd;
 }
 
-static bool flipDemorganSide(SmallVectorImpl<FixItHint> &Fixes,
-                             const ASTContext &Ctx, const Expr *E,
-                             Optional<BinaryOperatorKind> OuterBO);
-
 /// Inverts \p BinOp, Removing \p Parens if they exist and are safe to remove.
 /// returns \c true if there is any issue building the Fixes, \c false
 /// otherwise.
 static bool flipDemorganBinaryOperator(SmallVectorImpl<FixItHint> &Fixes,
                                        const ASTContext &Ctx,
                                        const BinaryOperator *BinOp,
                                        Optional<BinaryOperatorKind> OuterBO,
                                        const ParenExpr *Parens = nullptr) {

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`modernize-use-emplace <clang-tidy/checks/modernize/use-emplace>`
   ``emplace`` or ``emplace_front``.
 
 - Improved :doc:`modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default>`
   check.
 
   The check now skips unions since in this case a default constructor with empty body
   is not equivalent to the explicitly defaulted one.
 
+- Improved :doc:`readability-simplify-boolean-expr 
+  <clang-tidy/checks/readability/simplify-boolean-expr>` check.
+
+  The check can now transform ``if (Cond) return true; else return Something;`` 
+  into ``return Cond || Something;`` and other similar cases.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst

 ``if (b || true)``                             ``if (true)``
 ``if (b || false)``                            ``if (b)``
 ``e ? true : false``                           ``e``
 ``e ? false : true``                           ``!e``
 ``if (true) t(); else f();``                   ``t();``
 ``if (false) t(); else f();``                  ``f();``
 ``if (e) return true; else return false;``     ``return e;``
 ``if (e) return false; else return true;``     ``return !e;``
+``if (e) return true; else return x;``         ``return e || x;``
+``if (e) return false; else return x;``        ``return !e && x;``
 ``if (e) b = true; else b = false;``           ``b = e;``
 ``if (e) b = false; else b = true;``           ``b = !e;``
 ``if (e) return true; return false;``          ``return e;``
 ``if (e) return false; return true;``          ``return !e;``
+``if (e) return true; return x;``              ``return e || x;``
+``if (e) return false; return x;``             ``return !e && x;``
 ``!(!a || b)``                                 ``a && !b``
 ``!(a || !b)``                                 ``!a && b``
 ``!(!a || !b)``                                ``a && b``
 ``!(!a && b)``                                 ``a || !b``
 ``!(a && !b)``                                 ``!a || b``
 ``!(!a && !b)``                                ``a || b``
 ===========================================  ================
 

clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp

case 16:
     return false;
 
   case 17:
     if (j > 10)
       return false;
     return false;
     return true;
 
+  case 18:
+    if (j > 10)
+      return true;
+    return j < 5;
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: {{.*}} in conditional return
+    // CHECK-FIXES: return j > 10 ||  j < 5;
+
+  case 19:
+    if (j > 10)
+      return false;
+    return j > 5;
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: {{.*}} in conditional return
+    // CHECK-FIXES: return j <= 10 &&  j > 5;
+
   case 100: {
     if (b == true)
       j = 10;
     break;
     // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
     // CHECK-FIXES: {{if \(b\)}}
     // CHECK-FIXES-NEXT: {{j = 10;}}
   }

clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp

 // CHECK-MESSAGES: :[[@LINE-2]]:22: warning: {{.*}} in conditional return statement
 // CHECK-FIXES:      {{^}}  return i == 0;{{$}}
 // CHECK-FIXES-NEXT: {{^}$}}
 
 bool conditional_return_statements_then_expr(int i, int j) {
   if (i == j) return (i == 0); else return false;
 }
 
-bool conditional_return_statements_else_expr(int i, int j) {
-  if (i == j) return true; else return (i == 0);
-}
-
 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 == 1) {
     return false;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return statement
 // CHECK-FIXES:      {{^}}bool conditional_compound_return_statements(int i) {{{$}}
 // CHECK-FIXES-NEXT: {{^}}  return i == 1;{{$}}
 // CHECK-FIXES-NEXT: {{^}$}}
 
+bool getFallback();
+
+bool conditional_return_complex(bool B) {
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement
+  // CHECK-FIXES: return B ||  getFallback();
+  if (B)
+    return true;
+  else
+    return getFallback();
+}
+
+bool conditional_return_complex_compound(bool B) {
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement
+  // CHECK-FIXES: return B ||  getFallback();
+  if (B) {
+    return true;
+  } else {
+    return getFallback();
+  }
+}
+
+bool conditional_return_complex_compound_negated(bool B) {
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement
+  // CHECK-FIXES: return !B &&  getFallback();
+  if (B) {
+    return false;
+  } else {
+    return getFallback();
+  }
+}
+
+bool conditional_return_complex_negated(bool B) {
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement
+  // CHECK-FIXES: return !B &&  getFallback();
+  if (B)
+    return false;
+  else
+    return getFallback();
+}
+
 bool negated_conditional_compound_return_statements(int i) {
   if (i == 1) {
     return false;
   } else {
     return true;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return statement
bool complex_if_return_return_negated(int i) {
   }
   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 simple_if_return_return_expr(int i) {
+  if (i > 10)
+    return true;
+  return i < 5;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool simple_if_return_return_expr(int i) {{{$}}
+// CHECK-FIXES: {{^}}  return i > 10 ||  i < 5;{{$}}
+// CHECK-FIXES: {{^}$}}
+
+bool simple_if_return_return_expr_negated(int i) {
+  if (i > 10)
+    return false;
+  return i > 5;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool simple_if_return_return_expr_negated(int i) {{{$}}
+// CHECK-FIXES: {{^}}  return i <= 10 &&  i > 5;{{$}}
+// 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