Skip to content

Commit 423f965

Browse files
authored
Fix manual_range_contains NAN handling (#17065)
For floating-point out-of-range checks like `q < 0.0 || q > 1.0`, the manual form is `false` for `NaN`, but the lint’s suggestion `!(0.0..=1.0).contains(&q)` is `true`, which changes control flow. This PR skips linting for floating-point out-of-range patterns combined with `||` or `|`, documents the limitation, and adds UI tests. In-range float checks such as `q >= 0.0 && q < 1.0` are still linted, since rewriting to `.contains()` preserves `NaN` behavior there. Edit: I chose to skip linting rather than suggest `&& !q.is_nan()`. Adding an `is_nan()` guard would be more verbose than the original code. Assumes `NaN` handling the author may not want. changelog: [`manual_range_contains`] : fix NaN handling fixes #16706
2 parents f72ea1f + 882b25c commit 423f965

4 files changed

Lines changed: 80 additions & 19 deletions

File tree

clippy_lints/src/ranges.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use std::cmp::Ordering;
2020
declare_clippy_lint! {
2121
/// ### What it does
2222
/// Checks for expressions like `x >= 3 && x < 8` that could
23-
/// be more readably expressed as `(3..8).contains(x)`.
23+
/// be more readably expressed as `(3..8).contains(&x)`.
2424
///
2525
/// ### Why is this bad?
2626
/// `contains` expresses the intent better and has less
@@ -38,6 +38,12 @@ declare_clippy_lint! {
3838
///# let x = 6;
3939
/// assert!((3..8).contains(&x));
4040
/// ```
41+
///
42+
/// ### Limitations
43+
/// Out-of-range checks on floating-point types, such as `q < 0.0 || q > 1.0`,
44+
/// are not linted. For `NaN`, that expression is `false`, but
45+
/// `!(0.0..=1.0).contains(&q)` is `true`, so the suggested rewrite would
46+
/// change control flow.
4147
#[clippy::version = "1.49.0"]
4248
pub MANUAL_RANGE_CONTAINS,
4349
style,
@@ -250,6 +256,11 @@ fn check_possible_range_contains(
250256
applicability,
251257
);
252258
} else if !combine_and && ord == Some(l.ord) {
259+
// For floating-point types, `q < lo || q > hi` evaluates to `false` for NaN,
260+
// but `!(lo..=hi).contains(&q)` evaluates to `true` for NaN, so not linting.
261+
if matches!(cx.typeck_results().expr_ty(l.expr).kind(), ty::Float(_)) {
262+
return;
263+
}
253264
// `!_.contains(_)`
254265
// order lower bound and upper bound
255266
let (l_span, u_span, l_inc, u_inc) = if l.ord == Ordering::Less {

tests/ui/range_contains.fixed

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,7 @@ fn main() {
5656
let y = 3.;
5757
(0. ..1.).contains(&y);
5858
//~^ manual_range_contains
59-
!(0. ..=1.).contains(&y);
60-
//~^ manual_range_contains
59+
y < 0. || y > 1.; // no lint: float `||` differs in NaN handling (fix #16706)
6160

6261
// handle negatives #8721
6362
(-10..=10).contains(&x);
@@ -96,3 +95,23 @@ fn msrv_1_35() {
9695
(8..35).contains(&x);
9796
//~^ manual_range_contains
9897
}
98+
99+
// Fix #16706
100+
fn float_nan_no_lint() {
101+
let q = 0.5_f64;
102+
// these still lint — float && (in-range) is fine, NaN semantics match
103+
(0.0..1.0).contains(&q);
104+
//~^ manual_range_contains
105+
(0.0..=1.0).contains(&q);
106+
//~^ manual_range_contains
107+
108+
// these do NOT lint — float || (out-of-range) NaN semantics differ
109+
q < 0.0 || q > 1.0; // no lint
110+
q < 0.0 || q >= 1.0; // no lint
111+
112+
let r = 0.5_f32;
113+
(0.0..1.0).contains(&r);
114+
//~^ manual_range_contains
115+
116+
r < 0.0 || r > 1.0; // no lint
117+
}

tests/ui/range_contains.rs

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,7 @@ fn main() {
5656
let y = 3.;
5757
y >= 0. && y < 1.;
5858
//~^ manual_range_contains
59-
y < 0. || y > 1.;
60-
//~^ manual_range_contains
59+
y < 0. || y > 1.; // no lint: float `||` differs in NaN handling (fix #16706)
6160

6261
// handle negatives #8721
6362
x >= -10 && x <= 10;
@@ -96,3 +95,23 @@ fn msrv_1_35() {
9695
x >= 8 && x < 35;
9796
//~^ manual_range_contains
9897
}
98+
99+
// Fix #16706
100+
fn float_nan_no_lint() {
101+
let q = 0.5_f64;
102+
// these still lint — float && (in-range) is fine, NaN semantics match
103+
q >= 0.0 && q < 1.0;
104+
//~^ manual_range_contains
105+
q >= 0.0 && q <= 1.0;
106+
//~^ manual_range_contains
107+
108+
// these do NOT lint — float || (out-of-range) NaN semantics differ
109+
q < 0.0 || q > 1.0; // no lint
110+
q < 0.0 || q >= 1.0; // no lint
111+
112+
let r = 0.5_f32;
113+
r >= 0.0 && r < 1.0;
114+
//~^ manual_range_contains
115+
116+
r < 0.0 || r > 1.0; // no lint
117+
}

tests/ui/range_contains.stderr

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -79,53 +79,65 @@ error: manual `Range::contains` implementation
7979
LL | y >= 0. && y < 1.;
8080
| ^^^^^^^^^^^^^^^^^ help: use: `(0. ..1.).contains(&y)`
8181

82-
error: manual `!RangeInclusive::contains` implementation
83-
--> tests/ui/range_contains.rs:59:5
84-
|
85-
LL | y < 0. || y > 1.;
86-
| ^^^^^^^^^^^^^^^^ help: use: `!(0. ..=1.).contains(&y)`
87-
8882
error: manual `RangeInclusive::contains` implementation
89-
--> tests/ui/range_contains.rs:63:5
83+
--> tests/ui/range_contains.rs:62:5
9084
|
9185
LL | x >= -10 && x <= 10;
9286
| ^^^^^^^^^^^^^^^^^^^ help: use: `(-10..=10).contains(&x)`
9387

9488
error: manual `RangeInclusive::contains` implementation
95-
--> tests/ui/range_contains.rs:66:5
89+
--> tests/ui/range_contains.rs:65:5
9690
|
9791
LL | y >= -3. && y <= 3.;
9892
| ^^^^^^^^^^^^^^^^^^^ help: use: `(-3. ..=3.).contains(&y)`
9993

10094
error: manual `RangeInclusive::contains` implementation
101-
--> tests/ui/range_contains.rs:72:30
95+
--> tests/ui/range_contains.rs:71:30
10296
|
10397
LL | (x >= 0) && (x <= 10) && (z >= 0) && (z <= 10);
10498
| ^^^^^^^^^^^^^^^^^^^^^ help: use: `(0..=10).contains(&z)`
10599

106100
error: manual `RangeInclusive::contains` implementation
107-
--> tests/ui/range_contains.rs:72:5
101+
--> tests/ui/range_contains.rs:71:5
108102
|
109103
LL | (x >= 0) && (x <= 10) && (z >= 0) && (z <= 10);
110104
| ^^^^^^^^^^^^^^^^^^^^^ help: use: `(0..=10).contains(&x)`
111105

112106
error: manual `!Range::contains` implementation
113-
--> tests/ui/range_contains.rs:75:29
107+
--> tests/ui/range_contains.rs:74:29
114108
|
115109
LL | (x < 0) || (x >= 10) || (z < 0) || (z >= 10);
116110
| ^^^^^^^^^^^^^^^^^^^^ help: use: `!(0..10).contains(&z)`
117111

118112
error: manual `!Range::contains` implementation
119-
--> tests/ui/range_contains.rs:75:5
113+
--> tests/ui/range_contains.rs:74:5
120114
|
121115
LL | (x < 0) || (x >= 10) || (z < 0) || (z >= 10);
122116
| ^^^^^^^^^^^^^^^^^^^^ help: use: `!(0..10).contains(&x)`
123117

124118
error: manual `Range::contains` implementation
125-
--> tests/ui/range_contains.rs:96:5
119+
--> tests/ui/range_contains.rs:95:5
126120
|
127121
LL | x >= 8 && x < 35;
128122
| ^^^^^^^^^^^^^^^^ help: use: `(8..35).contains(&x)`
129123

130-
error: aborting due to 21 previous errors
124+
error: manual `Range::contains` implementation
125+
--> tests/ui/range_contains.rs:103:5
126+
|
127+
LL | q >= 0.0 && q < 1.0;
128+
| ^^^^^^^^^^^^^^^^^^^ help: use: `(0.0..1.0).contains(&q)`
129+
130+
error: manual `RangeInclusive::contains` implementation
131+
--> tests/ui/range_contains.rs:105:5
132+
|
133+
LL | q >= 0.0 && q <= 1.0;
134+
| ^^^^^^^^^^^^^^^^^^^^ help: use: `(0.0..=1.0).contains(&q)`
135+
136+
error: manual `Range::contains` implementation
137+
--> tests/ui/range_contains.rs:113:5
138+
|
139+
LL | r >= 0.0 && r < 1.0;
140+
| ^^^^^^^^^^^^^^^^^^^ help: use: `(0.0..1.0).contains(&r)`
141+
142+
error: aborting due to 23 previous errors
131143

0 commit comments

Comments
 (0)