Enhance clang-tidy readability-simplify-boolean-expr check to handle chained conditional assignment and chained conditional return

[LegalizeAdulthood]

Summary

Based on feedback from applying this tool to the clang/LLVM codebase, this changeset improves the readability-simplify-boolean-expr check so that conditional assignment or return statements at the end of a chain of if/else if statements are left unchanged unless a configuration option is supplied.

[LegalizeAdulthood]

Add documentation for ChainedConditionalAssignment option.

[alexfh]

Do we actually need two separate options here? Which combination of these would you use for LLVM code?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

111

The text on the left is clang-formatted. I prefer to keep code clang-format-clean, so if you don't like the result, please file a bug against clang-format rather than changing the formatting by hand (I personally don't care in this case).

121

I don't think this function makes code shorter or easier to understand. I'd remove it and use the implementation directly (maybe with implicit conversion to bool to avoid the == 0U part).

211

How about:

Finder->addMatcher(ifStmt(isExpansionInMainFile(),
                        ChainedConditionalReturn ? anything() : unless(hasParent(ifStmt())),
                        hasThen(ReturnsBool(Value, ThenLiteralId)),
                        hasElse(ReturnsBool(!Value))).bind(Id),
                 this);

? (if this works, of course)

Same below.

clang-tidy/readability/SimplifyBooleanExprCheck.h

41

I'd deduplicate some text here:

/// 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`
/// (`ChainedConditionalAssignmentis for an assignment) specified as non-zero.

52

Please remove the empty destructor, the top base class has a virtual destructor (we need a check for this as well ;).

[LegalizeAdulthood]

In D8996#155356, @alexfh wrote:

Do we actually need two separate options here? Which combination of these would you use for LLVM code?

Since this is a readability check and "beauty is in the eye of the beholder", I didn't want to make decisions for the user. This extension of the check embraces LLVM preferences by default based on feedback from the use of the check on the clang/LLVM code. (In particular, these two issues came up in the review of changes to clang/lib/Parse and clang/lib/AST.)

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

111

This change was introduced by clang-format from the latest. I don't apply any of my own personal taste in formatting anymore, but routinely run everything through clang-format before posting for review.

121

I meant to bring this up on the mailing list. I got stuck for a while because bool b = false; Options.get(LocalName, b); totally failed when LocalName was configured to true. I tried to dig into it and figure out why, apparently there is a member function template that is enable_if'ed for integral types and because bool is an integral type, it was trying to turn true into an integer and silently failing, leaving my boolean setting unset. I'd rather have the setting just plain work for booleans because it's weird to turn this yes/no option into 1/0 in the configuration. So I was trying to have a local function that did "what I wanted" from OptionView until I could figure out how to get OptionView to properly handle boolean options.

211

I thought about that, but I didn't try it out. I can go either way.

clang-tidy/readability/SimplifyBooleanExprCheck.h

52

OK. Most code bases where I've worked had explicit preferences for virtual d'tors to remind readers that the class participates in a virtual method hierarchy.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

121

Fixed.

211

I tried this and it gets a compile error because anything() and unless(hasParent(ifStmt())) don't have compatible types.

clang-tidy/readability/SimplifyBooleanExprCheck.h

41

Fixed.

52

Fixed.

[LegalizeAdulthood]

Update based on review comments.

[alexfh]

Looks good!

Sorry for the long delay. I was on vacation. Do you need me to commit this for you?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

208

I guess I missed this in the initial code review.

Using isExpansionInMainFile() you disable fixes in headers which is sub-optimal. I think we should disable fixes in macros (and when a true or false comes from a macro) in a different way: check the relevant source location for being a file location (SourceLocation::isFileID).

This is fine for a follow up.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

208

I will address this in a subsequent refinement.

[LegalizeAdulthood]

Add to CODE_OWNERS.TXT

[LegalizeAdulthood]

If this is acceptable, please commit for me, thanks :-).

[alexfh]

CODE_OWNERS.TXT

11

I don't think we add all contributors here. Also, "code author" and "code owner" are different concepts, the latter is defined at the top of this file. I doubt that each clang-tidy check needs a dedicated "owner" as per the definition above.

I'm not going to commit this particular change.

[LegalizeAdulthood]

CODE_OWNERS.TXT

11

Either way is fine with me. I was simply attempting to offload support requests from you to me :-).

[alexfh]

Committed revision 237541.

CODE_OWNERS.TXT

 This file is a list of the people responsible for ensuring that patches for a
 particular tool are reviewed, either by themself or by someone else. They are
 also the gatekeepers for their part of Clang, with the final word on what goes
 in or not.
 
 The list is sorted by surname and formatted to allow easy grepping and
 beautification by scripts.  The fields are: name (N), email (E), web-address
 (W), PGP key ID and fingerprint (P), description (D), and snail-mail address
 (S).
 
+N: Richard Thomson
+E: [e-mail]
+D: clang-tidy readability-simplify-boolean-expr
+
 N: Edwin Vane
 E: [e-mail]
 D: clang-modernize

clang-tidy/readability/SimplifyBooleanExprCheck.h

 /// `if (b && true)`                           becomes `if (b)`
 /// `if (b && false)`                          becomes `if (false)`
 /// `if (b || true)`                           becomes `if (true)`
 /// `if (b || false)`                          becomes `if (b)`
 /// `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) 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) b = false; else b = true;`         becomes `b = !e;`
