You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Context: #7692 has already bumped the float_cmp lint from deny-by-default correctness to allow-by-default pedantic. This lint is by far the most suppressed correctness lint on crates.io with over triple the number of suppressions than the second place.
In my opinion either restriction or nursery would be the more appropriate choice for this lint.
Using the example code from the lint's documentation, a typical manifestation of this lint is:
error: strict comparison of `f32` or `f64` --> src/main.rs:7:4 |7 | if y != x {} // where both are floats | ^^^^^^ help: consider comparing them within some margin of error: `(y - x).abs() > error_margin` | = note: `f32::EPSILON` and `f64::EPSILON` are available for the `error_margin` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp
As described in #6816, for most ranges of y and x the tentative suggested use of EPSILON is totally meaningless, as it ends up being equivalent to an exact equality check. Thus if someone doesn't already understand what they are doing before seeing this message, they are going to continue not understanding what they are doing after getting the lint, just with more confusing code. In my view this makes it unsuitable to be a pedantic lint.
Why restriction makes sense: I am prepared to believe that this lint as currently implemented can be helpful in niche use cases involving a history of floating point bugs, where the developer would choose to rule out exact comparison by enabling a restriction lint.
Why nursery makes sense: I am also prepared to believe that with some more effort to categorize and recognize particular problematic patterns, this lint could be made to give better turnkey recommendations without such a high incidence of being triggered in unactionable places.
Having had this lint enabled for the development of a ~20 000 line FP-heavy codebase, I agree. There are lots of cases where exact equality is perfectly correct.
I think the issue is that, while floating point is not perfectly exact under arithmetic operations and special functions, there are several float operations under which equality is well-behaved (modulo NaN and −0):
abs, signum, etc.
ceil, round, etc.
max, clamp, etc.
from_bits, to_ne_bytes, From<T>, etc.
std::convert::identity. I've found this to be the most common case. You set foo.x = 5.0 somewhere, and later you check foo.x == 5.0. Ubiquitous in tests, but also happens in application code.
Context: #7692 has already bumped the
float_cmp
lint from deny-by-defaultcorrectness
to allow-by-defaultpedantic
. This lint is by far the most suppressedcorrectness
lint on crates.io with over triple the number of suppressions than the second place.Following up on #7692 (review) and @giraffate's next comment:
In my opinion either
restriction
ornursery
would be the more appropriate choice for this lint.Using the example code from the lint's documentation, a typical manifestation of this lint is:
As described in #6816, for most ranges of
y
andx
the tentative suggested use ofEPSILON
is totally meaningless, as it ends up being equivalent to an exact equality check. Thus if someone doesn't already understand what they are doing before seeing this message, they are going to continue not understanding what they are doing after getting the lint, just with more confusing code. In my view this makes it unsuitable to be apedantic
lint.Why
restriction
makes sense: I am prepared to believe that this lint as currently implemented can be helpful in niche use cases involving a history of floating point bugs, where the developer would choose to rule out exact comparison by enabling a restriction lint.Why
nursery
makes sense: I am also prepared to believe that with some more effort to categorize and recognize particular problematic patterns, this lint could be made to give better turnkey recommendations without such a high incidence of being triggered in unactionable places.@rustbot label +S-needs-discussion
The text was updated successfully, but these errors were encountered: