Clippy suggests !RangeInclusive::contains for exclusive float range comparisons

[eldruin]

I tried this code:

if humidity_percentage < 0.0 || humidity_percentage > 100.0 {
// ...
}

I expected to see this happen: no lint

Instead, this happened:
Clippy suggested to replace the "manual range implementation" with this: !(0.0..=100.0).contains(&humidity_percentage)
However, as some floating point numbers might not be exactly representable in IEEE-754, I think creating a range that does contain them (independently of what approximation the compiler actually settles for, the code suggests an exact number range) and presumably checking for equality on the edges (only to negate the whole thing afterwards) is not a nice suggestion.

I think it would be better not to suggest this in this case and maybe also not on similar cases concerning floating point numbers.

Meta

• cargo clippy -V: clippy 0.0.212 (1700ca0 2020-12-08)
• rustc -Vv:

rustc 1.50.0-nightly (1700ca07c 2020-12-08)
binary: rustc
commit-hash: 1700ca07c6dd7becff85678409a5df6ad4cf4f47
commit-date: 2020-12-08
host: x86_64-unknown-linux-gnu
release: 1.50.0-nightly

This lint has also already made it to the beta channel.

Backtrace

warning: manual `!RangeInclusive::contains` implementation
  --> src/app_mode.rs:65:12
   |
65 |           if humidity_percentage < 0.0
   |  ____________^
66 | |             || humidity_percentage > 100.0
   | |__________________________________________^ help: use: `!(0.0..=100.0).contains(&humidity_percentage)`
   |
   = note: `#[warn(clippy::manual_range_contains)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains

warning: 1 warning emitted

    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

[ebroto]

Hi @eldruin, thanks for your interest in improving Clippy!

IIUC, the reasoning behind your disagreement would apply to the concept itself of a range of floating point numbers, right? Implementation-wise, the range is not doing something very different from what you are doing, see here.

[eldruin]

Hi @ebroto :)

IIUC, the reasoning behind your disagreement would apply to the concept itself of a range of floating point numbers, right? Implementation-wise, the range is not doing something very different from what you are doing, see here.

Yes. My point would be not recommending the use of inclusive ranges or any way of equality comparison when floats are involved due to the inherent representation issues in the IEEE-754 format.

In my case, I agree implementation-wise it would probably work either way and it is a trivial input range check anyway. I just happened to notice with that code.

[ebroto]

Since the range itself is not an incorrect suggestion per se as it's not changing the semantics of the original code, I personally disagree with your conclusion, but let's see what others think about it!

[kalcutter]

This clippy suggestion does change the semantics of the code:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=97a03cef8ebac2954dd60777656da949

[jw013]

There's an argument to be made that this is still a helpful lint. A very common pattern to try to classify floats into two buckets: valid and invalid.

• The valid case is usually one bounded interval or one half-bounded interval. It's probably reasonable to assume the scenario where the valid case is the union of two half-bounded intervals is much rarer compared to the other two. Thus if we see a conditional that looks like it is checking the union of two disjoint half-bounded intervals, in most cases we can assume it is an attempt to express an invalid bucket test.
• It's also probably reasonable to assume that in most cases, NAN is intended to go in the invalid bucket.
• NAN does not compare true to any floating point value with any comparison operator. Thus, any attempt to test for invalid floating point values with comparison operators is likely to be incorrect and fail to catch NAN.

Thus a clippy lint could warn against a x < a || x > b test and explicitly explain something like:

It looks like you are trying to test for floating point values that lie outside a closed interval [a, b]. If you intended to include NaN values in this test, either include an explicit test for NaN or negate a test for containment inside the interval, e.g.

x.is_nan() || x < a || x > b

or:

!(a..=b).contains(x)

This would be more of a correctness lint than a style one, so it could probably use a new name, perhaps manual_range_excludes_nan or something (I dunno, naming things is hard, someone can probably come up with a better one).