-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Prune CFG for obviously impossible true/false
edges
#17602
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
Conversation
/** | ||
* A completion that represents evaluation of an expression | ||
* with a Boolean value. | ||
*/ | ||
class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion { | ||
BooleanCompletion() { this = TBooleanCompletion(value) } | ||
|
||
override predicate isValidForSpecific(AstNode e) { | ||
private predicate isValidForSpecific0(AstNode e) { |
Check warning
Code scanning / CodeQL
Candidate predicate not marked as `nomagic` Warning
isValidForSpecific
true/false
edgestrue/false
edges
private predicate isBooleanConstant(AstNode n, Boolean value) { | ||
n.(LiteralExpr).getTextValue() = value.toString() | ||
or | ||
isBooleanConstant(n.(ParenExpr).getExpr(), value) |
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.
This looks a little odd. What about parentheses around other expressions? I'd have expected something like:
...
or
parent = any(BlockExpr be | e = be.getStmtList().getTailExpr())
or
parent = any(ParenExpr pe | e = pe.getExpr())
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.
Actually, the ParenExpr
case is a workaround for now; once we add ConditonalSplitting
a la Ruby, we will only need the LiteralExpr
case, and edges out of ParenExpr
, BlockExpr
, etc will then be restricted via splitting.
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.
Makes sense, I was just surprised to see that ParenExpr
was handled differently than BlockExpr
. The control flow of {{{expr}}}
isn't very different from (((expr)))
.
override predicate isValidForSpecific(AstNode e) { | ||
this.isValidForSpecific0(e) and | ||
( | ||
isBooleanConstant(e, value) |
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 would have expected some logic in the last()
predicate of a LiteralExprTree
that restricts the outgoing edge for a boolean literal to matching ConditionalCompletion.
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.
That happens automatically via the completionIsValidFor
predicate.
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.
It's not really "automatic", is it? You either handle it explicitly in isValidForSpecific
or in LiteralExprTree
. Either way should be fine, I just have a slight preference for handling it in LiteralExprTree
. In the end it is a matter of "taste", so let's leave it the way you prefer.
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.
Looks good to me.
In preparation for #17525.
For now, only recognize simple (parenthesized) Boolean literals.