JS: recognize more conditionals in useless-conditional #404
Conversation
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
That is an impressive amount of new alerts, well spotted!
I have one suggestion for additional alerts, but given the current PR pressure on the js/trivial-conditional query, we may want to wait with that.
| e = cond.(LogOrExpr).getLeftOperand() | ||
| e = cond.(LogicalBinaryExpr).getLeftOperand() or | ||
| // Include `z` in `if (x && z)`. | ||
| isConditional(_, cond) and e = cond.(LogicalBinaryExpr).getRightOperand() |
There was a problem hiding this comment.
I think we should squeeze this syntactic pattern a bit more: cond.getUnderlyingValue().(LogicalBinaryExpr).getRightOperand().
| predicate isConditional(ASTNode cond, Expr e) { | ||
| e = cond.(IfStmt).getCondition() or | ||
| e = cond.(WhileStmt).getExpr() or | ||
| e = cond.(ForStmt).getTest() or |
There was a problem hiding this comment.
How about just generalising all the way to LoopStmt.getTest()? Or do we not want this to cover DoWhileStmt.getTest()?
There was a problem hiding this comment.
I noticed EphemeralLoop.ql whitelists do-while because of Emscripten, so I thought we'd run into the same issue here.
There was a problem hiding this comment.
Actually it looks like I got that backwards. EphemeralLoop.ql whitelists Emscripten-generated files because of do {} while(0), but this query already whitelists constants, so it shouldn't be a problem.
There was a problem hiding this comment.
Updated to use LoopStmt
|
Conflicts. |
asger-semmle
force-pushed
the
useless-conditional2
branch
from
96948b0 to
87e0027
Compare
|
Rebased. I've also opened ODASA-7450 for the change note, to be authored once all the changes to UselessConditional are in. |
3 participants
Reimplements the following two improvements to Useless Conditional from #380:
zinif (x && z)as a conditional. Previously onlyxandx && zwere recognised.Evaluation.