[clang-tidy] Fix missing parentheses in readability-implicit-bool-conversion fixes (#74891)

Check now more properly add missing parentheses to code like this: 'bool
bar = true ? 1 : 0 != 0;'.

Closes #71867

Author: PiotrZSL

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

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "ImplicitBoolConversionCheck.h"
+#include "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
@@ -61,31 +62,6 @@ bool isUnaryLogicalNotOperator(const Stmt *Statement) {
   return UnaryOperatorExpr && UnaryOperatorExpr->getOpcode() == UO_LNot;
 }
 
-bool areParensNeededForOverloadedOperator(OverloadedOperatorKind OperatorKind) {
-  switch (OperatorKind) {
-  case OO_New:
-  case OO_Delete: // Fall-through on purpose.
-  case OO_Array_New:
-  case OO_Array_Delete:
-  case OO_ArrowStar:
-  case OO_Arrow:
-  case OO_Call:
-  case OO_Subscript:
-    return false;
-
-  default:
-    return true;
-  }
-}
-
-bool areParensNeededForStatement(const Stmt *Statement) {
-  if (const auto *OperatorCall = dyn_cast<CXXOperatorCallExpr>(Statement)) {
-    return areParensNeededForOverloadedOperator(OperatorCall->getOperator());
-  }
-
-  return isa<BinaryOperator>(Statement) || isa<UnaryOperator>(Statement);
-}
-
 void fixGenericExprCastToBool(DiagnosticBuilder &Diag,
                               const ImplicitCastExpr *Cast, const Stmt *Parent,
                               ASTContext &Context) {
@@ -105,9 +81,10 @@ void fixGenericExprCastToBool(DiagnosticBuilder &Diag,
 
   const Expr *SubExpr = Cast->getSubExpr();
 
-  bool NeedInnerParens = areParensNeededForStatement(SubExpr);
+  bool NeedInnerParens =
+      SubExpr != nullptr && utils::fixit::areParensNeededForStatement(*SubExpr);
   bool NeedOuterParens =
-      Parent != nullptr && areParensNeededForStatement(Parent);
+      Parent != nullptr && utils::fixit::areParensNeededForStatement(*Parent);
 
   std::string StartLocInsertion;
 
@@ -364,7 +341,7 @@ void ImplicitBoolConversionCheck::handleCastToBool(const ImplicitCastExpr *Cast,
     return;
   }
 
-  auto Diag = diag(Cast->getBeginLoc(), "implicit conversion %0 -> bool")
+  auto Diag = diag(Cast->getBeginLoc(), "implicit conversion %0 -> 'bool'")
               << Cast->getSubExpr()->getType();
 
   StringRef EquivalentLiteral =
@@ -381,7 +358,7 @@ void ImplicitBoolConversionCheck::handleCastFromBool(
     ASTContext &Context) {
   QualType DestType =
       NextImplicitCast ? NextImplicitCast->getType() : Cast->getType();
-  auto Diag = diag(Cast->getBeginLoc(), "implicit conversion bool -> %0")
+  auto Diag = diag(Cast->getBeginLoc(), "implicit conversion 'bool' -> %0")
               << DestType;
 
   if (const auto *BoolLiteral =

clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp

@@ -224,6 +224,40 @@ std::optional<FixItHint> addQualifierToVarDecl(const VarDecl &Var,
   return std::nullopt;
 }
 
+bool areParensNeededForStatement(const Stmt &Node) {
+  if (isa<ParenExpr>(&Node))
+    return false;
+
+  if (isa<clang::BinaryOperator>(&Node) || isa<UnaryOperator>(&Node))
+    return true;
+
+  if (isa<clang::ConditionalOperator>(&Node) ||
+      isa<BinaryConditionalOperator>(&Node))
+    return true;
+
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(&Node)) {
+    switch (Op->getOperator()) {
+    case OO_PlusPlus:
+      [[fallthrough]];
+    case OO_MinusMinus:
+      return Op->getNumArgs() != 2;
+    case OO_Call:
+      [[fallthrough]];
+    case OO_Subscript:
+      [[fallthrough]];
+    case OO_Arrow:
+      return false;
+    default:
+      return true;
+    };
+  }
+
+  if (isa<CStyleCastExpr>(&Node))
+    return true;
+
+  return false;
+}
+
 // Return true if expr needs to be put in parens when it is an argument of a
 // prefix unary operator, e.g. when it is a binary or ternary operator
 // syntactically.

clang-tools-extra/clang-tidy/utils/FixItHintUtils.h

@@ -47,6 +47,10 @@ addQualifierToVarDecl(const VarDecl &Var, const ASTContext &Context,
 
 // \brief Format a pointer to an expression
 std::string formatDereference(const Expr &ExprNode, const ASTContext &Context);
+
+// \brief Checks whatever a expression require extra () to be always used in
+// safe way in any other expression.
+bool areParensNeededForStatement(const Stmt &Node);
 } // namespace clang::tidy::utils::fixit
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_FIXITHINTUTILS_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -502,8 +502,8 @@ Changes in existing checks
   <clang-tidy/checks/readability/implicit-bool-conversion>` check to take
   do-while loops into account for the `AllowIntegerConditions` and
   `AllowPointerConditions` options. It also now provides more consistent
-  suggestions when parentheses are added to the return value. It also ignores
-  false-positives for comparison containing bool bitfield.
+  suggestions when parentheses are added to the return value or expressions. 
+  It also ignores false-positives for comparison containing bool bitfield.
 
 - Improved :doc:`readability-misleading-indentation
   <clang-tidy/checks/readability/misleading-indentation>` check to ignore

clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-allow-in-conditions.cpp

@@ -19,7 +19,7 @@ struct Struct {
 void regularImplicitConversionIntegerToBoolIsNotIgnored() {
   int integer = 0;
   functionTaking<bool>(integer);
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int' -> 'bool' [readability-implicit-bool-conversion]
   // CHECK-FIXES: functionTaking<bool>(integer != 0);
 }
 
@@ -54,12 +54,12 @@ void implicitConversionIntegerToBoolInConditionalsIsAllowed() {
 void regularImplicitConversionPointerToBoolIsNotIgnored() {
   int* pointer = nullptr;
   functionTaking<bool>(pointer);
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int *' -> bool
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int *' -> 'bool'
   // CHECK-FIXES: functionTaking<bool>(pointer != nullptr);
 
   int Struct::* memberPointer = &Struct::member;
   functionTaking<bool>(memberPointer);
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int Struct::*' -> bool
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int Struct::*' -> 'bool'
   // CHECK-FIXES: functionTaking<bool>(memberPointer != nullptr);
 }
 

clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-cxx98.cpp

@@ -15,31 +15,31 @@ struct Struct {
 void useOldNullMacroInReplacements() {
   int* pointer = NULL;
   functionTaking<bool>(pointer);
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int *' -> bool [readability-implicit-bool-conversion]
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int *' -> 'bool' [readability-implicit-bool-conversion]
   // CHECK-FIXES: functionTaking<bool>(pointer != 0);
 
   int Struct::* memberPointer = NULL;
   functionTaking<bool>(!memberPointer);
-  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: implicit conversion 'int Struct::*' -> bool
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: implicit conversion 'int Struct::*' -> 'bool'
   // CHECK-FIXES: functionTaking<bool>(memberPointer == 0);
 }
 
 void fixFalseLiteralConvertingToNullPointer() {
   functionTaking<int*>(false);
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion bool -> 'int *'
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'bool' -> 'int *'
   // CHECK-FIXES: functionTaking<int*>(0);
 
   int* pointer = NULL;
   if (pointer == false) {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: implicit conversion bool -> 'int *'
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: implicit conversion 'bool' -> 'int *'
   // CHECK-FIXES: if (pointer == 0) {}
 
   functionTaking<int Struct::*>(false);
-  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit conversion bool -> 'int Struct::*'
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit conversion 'bool' -> 'int Struct::*'
   // CHECK-FIXES: functionTaking<int Struct::*>(0);
 
   int Struct::* memberPointer = NULL;
   if (memberPointer != false) {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion bool -> 'int Struct::*'
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'bool' -> 'int Struct::*'
   // CHECK-FIXES: if (memberPointer != 0) {}
 }