rust-lang / rust-lang/rust-clippy

Consider downgrading float_cmp to restriction or nursery

Open
#7,725 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

S-needs-discussion
Dominant language
Rust
Stars
13.5k
Forks
2.2k
Avg merge
2d 10h
Merged PRs (30d)
32

Description

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.

Following up on https://github.com/rust-lang/rust-clippy/pull/7692#pullrequestreview-760208632 and @giraffate's next comment:

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 https://github.com/rust-lang/rust-clippy/issues/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.

@rustbot label +S-needs-discussion

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading the discussion in issues #7692 and #6816, along with the linked pull-request review, to understand the concerns about float_cmp's category and suggestion. Inspect the existing float_cmp lint classification and determine whether restriction or nursery is appropriate; done means the category decision is resolved and the corresponding Clippy changes are validated.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
devtools
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.