+///
+/// Parenthesis from the resulting expression `e` are removed whenever
+/// possible.
+///
+/// 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 {
 public:
-  SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context);
+
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void matchBoolBinOpExpr(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef OperatorName, StringRef BooleanId);
 
   void matchExprBinOpBool(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef OperatorName, StringRef BooleanId);
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);
+
+  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

std::string replacementExpression(const MatchFinder::MatchResult &Result,
   while (const auto *Parenthesized = dyn_cast<ParenExpr>(E)) {
     E = Parenthesized->getSubExpr();
   }
   if (Negated) {
     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();
+                " " + getText(Result, *BinOp->getRHS())).str();
       }
     }
   }
   StringRef Text = getText(Result, *E);
   return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")"
                                                       : "!" + Text)
-                  : Text)
-      .str();
+                  : Text).str();
 }
 
 } // 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))),
Finder->addMatcher(
                           hasTrueExpression(boolLiteral(equals(Value))),
                           hasFalseExpression(boolLiteral(equals(!Value))))
           .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),
+                       this);
+  } else {
     Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              unless(hasParent(ifStmt())),
                               hasThen(ReturnsBool(Value, ThenLiteralId)),
                               hasElse(ReturnsBool(!Value))).bind(Id),
                        this);
   }
+}
 
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   auto SimpleThen = binaryOperator(
       hasOperatorName("="),
       hasLHS(declRefExpr(hasDeclaration(decl().bind(IfAssignObjId)))),
       hasLHS(expr().bind(IfAssignVariableId)),
       hasRHS(boolLiteral(equals(Value)).bind(IfAssignLocId)));
   auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleThen)));
   auto SimpleElse = binaryOperator(
       hasOperatorName("="),
       hasLHS(declRefExpr(hasDeclaration(equalsBoundNode(IfAssignObjId)))),
       hasRHS(boolLiteral(equals(!Value))));
   auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
+  if (ChainedConditionalAssignment) {
     Finder->addMatcher(
         ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
         this);
+  } else {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              unless(hasParent(ifStmt())), hasThen(Then),
+                              hasElse(Else)).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);
   matchBoolBinOpExpr(Finder, false, "||", RightExpressionId);
   matchExprBinOpBool(Finder, false, "&&", RightExpressionId);
   matchExprBinOpBool(Finder, true, "||", RightExpressionId);
   matchBoolCompOpExpr(Finder, true, "==", RightExpressionId);

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

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalAssignment", value: 1}]}" --
+// REQUIRES: shell
+
+void chained_conditional_compound_assignment(int i) {
+  bool b;
+  if (i < 0) {
+    b = true;
+  } else if (i < 10) {
+    b = false;
+  } else if (i > 20) {
+    b = true;
+  } else {
+    b = false;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: redundant boolean literal in conditional assignment [readability-simplify-boolean-expr]
+  // CHECK-FIXES:      {{^}}  } else if (i < 10) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    b = false;{{$}}
+  // CHECK-FIXES-NEXT: {{^}}  } else b = i > 20;{{$}}
+}
+
+void chained_conditional_assignment(int i) {
+  bool b;
+  if (i < 0)
+    b = true;
+  else if (i < 10)
+    b = false;
+  else if (i > 20)
+    b = true;
+  else
+    b = false;
+  // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: {{.*}} in conditional assignment
+  // CHECK-FIXES:      {{^}}  else if (i < 10)
+  // CHECK-FIXES-NEXT: {{^}}    b = false;
+  // CHECK-FIXES-NEXT: {{^}}  else b = i > 20;
+}

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

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalReturn", value: 1}]}" --
+// REQUIRES: shell
+
+bool chained_conditional_compound_return(int i) {
+  if (i < 0) {
+    return true;
+  } else if (i < 10) {
+    return false;
+  } else if (i > 20) {
+    return true;
+  } else {
+    return false;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]
+  // CHECK-FIXES:      {{^}}  } else if (i < 10) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    return false;{{$}}
+  // CHECK-FIXES-NEXT: {{^}}  } else return i > 20;{{$}}
+}
+
+bool chained_conditional_return(int i) {
+  if (i < 0)
+    return true;
+  else if (i < 10)
+    return false;
+  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;
+}

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

void complex_conditional_assignment_statements(int i) {
   // unchanged; multiple statements
   bool h;
   if (j > 10) {
     j = 10;
     f = true;
   } else
     f = false;
 }
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_conditional_compound_return(int i) {
+  if (i < 0) {
+    return true;
+  } else if (i < 10) {
+    return false;
+  } else if (i > 20) {
+    return true;
+  } else {
+    return false;
+  }
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_conditional_return(int i) {
+  if (i < 0)
+    return true;
+  else if (i < 10)
+    return false;
+  else if (i > 20)
+    return true;
+  else
+    return false;
+}
+
+// unchanged: chained assignments, but ChainedConditionalAssignment not set
+void chained_conditional_compound_assignment(int i) {
+  bool b;
+  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
+void chained_conditional_assignment(int i) {
+  bool b;
+  if (i < 0)
+    b = true;
+  else if (i < 10)
+    b = false;
+  else if (i > 20)
+    b = true;
+  else
+    b = false;
+}