-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[Sema] Diagnose use of if/else-if condition variable inside else-if/else branch(s) #156436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2371,6 +2371,24 @@ NonOdrUseReason Sema::getNonOdrUseReasonInCurrentContext(ValueDecl *D) { | |
return NOUR_None; | ||
} | ||
|
||
bool Sema::isConditionVarReference(const DeclRefExpr *DRE) { | ||
if (!DRE) | ||
return false; | ||
|
||
const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()); | ||
|
||
if (!VD) | ||
return false; | ||
|
||
for (Scope *S = getCurScope(); S; S = S->getParent()) { | ||
if (VarDecl *CV = S->getConditionVar()) | ||
if (VD == CV) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should still use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the 5th testcase, I could see that it is returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you give a better example? They should be different entities... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is throwing here since it compares the local There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm... those shouldn't be the same entity, but I see the ASTContext function is doing a lot of assuming for the purposes of deserialization, so that is probably why. It just doesn't do what I think it should be doing. I think we actually want |
||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
DeclRefExpr * | ||
Sema::BuildDeclRefExpr(ValueDecl *D, QualType Ty, ExprValueKind VK, | ||
const DeclarationNameInfo &NameInfo, | ||
|
@@ -2425,6 +2443,10 @@ Sema::BuildDeclRefExpr(ValueDecl *D, QualType Ty, ExprValueKind VK, | |
if (const auto *BE = BD->getBinding()) | ||
E->setObjectKind(BE->getObjectKind()); | ||
|
||
if (isConditionVarReference(E)) | ||
Diag(E->getBeginLoc(), diag::warn_out_of_scope_var_usage) | ||
<< D->getDeclName(); | ||
|
||
return E; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_1 %s | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests aren't particularly great or varied, try your situations but also try to make them 'flow' a bit to show you've covered everything. Make sure you cover more than just pointer cases. Also, try some tests in a dependent case, where the variable itself is dependent. also-also: What about an unevaluated context? Or a situation where you don't actually take the variable? Should THAT warn? Consider:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If anyone can help with different types of testcases, I would appreciate that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gave you some ideas, dependence, non-evaluated contexts, etc. |
||
// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_2 %s | ||
// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_3 %s | ||
// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_4 %s | ||
// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_5 %s | ||
// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_6 %s | ||
|
||
int *get_something(); | ||
int *get_something_else(); | ||
int *get_something_else_again(); | ||
int *get_something_else_again_now(); | ||
|
||
#ifdef TEST_1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't use macros like this in the test, just do all of these in the same 'run' line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean to use one single function and all of the testcases together? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily. But you can capture multiple diagnostics in the same run, so don't do macros, just do multiple checks. |
||
|
||
int test() { | ||
if (int *ptr = get_something()) { | ||
return ptr[0] * ptr[0]; | ||
} | ||
// expected-warning@+2{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
else if (int *ptr2 = get_something_else()) { | ||
return ptr[0] * ptr2[0]; | ||
} | ||
// expected-warning@+3{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
// expected-warning@+2{{variable 'ptr2' declared in 'if' block is always false or null here}} | ||
else if (int* ptr3 = get_something_else_again()) { | ||
return ptr[0] * ptr2[0] * ptr3[0]; | ||
} | ||
// expected-warning@+4{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
// expected-warning@+3{{variable 'ptr2' declared in 'if' block is always false or null here}} | ||
// expected-warning@+2{{variable 'ptr3' declared in 'if' block is always false or null here}} | ||
else if (int *ptr4 = get_something_else_again_now()) { | ||
return ptr[0] * ptr2[0] * ptr3[0] * ptr4[0]; | ||
} | ||
else { | ||
return -1; | ||
} | ||
} | ||
|
||
#endif | ||
|
||
#ifdef TEST_2 | ||
|
||
int test() { | ||
if (int *ptr = get_something()) { | ||
return ptr[0] * ptr[0]; | ||
} | ||
else if (int *ptr2 = get_something_else()) { | ||
return ptr2[0] * ptr2[0]; | ||
} | ||
// expected-warning@+3{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
// expected-warning@+2{{variable 'ptr2' declared in 'if' block is always false or null here}} | ||
else if (int* ptr3 = get_something_else_again()) { | ||
return ptr[0] * ptr2[0] * ptr3[0]; | ||
} | ||
else { | ||
return -1; | ||
} | ||
} | ||
|
||
#endif | ||
|
||
#ifdef TEST_3 | ||
|
||
int test() { | ||
if (int *ptr = get_something()) { | ||
return ptr[0] * ptr[0]; | ||
} | ||
else if (int *ptr2 = get_something_else()) { | ||
return ptr2[0] * ptr2[0]; | ||
} | ||
// expected-warning@+3{{variable 'ptr2' declared in 'if' block is always false or null here}} | ||
// expected-warning@+2{{variable 'ptr2' declared in 'if' block is always false or null here}} | ||
else if (int* ptr3 = get_something_else_again()) { | ||
return ptr2[0] * ptr2[0] * ptr3[0]; | ||
} | ||
else { | ||
return -1; | ||
} | ||
} | ||
|
||
#endif | ||
|
||
#ifdef TEST_4 | ||
|
||
int test() { | ||
int x = 10; | ||
|
||
if (x == 30) { | ||
return x; | ||
} | ||
else if (int *ptr = get_something()) { | ||
return ptr[0]; | ||
} | ||
// expected-warning@+3{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
// expected-warning@+2{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
else if (x == 20) { | ||
return ptr[0] * ptr[0]; | ||
} | ||
// expected-warning@+2{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
else if (int* ptr2 = get_something_else()) { | ||
return ptr[0] * ptr2[0]; | ||
} | ||
// expected-warning@+3{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
// expected-warning@+2{{variable 'ptr2' declared in 'if' block is always false or null here}} | ||
else if (int *ptr3 = get_something_else_again()) { | ||
return ptr[0] * ptr2[0] * ptr3[0]; | ||
} | ||
else { | ||
return -1; | ||
} | ||
} | ||
|
||
#endif | ||
|
||
#ifdef TEST_5 | ||
|
||
void test() { | ||
// expected-no-diagnostics | ||
if (bool x = get_something()) {} | ||
else { | ||
{ | ||
bool x = get_something_else(); | ||
if (x) {} | ||
} | ||
} | ||
} | ||
|
||
#endif | ||
|
||
#ifdef TEST_6 | ||
|
||
int test() { | ||
if (int *ptr = get_something()) { | ||
return ptr[0]; | ||
} | ||
else if (int *ptr = get_something_else()) { | ||
return ptr[0] * ptr[0]; | ||
} | ||
// expected-warning@+3{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
// expected-warning@+2{{variable 'ptr' declared in 'if' block is always false or null here}} | ||
else { | ||
return ptr[0] * ptr[0]; | ||
} | ||
} | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a fan of this wording. Perhaps we could use a
%select
here to decide whether this is afalse
ornull
(or perhaps even0
?). Or perhaps we should just sayfalse
always? What do others think?Also, a note pointing to the condition-var is probably a good idea too.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try that.
Its pointing to the "used" condition variable though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, hmm... it should point to the
DeclRefExpr
not to the condition variable then. We want to say "this use right here is wrong".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erichkeane Correct me if I am wrong.
In here, this is the current scenario
It should warn for something else too?