[clang] [clang] Added warn-assignment-bool-context (PR #115234)
Philipp Rados via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 6 15:24:22 PST 2024
https://github.com/PhilippRados created https://github.com/llvm/llvm-project/pull/115234
This is a fix for #33528 as I messed up my other PR with unsynced changes.
A couple of things make this less straightforward as initially thought, which is why I would like some feedback as to how these things should be handled.
This warning should get issued for both C and C++ (I'm not sure about the other languages). For C the tests pass. For C++ some fail because another similar warning is expected `warn_condition_is_assignment`. That warning can be covered by this new warning but it would be better to issue the existing since it is more specific in cases of condition.
The problem is that in C, conditions do not emit an implicit-cast to booleans while in C++ they do. So the original warning is needed for C, but would result in double warning in C++. This requires special handling.
In the code I have left `NOTE:` comments whenever I was unsure about the implementation of something, which might need some clearing up since this is my first proper contribution.
>From e819360e6510f12f19b7df9d4eb22c0943776440 Mon Sep 17 00:00:00 2001
From: PhilippR <phil.black at gmx.net>
Date: Thu, 7 Nov 2024 00:06:24 +0100
Subject: [PATCH] [clang] Added warn-assignment-bool-context
This warning is issued if an assignment is used in a context
where a boolean result is required.
---
.../clang/Basic/DiagnosticSemaKinds.td | 3 ++
clang/include/clang/Sema/Sema.h | 2 +
clang/lib/Sema/Sema.cpp | 53 +++++++++++++++++++
clang/lib/Sema/SemaExpr.cpp | 6 ++-
.../test/Sema/warn-assignment-bool-context.c | 35 ++++++++++++
.../SemaCXX/warn-assignment-bool-context.cpp | 45 ++++++++++++++++
6 files changed, 143 insertions(+), 1 deletion(-)
create mode 100644 clang/test/Sema/warn-assignment-bool-context.c
create mode 100644 clang/test/SemaCXX/warn-assignment-bool-context.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index ae3e243bdc58bd..4ba4327bde5cfb 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8477,6 +8477,9 @@ 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_assignment_bool_context : 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..6bf73956d3f944 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 DiagnoseAssignmentBoolContext(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..a8e4657ac0b6ec 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -687,6 +687,48 @@ void Sema::diagnoseZeroToNullptrConversion(CastKind Kind, const Expr *E) {
<< FixItHint::CreateReplacement(E->getSourceRange(), "nullptr");
}
+void Sema::DiagnoseAssignmentBoolContext(Expr *E, QualType Ty) {
+ // Use copy to not alter original expression.
+ Expr *ECopy = E;
+
+ if (Ty->isBooleanType()) {
+ // `bool(x=0)` and if (x=0){} emit:
+ // - ImplicitCastExpr bool IntegralToBoolean
+ // -- ImplicitCastExpr int LValueToRValue
+ // --- Assignment ...
+ // But should still emit this warning (at least gcc does), even if bool-cast
+ // is not directly followed by assignment.
+ // NOTE: Is this robust enough or can there be other semantic expression
+ // until the assignment?
+ while (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(ECopy)) {
+ // If there is another implicit cast to bool then this warning would have
+ // been already emitted.
+ if (ICE->getType()->isBooleanType())
+ return;
+ ECopy = ICE->getSubExpr();
+ }
+
+ if (BinaryOperator *Op = dyn_cast<BinaryOperator>(ECopy)) {
+ // Should only be issued for regular assignment `=`,
+ // not for compound-assign like `+=`.
+ // NOTE: Might make sense to emit for all assignments even if gcc
+ // only does for regular assignment.
+ if (Op->getOpcode() == BO_Assign) {
+ SourceLocation Loc = Op->getOperatorLoc();
+ Diag(Loc, diag::warn_assignment_bool_context)
+ << ECopy->getSourceRange();
+
+ SourceLocation Open = ECopy->getBeginLoc();
+ SourceLocation Close =
+ getLocForEndOfToken(ECopy->getSourceRange().getEnd());
+ Diag(Loc, diag::note_condition_assign_silence)
+ << FixItHint::CreateInsertion(Open, "(")
+ << FixItHint::CreateInsertion(Close, ")");
+ }
+ }
+ }
+}
+
/// 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 +803,17 @@ ExprResult Sema::ImpCastExprToType(Expr *E, QualType Ty,
}
}
+ // FIXME: Doesn't include C89, so this warning isn't emitted when passing
+ // `std=c89`.
+ auto isC = getLangOpts().C99 || getLangOpts().C11 || getLangOpts().C17 ||
+ getLangOpts().C23;
+ // Do not emit this warning for Objective-C, since it's a common idiom.
+ // NOTE: Are there other languages that this could affect besides C and C++?
+ // Ideally would check `getLangOpts().Cplusplus || getLangOpts().C` but there
+ // is no option for C (only C99 etc.).
+ if ((getLangOpts().CPlusPlus || isC) && !getLangOpts().ObjC)
+ DiagnoseAssignmentBoolContext(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/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 49fdb5b5ab43da..bee8751c3fa7ac 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -20287,7 +20287,11 @@ void Sema::DiagnoseEqualityWithExtraParens(ParenExpr *ParenE) {
ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
bool IsConstexpr) {
- DiagnoseAssignmentAsCondition(E);
+ // This warning is already covered by `warn_assignment_bool_context` in C++.
+ // NOTE: Ideally both warnings would be combined
+ if (!getLangOpts().CPlusPlus || getLangOpts().ObjC)
+ DiagnoseAssignmentAsCondition(E);
+
if (ParenExpr *parenE = dyn_cast<ParenExpr>(E))
DiagnoseEqualityWithExtraParens(parenE);
diff --git a/clang/test/Sema/warn-assignment-bool-context.c b/clang/test/Sema/warn-assignment-bool-context.c
new file mode 100644
index 00000000000000..bf746e0bed2c22
--- /dev/null
+++ b/clang/test/Sema/warn-assignment-bool-context.c
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -Wparentheses -verify %s
+
+// NOTE: Don't know if tests allow includes.
+#include <stdbool.h>
+
+// Do not emit the warning for compound-assignments.
+bool f(int x) { return x = 0; } // expected-warning {{suggest parentheses around assignment used as truth value}}\
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+bool f2(int x) { return x += 0; }
+
+bool f3(bool x) { return x = 0; }
+
+void test() {
+ int x;
+
+ // This should emit the `warn_condition_is_assignment` warning, since
+ // C doesn't do implicit conversion booleans for conditions.
+ if (x = 0) {} // expected-warning {{using the result of an assignment as a condition without parentheses}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}\
+ // expected-note{{use '==' to turn this assignment into an equality comparison}}
+ if (x = 4 && x){} // expected-warning {{using the result of an assignment as a condition without parentheses}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}\
+ // expected-note{{use '==' to turn this assignment into an equality comparison}}
+
+ (void)(bool)(x = 1);
+ (void)(bool)(int)(x = 1);
+
+
+ bool _a = x = 3; // expected-warning {{suggest parentheses around assignment used as truth value}}\
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+
+ // Shouldn't warn for above cases if parentheses were provided.
+ if ((x = 0)) {}
+ bool _b = (x = 3);
+}
diff --git a/clang/test/SemaCXX/warn-assignment-bool-context.cpp b/clang/test/SemaCXX/warn-assignment-bool-context.cpp
new file mode 100644
index 00000000000000..a4831f53e311b1
--- /dev/null
+++ b/clang/test/SemaCXX/warn-assignment-bool-context.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wparentheses -verify %s
+
+// Do not emit the warning for compound-assignments.
+bool f(int x) { return x = 0; } // expected-warning {{suggest parentheses around assignment used as truth value}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+bool f2(int x) { return x += 0; }
+
+bool f3(bool x) { return x = 0; }
+
+void test() {
+ int x;
+
+ // Assignemnts inside of conditions should still emit the more specific `warn_condition_is_assignment` warning.
+ if (x = 0) {} // expected-warning {{using the result of an assignment as a condition without parentheses}} \
+ // expected-note{{use '==' to turn this assignment into an equality comparison}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+ if (x = 4 && x){} // expected-warning {{using the result of an assignment as a condition without parentheses}} \
+ // expected-note{{use '==' to turn this assignment into an equality comparison}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+
+
+ (void)bool(x = 1); // expected-warning {{suggest parentheses around assignment used as truth value}}\
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+ (void)(bool)(x = 1);
+
+ // This should still emit since the RHS is casted to `int` before being casted back to `bool`.
+ (void)bool(x = false); // expected-warning {{suggest parentheses around assignment used as truth value}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+
+ // Should only issue warning once, even if multiple implicit casts.
+ // FIXME: This only checks that warning occurs not how often.
+ (void)bool(bool(x = 1)); // expected-warning {{suggest parentheses around assignment used as truth value}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+ (void)bool(int(bool(x = 1))); // expected-warning {{suggest parentheses around assignment used as truth value}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+ (void)bool(int(x = 1));
+
+ bool _a = x = 3; // expected-warning {{suggest parentheses around assignment used as truth value}} \
+ // expected-note{{place parentheses around the assignment to silence this warning}}
+
+ // Shouldn't warn for above cases if parentheses were provided.
+ if ((x = 0)) {}
+ (void)bool((x = 1));
+ bool _b= (x = 3);
+}
More information about the cfe-commits
mailing list