`eq_op` lint in clippy is confusing for floats

[Takashiidobe]

Summary

Clippy suggests the eq_op lint in cases where it's not clear how to fix the underlying problem.

I was looking at libm's code (https://github.com/rust-lang/libm/tree/cb2ffdf5435d3302c97a27c8ce7de48e214de037). For this block (https://github.com/rust-lang/libm/blob/cb2ffdf5435d3302c97a27c8ce7de48e214de037/src/math/sinf.rs#L81-L83), clippy shows an error, because this triggers the eq_op lint: https://rust-lang.github.io/rust-clippy/master/index.html#/eq_op.

In my mind, this makes sense, since x - x should always be 0. But, if I swap the line return x - x; to return 0.0;, tests fail immediately in the repo.

thread 'sinf_matches_musl' panicked at /home/takashi/.build/debug/build/libm-33ae80df27fee36a/out/musl-tests.rs:71754:9:
INPUT: [2139095040] EXPECTED: [4290772992] ACTUAL 0.0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

as far as I can tell, the guard clause of if ix >= 0x7f800000 { means that x's bitwise representation is infinity or NaN, so the return can be return f32::NAN instead of return x - x;. This works because NaN - NaN and inf - inf are NaN: (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b060559e29d69eca8c424429b908ad27), and thus the tests pass again.

Is there a way to issue better diagnostics in this case / have the lint explain some common pitfalls with this lint when applied to floats? In the case of libm, return x - x; is intentional because it's basically doing x.is_nan() || x.is_infinite() before doing the operation, so return x - x; looks to be totally fine.

Reproducer

No response

Version

rustc 1.75.0 (82e1608df 2023-12-21)
binary: rustc
commit-hash: 82e1608dfa6e0b5569232559e3d385fea5a93112
commit-date: 2023-12-21
host: x86_64-unknown-linux-gnu
release: 1.75.0
LLVM version: 17.0.6

Additional Labels

No response

[J-ZhengLi]

currently there's a note suggest using is_nan: eq_op.rs

However, it only apears with not equal operand... So, yeah,

Is there a way to issue better diagnostics in this case / have the lint explain some common pitfalls with this lint when applied to floats?

this lint will need some extra explaination.

One more thing, since the is_nan is still behind an unstable feature, should we really suggest it tho?

[Takashiidobe]

I think taking the note from the not equal operand and also applying it to subtraction should work (in this case). That would've left me less confused when I tried to replace the return x - x; with return 0.0;.

is_nan isn't unstable, the const version of it currently is. If it's ok to suggest it in the case of !=, I don't see why it wouldn't be ok to suggest for other binary ops as well.

As a follow-up, I wonder what other lints could be applied to floats or if this lint could be applied in other cases. Floats are pretty tricky to deal with (case in point, I didn't understand them) and it would be nice to have some lints in other cases.

If adding this case to clippy makes sense, I can get started working on it.

[Rudxain]

this lint could be applied in other cases

Yes! In general, the lint should be deny if both values mutually impl Eq (reflexive), but warn if they only impl PartialEq.

AFAIK, it's impossible for a single lint to have multiple levels, so it should be split in 2:

• eq_op for total Eq
• part_eq_op for PartialEq

eq_op would have to be modified to never trigger when part_eq_op would trigger (mutually exclusive), regardless of whether part_eq_op is warn or allow.

That wouldn't be a breaking-change, if it wasn't for #[expect(clippy::eq_op)], which (after the hypothetical update) would trigger warns in some cases