[clang] [clang] Diagnose for implicit boolean conversions in assignments (PR #114687)

[Philipp Rados via cfe-commits]

https://github.com/PhilippRados created https://github.com/llvm/llvm-project/pull/114687

This is an unfinished implementation of #33528. I have a couple of questions since I'm new to the codebase:

This warning should be issued for both C and C++. For C++ it is pretty straightforward since I can check this when calling `PerformImplicitConversion`. However this function isn't called in C and right now I issue the warning for C in `Sema::ImpCastExprToType`, but I don't know if this function also might be called in C++ resulting in the same warning issued twice.
Ideally I would just have to issue this once for both languages but I can't seem to find the correct `ImplicitCast` function that gets invoked for both langs.

I also found another already existing warning when running the tests (warn_condition_is_assignment) that is similar to this warning but only gets issued on assignments in conditional-expressions, as opposed to all assignment expressions. In C++ this results in the new and old warning both being displayed even though they both suggest the same thing.
How should this be handled? Should I manually check if the broader warning has already been issued and then remove it or maybe just keep the more general warning as they mean the same thing (-Wparentheses)?

Any feedback is appreciated!

>From b8244b346c7a97c2eb35ccfea23675b8df4f046e Mon Sep 17 00:00:00 2001
From: PhilippR <phil.black at gmx.net>
Date: Sat, 2 Nov 2024 18:51:26 +0100
Subject: [PATCH] [clang] adding diagnose for impl-cast (draft) This is an
 unfinished implementation of #33528

---
 clang/include/clang/Basic/DiagnosticSemaKinds.td |  2 ++
 clang/include/clang/Sema/Sema.h                  |  2 ++
 clang/lib/Sema/Sema.cpp                          | 14 ++++++++++++++
 clang/lib/Sema/SemaExprCXX.cpp                   |  3 +++
 4 files changed, 21 insertions(+)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d697e6d61afa9a..51879c359b3060 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8477,6 +8477,8 @@ def err_incomplete_object_call : Error<
 def warn_condition_is_assignment : Warning<"using the result of an "
   "assignment as a condition without parentheses">,
   InGroup<Parentheses>;
+def warn_parens_bool_assign : Warning<"suggest parentheses around assignment used as truth value">,
+    InGroup<Parentheses>;
 def warn_free_nonheap_object
   : Warning<"attempt to call %0 on non-heap %select{object %2|object: block expression|object: lambda-to-function-pointer conversion}1">,
     InGroup<FreeNonHeapObject>;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c81..30767b11998bb7 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7232,6 +7232,8 @@ class Sema final : public SemaBase {
   /// being used as a boolean condition, warn if it's an assignment.
   void DiagnoseAssignmentAsCondition(Expr *E);
 
+  void DiagnoseImplicitCastBoolAssignment(Expr *E, QualType ToType);
+
   /// Redundant parentheses over an equality comparison can indicate
   /// that the user intended an assignment used as condition.
   void DiagnoseEqualityWithExtraParens(ParenExpr *ParenE);
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 2b51765e80864a..cc9cb173c01480 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -687,6 +687,18 @@ void Sema::diagnoseZeroToNullptrConversion(CastKind Kind, const Expr *E) {
       << FixItHint::CreateReplacement(E->getSourceRange(), "nullptr");
 }
 
+void Sema::DiagnoseImplicitCastBoolAssignment(Expr* E,QualType Ty) {
+  if (Ty->isBooleanType()) {
+    if (BinaryOperator* Op = dyn_cast<BinaryOperator>(E)) {
+      // should only be issued for regular assignment `=`, not for e.g `+=`
+      if (Op->getOpcode() == BO_Assign) {
+        SourceLocation Loc = Op->getOperatorLoc();
+        Diag(Loc,diag::warn_parens_bool_assign) << E->getSourceRange();
+      }
+    }
+  }
+}
+
 /// ImpCastExprToType - If Expr is not of type 'Type', insert an implicit cast.
 /// If there is already an implicit cast, merge into the existing one.
 /// The result is of the given category.
@@ -761,6 +773,8 @@ ExprResult Sema::ImpCastExprToType(Expr *E, QualType Ty,
     }
   }
 
+  DiagnoseImplicitCastBoolAssignment(E,Ty);
+
   if (ImplicitCastExpr *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
     if (ImpCast->getCastKind() == Kind && (!BasePath || BasePath->empty())) {
       ImpCast->setType(Ty);
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 50c1b24fce6da7..14ca6a743049e4 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprConcepts.h"
 #include "clang/AST/ExprObjC.h"
@@ -4392,6 +4393,8 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType,
     FromType = From->getType();
   }
 
+  DiagnoseImplicitCastBoolAssignment(From,ToType);
+
   // If we're converting to an atomic type, first convert to the corresponding
   // non-atomic type.
   QualType ToAtomicType;