-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Demote float_cmp to pedantic #7692
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @giraffate (or someone else) soon. Please see the contribution instructions for more information. |
54eeb4d
to
60786ff
Compare
IMO at least it should be @rust-lang/clippy do you have any opinions on this? |
I think the FP rate is too high for warn-by-default. |
I would always be critical about float comparisons like |
I'm not sure if this should be restriction, but perhaps pedantic? Restriction lints are for things "not generally considered problematic". |
That sounds fair. Pedantic is for "lints which are rather strict or might have false positives". I did consider suspicious briefly, but I took a closer look at actual use and found that people are often using float equality for the right reasons and getting heat from clippy then. e.g. #2834. And the guidance from the lint is itself a touch questionable, as mentioned in #6816. If it was honed in so that it mostly didn't fire on the "right" uses, and the guidance was unambiguously Good, maybe suspicious would be best, because then silencing it would be a "yes, I know what I am doing" signal. As-is, it's a bit much and risks mis-teaching the subject. I am happy with pedantic, so let's go with that? |
60786ff
to
3ad3c51
Compare
I'm a little on the fence since I think this may have a high FP rate even for the pedantic category. But I'm okay with pedantic. The lint docs probably should have a Known problems section. |
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.
IMO either restriction
or nursery
would be the appropriate choice here.
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.
I am prepared to believe that this lint 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 the restriction
lint.
Thanks for comments! I'm thinking of merging this as is, since it should at least be allow-by-default. And the categories can be discussed in another issue. If it looks good, I'll merge this later. |
SGTM. I filed #7725 to follow up. |
@bors r+ Thanks! |
📌 Commit 3ad3c51 has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
* Refactor bank rule code to be more rust-idiomatic * Refactor `chain` bank rule handler * Do not classify language courses as "malags" * Remove `float_cmp` allow, now default rust-lang/rust-clippy#7692 * Set course status type outside of condition
See this issue: #7666
This is one of the most frequently suppressed lints. It is deny-by-default. It is not actually clearly wrong, as there are many instances where direct float comparison is actually desirable. It is only after operating on floats that they may lose precision, and that depends greatly on the operation. As most correctness lints have a much higher standard of error, being based on hard and fast binary logic, this should not be amongst them.
A linter is not a substitute for observing the math carefully and running tests, and doing the desirable thing is even more likely to lead one to want exact comparisons.
changelog: Demote [
float_cmp
] from correctness to pedantic lints