[clang] Added warn-assignment-bool-context #115234
Conversation
This warning is issued if an assignment is used in a context where a boolean result is required.
|
@Endilll Do you have some advice regarding this implementation? Or can you otherwise maybe forward this to someone who can take a look. |
|
I'm sorry it took us a while. |
|
No problem, I know everybody is busy.
Do you just mean to mark it "read for review" or to apply changes so that it can be merged? Because the current implementation is not ready as I still have some questions as to how to implement things in order to pass the failing tests. |
I mean "ready for review". The draft status of your PR actively discourages anyone to look at it, while you need someone to look and answer your questions. It's not going to be merged unless you and reviewers are happy. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Philipp Rados (PhilippRados) ChangesThis 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 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 Full diff: https://github.com/llvm/llvm-project/pull/115234.diff 6 Files Affected:
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);
+}
|
|
Yup ok. I thought that was what draft was for to check on not-finished implementations but I just changed it back for review. |
clang/lib/Sema/SemaExpr.cpp
Outdated
| 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) |
There was a problem hiding this comment.
Is this right? Won't this cause double-diagnostic in ObjC++?
There was a problem hiding this comment.
Also, rather than this, I'd prefer we are smarter about the new diagnostic to not overlap in the condition case (suppressing THAT instead of htis here).
There was a problem hiding this comment.
Also, rather than this, I'd prefer we are smarter about the new diagnostic to not overlap in the condition case (suppressing THAT instead of htis here).
Yes, I agree. Is there a way to check if a certain other diagnostic has been issued? Or do I need to pass a flag like isCond to Sema::ImpCastExprToType to be able to check if the outer expression is a condition? The latter would require a lot of function changes which would be a little tedious which is why I'm asking beforehand.
There was a problem hiding this comment.
There isn't unfortunately. We'd have to find some way to intuit whether this is a condition or not, and I don't have a good feel for how to do that. Perhaps @cor3ntin has an idea?
There was a problem hiding this comment.
Is it not possible to store that info in the expr and then just check it later on? Or in the context that gets passed around?
| // RUN: %clang_cc1 -x c -fsyntax-only -Wparentheses -verify %s | ||
|
|
||
| // NOTE: Don't know if tests allow includes. | ||
| #include <stdbool.h> |
There was a problem hiding this comment.
Include shouldn't be here, you can just define the thing manually if you'd like.
| void test() { | ||
| int x; | ||
|
|
||
| // Assignemnts inside of conditions should still emit the more specific `warn_condition_is_assignment` warning. |
There was a problem hiding this comment.
| // Assignemnts inside of conditions should still emit the more specific `warn_condition_is_assignment` warning. | |
| // Assignments inside of conditions should still emit the more specific `warn_condition_is_assignment` warning. |
| 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">, |
There was a problem hiding this comment.
This diagnostic wording doesn't really make sense, it needs to tell the person what is wrong.
There was a problem hiding this comment.
I realize this is the C diagnostic text, but I'm still not a huge fan. The text in warn_condition_is_idiomatic_assignment is pretty close to what we want, right? Perhaps we should use that? Also, emitting note_condition_assign_silence might be sensible.
There was a problem hiding this comment.
The text in warn_condition_is_idiomatic_assignment is pretty close to what we want, right?
Yes, but maybe just having the more generic warn_assignment_bool_context with a message like "using the result of an assignment as a truth value without parentheses" which can be emitted during conditions as well. Since this implicit cast can also happen outside of conditions.
There was a problem hiding this comment.
Also, emitting note_condition_assign_silence might be sensible.
Yes that is already being submitted in DiagnoseAssignmentBoolContext()
clang/lib/Sema/Sema.cpp
Outdated
|
|
||
| // FIXME: Doesn't include C89, so this warning isn't emitted when passing | ||
| // `std=c89`. | ||
| auto isC = getLangOpts().C99 || getLangOpts().C11 || getLangOpts().C17 || |
There was a problem hiding this comment.
Usually for "C" we can just test "not C++". That said, why does this diagnostic not make sense in C++?
There was a problem hiding this comment.
It does make sense for C++. It should be issued for both C and C++ (I'm not proficient in OpenMP/Cuda so I can't really talk for those).
I wanted to clarify that with the if (getLangOpts().CPlusPlus || isC) && !getLangOpts().ObjC) {...} line, that this would actually just affect C and C++ and none of the other languages.
Of course I could also just do !isCuda && !isOpenMP && !isObjC so that just C and C++ remain but that seems a little verbose.
There was a problem hiding this comment.
Why doesn't this make sense in ObjC? it seems like it should. And we dont' want to suppress this in CUDA or OMP mode either, that is still valid C/C++ with extensions.
There was a problem hiding this comment.
Why doesn't this make sense in ObjC?
I thought that this was some kind of common pattern in ObjC which is why the existing DiagnoseAssignmentAsCondition() also special cases this. I think I might have also seen some tests that check this, but I could be wrong.
Edit: I checked the tests that mentioned this as an idiom and it only applied inside of conditions which is already handled so this should be fine to emit in ObjC too. This makes everything way simpler as the diagnostic can issued for every lang.
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Still hasn't changed based on my previous feedback, but a few more things as I was scrolling through.
| 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">, |
There was a problem hiding this comment.
I realize this is the C diagnostic text, but I'm still not a huge fan. The text in warn_condition_is_idiomatic_assignment is pretty close to what we want, right? Perhaps we should use that? Also, emitting note_condition_assign_silence might be sensible.
clang/lib/Sema/Sema.cpp
Outdated
| } | ||
|
|
||
| void Sema::DiagnoseAssignmentBoolContext(Expr *E, QualType Ty) { | ||
| // Use copy to not alter original expression. |
There was a problem hiding this comment.
This is unnecessary. You don't use the original value of E anywhere else below, so there isn't a good reason to not just use the parameter.
| // - ImplicitCastExpr bool IntegralToBoolean | ||
| // -- ImplicitCastExpr int LValueToRValue | ||
| // --- Assignment ... | ||
| // But should still emit this warning (at least gcc does), even if bool-cast |
There was a problem hiding this comment.
Not sure what the question is here, can you clarify?
ALSO ALSO, if this ends up in C++ anywhere, you'll have some user-defined operators to mess with as well.
There was a problem hiding this comment.
Not sure what the question is here, can you clarify?
Basically just asking if ImplicitCastExpr is the only valid expression that can be in between the outer cast and the assignment expression or if there is another implicit expression that would need to be matched here.
There was a problem hiding this comment.
ALSO ALSO, if this ends up in C++ anywhere, you'll have some user-defined operators to mess with as well.
How can I check for that? Do you have a more robust approach?
There was a problem hiding this comment.
Basically just asking if ImplicitCastExpr is the only valid expression that can be in between the outer cast and the assignment expression or if there is another implicit expression that would need to be matched here.
None I can think of? We might end up having a case where we don't properly warn if we miss anything here, which is perhaps acceptable.
How can I check for that? Do you have a more robust approach?
Having this happen on the implicit cast makes this a little awkward since we can't record 'what is going on here' as we go. That said, I think punting /not warning in the case of user-defined-operators is probably OK, that will prevent us from diagnosing in the cases of some DSLs or something, which is the behavior we probably want?
llvmbot
added
clang:modules
|
I applied the mentioned changes from above and merged the two diagnostics into a single one. I also added the field IsInsideCondition to the I also saw that enum/boolean fields in |
4 participants
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.