Added a lint which corrects expressions like (a - b) < f32::EPSILON, according to #5819 #5952
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
| // check if expr is a binary expression with a lt or gt operator | ||
| if let ExprKind::Binary(op, ref left, ref right) = expr.kind { | ||
| match op.node { | ||
| BinOpKind::Lt => { |
There was a problem hiding this comment.
I think this can be done as if let ExprKind::Binary(Lt | Rt, left, right) ...
There was a problem hiding this comment.
I don't know if I get this wrong, but I still need to differentiate between BinOpKind::Lt and BinOpKind::Gt. If I would use something like
if let ExprKind::Binary( Spanned { node: BinOpKind::Lt | BinOpKind::Gt, .. }, left, right, ) = ...
Then I would still need to sort out whether my Operator is < or >. Or am I wrong?
There was a problem hiding this comment.
oh sorry, you're absolutely right
|
|
||
| // right hand side matches either f32::EPSILON or f64::EPSILON | ||
| if let ExprKind::Path(ref epsilon_path) = rhs.kind; | ||
| if match_qpath(epsilon_path, &["f32", "EPSILON"]) || match_qpath(epsilon_path, &["f64", "EPSILON"]); |
There was a problem hiding this comment.
|
@bors r+ thanks! |
|
📌 Commit 179df0b has been approved by |
|
💔 Test failed - checks-action_test |
|
Mh.. looks like an error with the latest nightly version. Should I fix this by removing the |
flip1995
added
S-waiting-on-bors
|
Let's wait. I'll fix this in a separate PR. |
|
@bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
| --> $DIR/float_equality_without_abs.rs:13:13 | ||
| | | ||
| LL | let _ = (a - b) < f32::EPSILON; | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b).abs()` |
There was a problem hiding this comment.
This suggestion doesn't work. If applied, this will transform the code like this:
let _ = (a - b) < f32::EPSILON; // before
let _ = (a - b).abs(); // afterA better way to do this would be to use span_lint_and_then and then use the db.span_suggestion to only add the above suggestion to the span of (a - b).
There was a problem hiding this comment.
Oh you're right! Thank you @flip1995 and sorry that I didn't see that earlier! I've corrected it on my own fork. Should I open a new PR for this fix? I've also started to use utils::Sugg. But I had some issues while parsing the expression, the simple way of
let suggestion = format!("{}.abs()", sugg::Sugg::hir(cx, &lhs, "..").maybe_par());always brought up doubled parentheses. E.g. (a - b) -> ((a - b)). That's why I took a longer way and assembled the suggestion by myself. Maybe you could have a look on the code, it's all in this commit here.
There was a problem hiding this comment.
The linked commit looks good to me.
Yes please open a new PR about this!
| // get the snippet string | ||
| let lhs_string = snippet( | ||
| cx, | ||
| lhs.span, | ||
| "(...)", | ||
| ); | ||
| // format the suggestion | ||
| let suggestion = if lhs_string.starts_with('(') { | ||
| format!("{}.abs()", lhs_string) | ||
| } else { | ||
| format!("({}).abs()", lhs_string) | ||
| }; |
There was a problem hiding this comment.
@1c3t3a If you want to further improve this code, you can use utils::Sugg here, that will take care about this automatically.
Corrects the float_equality_without_abs lint Fixes an issue in the `float_equality_without_abs` lint. The lint suggestion was configured in a way that it lints the whole error and not just the subtraction part. In the current configuration the lint would suggest to change the expression in a wrong way, e.g. ```rust let _ = (a - b) < f32::EPSILON; // before let _ = (a - b).abs(); // after ``` This was dicovered by @flip1995. (See discussion of PR #5952). Also the suggestion is now formatted via `utils::sugg`. changelog: none
Fixes #5819
changelog: none