[Sema] Diagnose use of if/else-if condition variable inside else-if/else branch(s) #156436
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
tbaederr
requested review from
AaronBallman,
Sirraide,
erichkeane and
shafik
There was a problem hiding this comment.
So from a high-level point of view, I definitely think there is value in having this because I personally have made this mistake so many times, and honestly, I think we should just straight-up error on it (or at least make it a warning that defaults to an error) because I cannot think of a situation where doing this would ever be valid.
That said, using an AST visitor for this is... probably not great because we now do an AST traversal on every else branch ever, which sounds like it might hurt performance quite a bit—that said, this is pretty much my only concern w/ this patch, so if it turns out that this isn’t the case at all then this approach is fine I suppose.
CC @nikic could you put this on llvm-compile-time-tracker so we know what the overhead of this is if any?
At the same time, I’m not sure I can think of a better way to do this. I suppose another option might be to add a flag to VarDecl that keeps track of whether it’s declared in the condition of an if statement and then somehow have scope information to figure out where a given use of it is relative to that if statement, but that sounds like it’d get a bit complicated, and I’m not sure running this sort of check on every DRE would be any better...
(Side note: sometimes when there are performance considerations we prefer to move these sorts of diagnostics into the static analyser, but I think this is situation is bad enough to where we want to do this in the compiler proper—at least I can think of many instances where this would have helped me quite a bit.)
There was a problem hiding this comment.
| : Warning<"variable %0 used in else/else if block is out of scope">, | |
| : Warning<"variable %0 used in 'else'/'else if' block is out of scope">, |
There was a problem hiding this comment.
The variable is in scope per the C++ spec, so that's technically untrue and/or misleading.
I'd rather go for something like 'variable %s declared in if block is {null|zero|falsey|etc} in this branch'.
There was a problem hiding this comment.
@Alcaro As you said 'variable %s declared in if block is {null|zero|falsey|etc} in this branch'. Should we write if/if-else instead of if, because the variable can be declared in if-else block too right?
There was a problem hiding this comment.
I'd vote no. The variable is declared in the if part of the if-statement, not in the else.
If you mean if/else-if, then I'd still vote no - else-if is not a thing in the C++ spec, it's just an if-statement whose statement-false is another if-statement (and the variable is still declared inside the if part). No need to be pedantic at the expense of conciseness.
clang/lib/Sema/SemaStmt.cpp
Outdated
There was a problem hiding this comment.
If you’re going to do it this way, please use DynamicRecursiveASTVisitor instead.
There was a problem hiding this comment.
@Sirraide RecursiveASTVisitor uses CRTP method which is already optimized if I am not wrong. So, don't you think that DynamicRecursiveASTVisitor will slow down the compilation process?
There was a problem hiding this comment.
The CRTP-based version has a horrible binary size (and compile-time) overhead and we’ve actively tried to stop using it as much as possible; see #110040 for more context.
There was a problem hiding this comment.
I should do another pass and migrate any visitors that may have been introduced in the meantime or are still left unmigrated
clang/lib/Sema/SemaStmt.cpp
Outdated
There was a problem hiding this comment.
| VarDecl *ConditionVar = nullptr; | |
| if (auto *CondVar = Cond.get().first) { | |
| ConditionVar = dyn_cast<VarDecl>(CondVar); | |
| } | |
| VarDecl *ConditionVar = dyn_cast_if_present<VarDecl>(Cond.get().first); |
|
Oh yeah, two more things I was going to point out: we should definiltely have some tests in unevaluated contexts (e.g. |
There was a problem hiding this comment.
I also have done this a ton, but it is something we need to leave legal, as it IS a pattern folks actually use intentionally. So making this an error isn't something we can really do.
Doing this as a visitor is, IMO, not the right solution here. We can actually do this via https://clang.llvm.org/doxygen/classclang_1_1Scope.html I think instead? I wonder if anyone else has better ideas, but a visitor on every variable reference inside of an else is not acceptable.
Right, I think I wasn’t quite awake yet earlier because for some reason I was only thinking about the very specific use case where the After thinking about this some more, I wonder if it would make sense to restrict this warning to situations where we both
Not doing 1. would cause us to diagnose cases such as if (Foo* x = foo(); not x) {
// ...
} else {
bar(*x); // We should not diagnose this.
}And 2. should help minimise false positives; e.g. using a variable that is known to be null in the else branch feels like a bit of a weird thing to do—though I guess someone could reuse it by assigning to it.
Yeah, that’s more or less what I meant by ‘somehow have scope information to figure out where a given use of it is’. |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp -- clang/test/Sema/warn-conditional-scope.cpp clang/lib/Sema/SemaStmt.cpp
View the diff from clang-format here.diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index f0f5c6238..a6dc71973 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -21,11 +21,11 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ExprObjC.h"
#include "clang/AST/IgnoreExpr.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/StmtCXX.h"
#include "clang/AST/StmtObjC.h"
#include "clang/AST/TypeLoc.h"
#include "clang/AST/TypeOrdering.h"
-#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Basic/TargetInfo.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Sema/EnterExpressionEvaluationContext.h"
@@ -978,26 +978,28 @@ StmtResult Sema::ActOnIfStmt(SourceLocation IfLoc,
if (auto *CondVar = Cond.get().first) {
ConditionVar = dyn_cast<VarDecl>(CondVar);
- }
+ }
if (ConditionVar) {
struct ElseVariableUsageChecker
- : public RecursiveASTVisitor<ElseVariableUsageChecker> {
+ : public RecursiveASTVisitor<ElseVariableUsageChecker> {
VarDecl *TargetVar;
Sema &SemaRef;
- ElseVariableUsageChecker(VarDecl *Var, Sema &S) : TargetVar(Var), SemaRef(S) {}
+ ElseVariableUsageChecker(VarDecl *Var, Sema &S)
+ : TargetVar(Var), SemaRef(S) {}
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
if (DRE->getDecl() == TargetVar) {
- SemaRef.Diag(DRE->getBeginLoc(), diag::warn_out_of_scope_var_usage) << TargetVar->getName();
+ SemaRef.Diag(DRE->getBeginLoc(), diag::warn_out_of_scope_var_usage)
+ << TargetVar->getName();
}
return true;
}
- };
+ };
ElseVariableUsageChecker Checker(ConditionVar, *this);
Checker.TraverseStmt(elseStmt);
- }
+ }
}
if (ConstevalOrNegatedConsteval ||
|
I think that is reasonable, though we'd need to make sure
|
|
Yes, std::expected would be a false positive. But how would you tell apart std::expected, where a falsey object still contains something usable, from std::unique_ptr, where it does not? One option would be simply ignore that - this warning applies to raw pointers only, not even integers. It can always be expanded later, if we can think of how.
I wouldn't get my hopes up, not until forgetting a return statement errors by default. Though I do agree both should error by default. |
Yeah, this is pretty much what I had in mind. We could hard-code some common standard-library types too (e.g.
Tell me about it... |
Not seeing an impact when applying this patch -- but that's probably expected as the warning is not enabled by default? I'd probably additional changes to enable it. |
It does the analysis all the time, but the situation in which it uses the analysis (if with condition var, also has an else) is reasonably limited. So you'd need a fairly sizable 'else' for the visitor to have to do a bunch of work. I don't anticipate the situation there to be particularly common in our build suite though. That said, this is still going to be a pretty bad perf in cases that DO have that, just not in our test suite. |
|
I agree the worst case would be bad; if I'm reading things correctly, it'd be quadratic for things like if (int* a = something()) return *a;
else if (int* b = something()) return *b;
else if (int* c = something()) return *c;
else if (int* d = something()) return *d;
else if (int* e = something()) return *e;
else return -1;where it'd check the Though I'd expect such code patterns to be rare in practice, and the constant factor to be good. (Also github stop re-checking mergeability every single time someone updates main, it messes with the scrollbar) |
Any specific code changes you want to suggest? |
I suggested something using the |
There was a problem hiding this comment.
I don't think this diagnostic is a good explanation of the problem.
The issue is that ptr must be a nullptr in the else branch and therefore dereferencing it will be undefined behavior.
It is definitely in scope otherwise that would be ill-formed.
arrowten
force-pushed
the
fix/145506-warn-conditional-scope
branch
from
232cb40 to
d0f063a
Compare
|
@nikic @shafik @erichkeane @Alcaro @Sirraide Sorry to tag you all. I made the changes according to
and
Any more changes that I should be doing? |
clang/lib/Sema/SemaStmt.cpp
Outdated
| VarDecl *ConditionVar = dyn_cast_if_present<VarDecl>(Cond.get().first); | ||
|
|
||
| if (ConditionVar) { | ||
| struct ElseVariableUsageChecker |
There was a problem hiding this comment.
I mentioned a few times, we shouldn't be using the recursive ast visitor. Scope can store the variables in the stack on an if and check those. Also, since this is so costly, we shouldn't be doing it unless th e diagnostic is enabled.
arrowten
force-pushed
the
fix/145506-warn-conditional-scope
branch
from
d0f063a to
e5ded95
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (arrowten) ChangesThis patch adds a warning when variable(s) declared in if/else-if scope are used in else-if/else branch(s) Full diff: https://github.com/llvm/llvm-project/pull/156436.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c934fed2c7462..daabb3cd188d8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -13594,6 +13594,9 @@ def warn_acc_var_referenced_lacks_op
"reference has no effect">,
InGroup<DiagGroup<"openacc-var-lacks-operation">>,
DefaultError;
+def warn_out_of_scope_var_usage
+ : Warning<"variable %0 declared in 'if' block is either false or null">,
+ InGroup<DiagGroup<"conditional-scope">>;
// AMDGCN builtins diagnostics
def err_amdgcn_load_lds_size_invalid_value : Error<"invalid size value">;
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 5625fb359807a..93795db87427e 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -971,6 +971,35 @@ StmtResult Sema::ActOnIfStmt(SourceLocation IfLoc,
if (!ConstevalOrNegatedConsteval && !elseStmt)
DiagnoseEmptyStmtBody(RParenLoc, thenStmt, diag::warn_empty_if_body);
+ // Checks for if condition variable usage in else scope
+ if (elseStmt) {
+ if (auto* CondVar = dyn_cast_or_null<VarDecl>(Cond.get().first)) {
+ bool usedInElse = false;
+ std::function<bool(Stmt*)> checkForUsage = [&](Stmt *S) -> bool {
+ if (!S) return false;
+ if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(S)) {
+ if (DRE->getDecl() == CondVar) {
+ return true;
+ }
+ }
+
+ for (Stmt *Child: S->children()) {
+ if (checkForUsage(Child)) {
+ return true;
+ }
+ }
+
+ return false;
+ };
+ usedInElse = checkForUsage(elseStmt);
+
+ if (usedInElse) {
+ Diag(elseStmt->getBeginLoc(), diag::warn_out_of_scope_var_usage)
+ << CondVar->getName();
+ }
+ }
+ }
+
if (ConstevalOrNegatedConsteval ||
StatementKind == IfStatementKind::Constexpr) {
auto DiagnoseLikelihood = [&](const Stmt *S) {
diff --git a/clang/test/Sema/warn-conditional-scope.cpp b/clang/test/Sema/warn-conditional-scope.cpp
new file mode 100644
index 0000000000000..b4b4e2e163c2f
--- /dev/null
+++ b/clang/test/Sema/warn-conditional-scope.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_2 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_3 %s
+
+int *get_something();
+int *get_something_else();
+int *get_something_else_again();
+int *get_something_else_again_now();
+
+#ifdef TEST_1
+
+int test() {
+ if (int *ptr = get_something()) {
+ return ptr[0] * ptr[0];
+ }
+ // expected-warning@+1{{variable ptr declared in 'if' block is either false or null}}
+ else if (int *ptr2 = get_something_else()) {
+ return ptr[0] * ptr2[0];
+ }
+ // expected-warning@+1{{variable ptr2 declared in 'if' block is either false or null}}
+ else if (int* ptr3 = get_something_else_again()) {
+ return ptr[0] * ptr2[0] * ptr3[0];
+ }
+ // expected-warning@+1{{variable ptr3 declared in 'if' block is either false or null}}
+ 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];
+ }
+ // expected-warning@+1{{variable ptr declared in 'if' block is either false or null}}
+ else if (int *ptr2 = get_something_else()) {
+ return ptr2[0] * ptr2[0];
+ }
+ // expected-warning@+1{{variable ptr2 declared in 'if' block is either false or null}}
+ 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@+1{{variable ptr2 declared in 'if' block is either false or null}}
+ else if (int* ptr3 = get_something_else_again()) {
+ return ptr2[0] * ptr2[0] * ptr3[0];
+ }
+ else {
+ return -1;
+ }
+}
+
+#endif
|
|
@erichkeane I tried with Scope but couldn't emit the warning. So this is the best I could come up with. If it still needs to be done with Scope, I will require a bit of help. Please let me know if this is correct. |
clang/lib/Sema/SemaStmt.cpp
Outdated
| @@ -971,6 +971,35 @@ StmtResult Sema::ActOnIfStmt(SourceLocation IfLoc, | |||
| if (!ConstevalOrNegatedConsteval && !elseStmt) | |||
| DiagnoseEmptyStmtBody(RParenLoc, thenStmt, diag::warn_empty_if_body); | |||
|
|
|||
| // Checks for if condition variable usage in else scope | |||
| if (elseStmt) { | |||
There was a problem hiding this comment.
Hmm.. This whole thing is a little too pattern matchy for me, and without a visitor is now going to just do this the 'hard' way.
I think ActOnIfStmt is probably the wrong place for this entirely, its too late at this point. We should probably do it somewhere in ActOnVariableDeclarator (or something like that?) and check the scope. Scope itself doesn't do diagnostics, but will have the context that we are 'in' a for loop, and can store the condition variable.
As this is written, an if-else chain ends up having the last else body checked N times. In addition to everything else, thats just wrong.
Instead, when we see a var-reference being created, we should be checking all the active if statements, and be variables that we've stored along the way.
There was a problem hiding this comment.
@erichkeane Please take look at the changes. Let me know if this can be considered.
arrowten
force-pushed
the
fix/145506-warn-conditional-scope
branch
from
e5ded95 to
52ce1d2
Compare
| @@ -13594,6 +13594,9 @@ def warn_acc_var_referenced_lacks_op | |||
| "reference has no effect">, | |||
| InGroup<DiagGroup<"openacc-var-lacks-operation">>, | |||
| DefaultError; | |||
| def warn_out_of_scope_var_usage | |||
| : Warning<"variable %0 declared in 'if' block is either false or null">, | |||
There was a problem hiding this comment.
| : Warning<"variable %0 declared in 'if' block is either false or null">, | |
| : Warning<"variable %0 declared in 'if' condition is always false or null here">, |
Also, this should use %enum_select and say either ‘false’ or ‘null’, depending on which one is appropriate for the type of the variable.
clang/lib/Parse/ParseStmt.cpp
Outdated
| @@ -1526,6 +1526,44 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) { | |||
| SourceLocation ElseStmtLoc; | |||
| StmtResult ElseStmt; | |||
|
|
|||
| static llvm::DenseMap<const IdentifierInfo*, SourceLocation> UsedCondVars; | |||
There was a problem hiding this comment.
You can’t use static local variables for this; this has to be a class member somewhere. Additionally, you have to store the actual variable declaration and not just its name; otherwise, this is not going to do the right thing for cases such as
void f() {
if (bool x = foo()) {}
else {
{
bool x = foo();
if (x) {} // Should not warn here since this is a different variable.
}
}
}
clang/lib/Parse/ParseStmt.cpp
Outdated
| std::function<void(const Stmt *)> DiagnoseOutOfScopeUse = [&](const Stmt *S) { | ||
| if (!S) return; | ||
|
|
||
| for (const Stmt *Child : S->children()) { | ||
| if (!Child) continue; | ||
|
|
||
| if (const auto *DRE = dyn_cast<DeclRefExpr>(Child)) { | ||
| if (const auto *UsedVD = dyn_cast<VarDecl>(DRE->getDecl())) { | ||
| if (UsedCondVars.count(UsedVD->getIdentifier()) && UsedVD != VD) { | ||
| if (Diags.isIgnored(diag::warn_out_of_scope_var_usage, DRE->getExprLoc())) | ||
| continue; | ||
|
|
||
| Diag(DRE->getExprLoc(), diag::warn_out_of_scope_var_usage) | ||
| << UsedVD->getName(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| DiagnoseOutOfScopeUse(Child); | ||
| } | ||
| }; | ||
|
|
||
| if (ThenStmt.get()) | ||
| DiagnoseOutOfScopeUse(ThenStmt.get()); | ||
|
|
||
| if (ElseStmt.get()) | ||
| DiagnoseOutOfScopeUse(ElseStmt.get()); |
There was a problem hiding this comment.
This is basically still just the same as using RAV but with extra steps; we should instead emit the warning when Sema builds a DeclRefExpr.
There was a problem hiding this comment.
Yep, exactly. Searching the AST for this pattern is, as I've said before, the wrong approach, and I'm against this.
There was a problem hiding this comment.
@Sirraide @erichkeane Sorry for the AST approach again. Any specific function or code I should be looking into for this?
There was a problem hiding this comment.
See: #156436 (comment)
There was a problem hiding this comment.
@erichkeane @Sirraide Please take a look at the new changes. If it still does not work, then I would require some bit of help. Thanks.
There was a problem hiding this comment.
@erichkeane Please review the changes, Thankyou.
clang/lib/Parse/ParseStmt.cpp
Outdated
| std::function<void(const Stmt *)> DiagnoseOutOfScopeUse = [&](const Stmt *S) { | ||
| if (!S) return; | ||
|
|
||
| for (const Stmt *Child : S->children()) { | ||
| if (!Child) continue; | ||
|
|
||
| if (const auto *DRE = dyn_cast<DeclRefExpr>(Child)) { | ||
| if (const auto *UsedVD = dyn_cast<VarDecl>(DRE->getDecl())) { | ||
| if (UsedCondVars.count(UsedVD->getIdentifier()) && UsedVD != VD) { | ||
| if (Diags.isIgnored(diag::warn_out_of_scope_var_usage, DRE->getExprLoc())) | ||
| continue; | ||
|
|
||
| Diag(DRE->getExprLoc(), diag::warn_out_of_scope_var_usage) | ||
| << UsedVD->getName(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| DiagnoseOutOfScopeUse(Child); | ||
| } | ||
| }; | ||
|
|
||
| if (ThenStmt.get()) | ||
| DiagnoseOutOfScopeUse(ThenStmt.get()); | ||
|
|
||
| if (ElseStmt.get()) | ||
| DiagnoseOutOfScopeUse(ElseStmt.get()); |
There was a problem hiding this comment.
Yep, exactly. Searching the AST for this pattern is, as I've said before, the wrong approach, and I'm against this.
arrowten
force-pushed
the
fix/145506-warn-conditional-scope
branch
4 times, most recently
from
e77df21 to
c9fd035
Compare
|
This seems fairly expensive while not actually doing anything to check lifeness. At the very minimum we need to limit it to pointer types. There are lots of types for which that would be false positives. But ultimately maybe that check will be more effective when we can use CIR to remove the layers of c++ abstractions. Currently we can just "warn on common patterns that's bad sometimes", and that sounds something clang analyzer should do, rather than Sema. |
clang/include/clang/Sema/Sema.h
Outdated
| /// mapping each VarDecl to the `Scope*` where it was created. | ||
| /// Used to detect references to these variables outside their | ||
| /// lifetime (e.g. in the `else` branch or after the `if`). | ||
| llvm::DenseMap<const VarDecl*, Scope*> IfScopeVars; |
There was a problem hiding this comment.
This shouldn't live in Sema, the Scope itself should have a reference to the var.
clang/lib/Sema/SemaDecl.cpp
Outdated
| @@ -8382,6 +8350,11 @@ NamedDecl *Sema::ActOnVariableDeclarator( | |||
|
|
|||
| emitReadOnlyPlacementAttrWarning(*this, NewVD); | |||
|
|
|||
| if (NewVD) { | |||
There was a problem hiding this comment.
Map inserts can be expensive, and this is a default ignore daignostic, we should check the diagnostic is enabled before starting this.
Also, this looks like it is adding on ALL new variable declarators, not just condition variables. This needs to happen somewhere we absolutely know this is the condition variable of an 'if'.
clang/lib/Sema/SemaExpr.cpp
Outdated
| if (auto *VD = dyn_cast_or_null<VarDecl>(D)) { | ||
| auto It = IfScopeVars.find(VD); | ||
|
|
||
| if (It != IfScopeVars.end()) { |
There was a problem hiding this comment.
This whole bit looks expensive, we should jsut be able to walk up the Scope stack and find the appropriate scope, and pick check whether it has the var.
ALSO, the var decl probably needs to get its canonical version.
arrowten
force-pushed
the
fix/145506-warn-conditional-scope
branch
from
c9fd035 to
6cfcc64
Compare
|
@erichkeane Hi Erich, please have a look. I have not used the |
| @@ -13660,10 +13594,11 @@ def warn_acc_var_referenced_lacks_op | |||
| "reference has no effect">, | |||
| InGroup<DiagGroup<"openacc-var-lacks-operation">>, | |||
| DefaultError; | |||
| def warn_out_of_scope_var_usage | |||
| : Warning<"variable %0 declared in 'if' block is always false or null here">, | |||
| InGroup<DiagGroup<"conditional-scope">>; | |||
There was a problem hiding this comment.
Not really a fan of this wording. Perhaps we could use a %select here to decide whether this is a false or null (or perhaps even 0?). Or perhaps we should just say false always? What do others think?
Also, a note pointing to the condition-var is probably a good idea too.
There was a problem hiding this comment.
Not really a fan of this wording. Perhaps we could use a %select here to decide whether this is a false or null (or perhaps even 0?). Or perhaps we should just say false always? What do others think?
I will try that.
Also, a note pointing to the condition-var is probably a good idea too.
Its pointing to the "used" condition variable though.
There was a problem hiding this comment.
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.
@erichkeane Correct me if I am wrong.
In here, this is the current scenario
if (int *ptr = get_something()) {
return ptr[0];
}
else if (int *ptr2 = get_something_else()) {
return ptr[0] * ptr2[0]; //warns for ptr
}
else if (int *ptr3 = get_something_else_again()) {
return ptr[0] * ptr2[0] * ptr3[0]; // warns for ptr and ptr2
}
else if (int *ptr4 = get_something_else_again_now()) {
return ptr[0] * ptr2[0] * ptr3[0] * ptr4[0]; // warns for ptr, ptr2 and ptr3
}
It should warn for something else too?
|
|
||
| for (Scope *S = getCurScope(); S; S = S->getParent()) { | ||
| if (VarDecl *CV = S->getConditionVar()) | ||
| if (VD == CV) |
There was a problem hiding this comment.
I think we should still use isSameEntity, and we should probably figure out why that doesn't work. If it doesn't, I have some concerns about this.
There was a problem hiding this comment.
In the 5th testcase, I could see that it is returning true for VarX->getType() == VarY->getType() in isSameEntity.
There was a problem hiding this comment.
can you give a better example? They should be different entities...
There was a problem hiding this comment.
if (bool x = get_something()) {}
else {
{
bool x = get_something_else();
if (x) {} //throws warning here
}
}
It is throwing here since it compares the local x to the if condition x and since both have the same type, it eventually throws a warning.
There was a problem hiding this comment.
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 Sema::DeclaresSameEntity which'll handle it correctly.
| int *get_something_else_again(); | ||
| int *get_something_else_again_now(); | ||
|
|
||
| #ifdef TEST_1 |
There was a problem hiding this comment.
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.
Do you mean to use one single function and all of the testcases together?
There was a problem hiding this comment.
Not necessarily. But you can capture multiple diagnostics in the same run, so don't do macros, just do multiple checks.
| @@ -0,0 +1,146 @@ | |||
| // RUN: %clang_cc1 -fsyntax-only -verify -Wconditional-scope -DTEST_1 %s | |||
There was a problem hiding this comment.
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:
if (auto x = bar()) {
}
else {
decltype(x) f = g; // should this warn?
}
There was a problem hiding this comment.
If anyone can help with different types of testcases, I would appreciate that.
There was a problem hiding this comment.
I gave you some ideas, dependence, non-evaluated contexts, etc.
Yeah, this is a tricky case because there are valid uses but it's also reasonably easy for there to be confused uses as well. We want to diagnose the confused uses without diagnosing the valid ones, and I'm not convinced that's possible to do with a heuristic. Even if it only applies to pointers, it will still diagnose valid uses, like: it might be perfectly fine for This might make more sense as a clang-tidy check. |
9 participants
This patch adds a warning when variable(s) declared in if/else-if scope are used in else-if/else branch(s)