-
Notifications
You must be signed in to change notification settings - Fork 130
fix: match_between rewriting #4945
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
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) { | ||
| (true, false) => lhs.clone(), | ||
| (false, true) => BinaryExpr::new(lhs.rhs().clone(), lhs.op().swap()?, lhs.lhs().clone()), | ||
| _ => return None, |
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 assume that a variable could be between two other variables?
a <= x <= b
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 function only does the rewrite if it's variable between two lits
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 see, I guess we can leave it like than, but there is not reason this needs to be the case
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
891d8d3 to
221b7cd
Compare
| _ => return None, | ||
| }; | ||
|
|
||
| let (left, left_op, right, right_op) = if left_lit.value() > right_lit.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 was the bad piece
joseph-isaacs
left a comment
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.
LG
Found this while fuzzing, the failure was
Was confused how the expression was getting reverse. The previous logic was just discarding information and picking the larger lit as the upper bound. This version should be more robust.
I've added a unit test for this case, it fails with the old version of
find_betweenand succeeds now. I've also confirmed my fuzz artifact succeeds when I run with new code: