Skip to content

Commit acd4342

Browse files
flip1995flip1995
authored
Rollup merge of rust-lang#5345 - Toxyxer:add-lint-for-float-in-array-comparison, r=flip1995
Add lint for float in array comparison Fixes rust-lang#4277 changelog: - Added new handler for expression of index kind (e.g. `arr[i]`). It returns a constant when both array and index are constant, or when the array is constant and all values are equal. - Trigger float_cmp and float_cmp_const lint when comparing arrays. Allow for comparison when one of the arrays contains only zeros or infinities. - Added appropriate tests for such cases.
2 parents 7907abe + 4348af2 commit acd4342

File tree

6 files changed

+200
-71
lines changed

6 files changed

+200
-71
lines changed

‎clippy_lints/src/consts.rs‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,7 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
268268
}
269269
}
270270
},
271+
ExprKind::Index(ref arr, ref index) => self.index(arr, index),
271272
// TODO: add other expressions.
272273
_ => None,
273274
}
@@ -345,6 +346,31 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
345346
}
346347
}
347348

349+
fn index(&mut self, lhs: &'_ Expr<'_>, index: &'_ Expr<'_>) -> Option<Constant> {
350+
let lhs = self.expr(lhs);
351+
let index = self.expr(index);
352+
353+
match (lhs, index) {
354+
(Some(Constant::Vec(vec)), Some(Constant::Int(index))) => match vec[index as usize] {
355+
Constant::F32(x) => Some(Constant::F32(x)),
356+
Constant::F64(x) => Some(Constant::F64(x)),
357+
_ => None,
358+
},
359+
(Some(Constant::Vec(vec)), _) => {
360+
if !vec.is_empty() && vec.iter().all(|x| *x == vec[0]) {
361+
match vec[0] {
362+
Constant::F32(x) => Some(Constant::F32(x)),
363+
Constant::F64(x) => Some(Constant::F64(x)),
364+
_ => None,
365+
}
366+
} else {
367+
None
368+
}
369+
},
370+
_ => None,
371+
}
372+
}
373+
348374
/// A block can only yield a constant if it only has one constant expression.
349375
fn block(&mut self, block: &Block<'_>) -> Option<Constant> {
350376
if block.stmts.is_empty() {
@@ -492,6 +518,41 @@ pub fn miri_to_const(result: &ty::Const<'_>) -> Option<Constant> {
492518
},
493519
_ => None,
494520
},
521+
ty::ConstKind::Value(ConstValue::ByRef { alloc, offset: _ }) => match result.ty.kind {
522+
ty::Array(sub_type, len) => match sub_type.kind {
523+
ty::Float(FloatTy::F32) => match miri_to_const(len) {
524+
Some(Constant::Int(len)) => alloc
525+
.inspect_with_undef_and_ptr_outside_interpreter(0..(4 * len as usize))
526+
.to_owned()
527+
.chunks(4)
528+
.map(|chunk| {
529+
Some(Constant::F32(f32::from_le_bytes(
530+
chunk.try_into().expect("this shouldn't happen"),
531+
)))
532+
})
533+
.collect::<Option<Vec<Constant>>>()
534+
.map(Constant::Vec),
535+
_ => None,
536+
},
537+
ty::Float(FloatTy::F64) => match miri_to_const(len) {
538+
Some(Constant::Int(len)) => alloc
539+
.inspect_with_undef_and_ptr_outside_interpreter(0..(8 * len as usize))
540+
.to_owned()
541+
.chunks(8)
542+
.map(|chunk| {
543+
Some(Constant::F64(f64::from_le_bytes(
544+
chunk.try_into().expect("this shouldn't happen"),
545+
)))
546+
})
547+
.collect::<Option<Vec<Constant>>>()
548+
.map(Constant::Vec),
549+
_ => None,
550+
},
551+
// FIXME: implement other array type conversions.
552+
_ => None,
553+
},
554+
_ => None,
555+
},
495556
// FIXME: implement other conversions.
496557
_ => None,
497558
}

‎clippy_lints/src/misc.rs‎

Lines changed: 59 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -370,26 +370,28 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints {
370370
return;
371371
}
372372
}
373-
let (lint, msg) = if is_named_constant(cx, left) || is_named_constant(cx, right) {
374-
(FLOAT_CMP_CONST, "strict comparison of `f32` or `f64` constant")
375-
} else {
376-
(FLOAT_CMP, "strict comparison of `f32` or `f64`")
377-
};
373+
let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
374+
let (lint, msg) = get_lint_and_message(
375+
is_named_constant(cx, left) || is_named_constant(cx, right),
376+
is_comparing_arrays,
377+
);
378378
span_lint_and_then(cx, lint, expr.span, msg, |db| {
379379
let lhs = Sugg::hir(cx, left, "..");
380380
let rhs = Sugg::hir(cx, right, "..");
381381

382-
db.span_suggestion(
383-
expr.span,
384-
"consider comparing them within some error",
385-
format!(
386-
"({}).abs() {} error",
387-
lhs - rhs,
388-
if op == BinOpKind::Eq { '<' } else { '>' }
389-
),
390-
Applicability::HasPlaceholders, // snippet
391-
);
392-
db.span_note(expr.span, "`std::f32::EPSILON` and `std::f64::EPSILON` are available.");
382+
if !is_comparing_arrays {
383+
db.span_suggestion(
384+
expr.span,
385+
"consider comparing them within some error",
386+
format!(
387+
"({}).abs() {} error",
388+
lhs - rhs,
389+
if op == BinOpKind::Eq { '<' } else { '>' }
390+
),
391+
Applicability::HasPlaceholders, // snippet
392+
);
393+
}
394+
db.note("`std::f32::EPSILON` and `std::f64::EPSILON` are available for the `error`");
393395
});
394396
} else if op == BinOpKind::Rem && is_integer_const(cx, right, 1) {
395397
span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0");
@@ -441,6 +443,31 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints {
441443
}
442444
}
443445

446+
fn get_lint_and_message(
447+
is_comparing_constants: bool,
448+
is_comparing_arrays: bool,
449+
) -> (&'static rustc_lint::Lint, &'static str) {
450+
if is_comparing_constants {
451+
(
452+
FLOAT_CMP_CONST,
453+
if is_comparing_arrays {
454+
"strict comparison of `f32` or `f64` constant arrays"
455+
} else {
456+
"strict comparison of `f32` or `f64` constant"
457+
},
458+
)
459+
} else {
460+
(
461+
FLOAT_CMP,
462+
if is_comparing_arrays {
463+
"strict comparison of `f32` or `f64` arrays"
464+
} else {
465+
"strict comparison of `f32` or `f64`"
466+
},
467+
)
468+
}
469+
}
470+
444471
fn check_nan(cx: &LateContext<'_, '_>, expr: &Expr<'_>, cmp_expr: &Expr<'_>) {
445472
if_chain! {
446473
if !in_constant(cx, cmp_expr.hir_id);
@@ -476,6 +503,11 @@ fn is_allowed<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) -> boo
476503
match constant(cx, cx.tables, expr) {
477504
Some((Constant::F32(f), _)) => f == 0.0 || f.is_infinite(),
478505
Some((Constant::F64(f), _)) => f == 0.0 || f.is_infinite(),
506+
Some((Constant::Vec(vec), _)) => vec.iter().all(|f| match f {
507+
Constant::F32(f) => *f == 0.0 || (*f).is_infinite(),
508+
Constant::F64(f) => *f == 0.0 || (*f).is_infinite(),
509+
_ => false,
510+
}),
479511
_ => false,
480512
}
481513
}
@@ -500,7 +532,17 @@ fn is_signum(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
500532
}
501533

502534
fn is_float(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
503-
matches!(walk_ptrs_ty(cx.tables.expr_ty(expr)).kind, ty::Float(_))
535+
let value = &walk_ptrs_ty(cx.tables.expr_ty(expr)).kind;
536+
537+
if let ty::Array(arr_ty, _) = value {
538+
return matches!(arr_ty.kind, ty::Float(_));
539+
};
540+
541+
matches!(value, ty::Float(_))
542+
}
543+
544+
fn is_array(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
545+
matches!(&walk_ptrs_ty(cx.tables.expr_ty(expr)).kind, ty::Array(_, _))
504546
}
505547

506548
fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) {

‎tests/ui/float_cmp.rs‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
#![warn(clippy::float_cmp)]
2-
#![allow(unused, clippy::no_effect, clippy::unnecessary_operation, clippy::cast_lossless)]
2+
#![allow(
3+
unused,
4+
clippy::no_effect,
5+
clippy::unnecessary_operation,
6+
clippy::cast_lossless,
7+
clippy::many_single_char_names
8+
)]
39

410
use std::ops::Add;
511

@@ -77,6 +83,21 @@ fn main() {
7783

7884
assert_eq!(a, b); // no errors
7985

86+
const ZERO_ARRAY: [f32; 2] = [0.0, 0.0];
87+
const NON_ZERO_ARRAY: [f32; 2] = [0.0, 0.1];
88+
89+
let i = 0;
90+
let j = 1;
91+
92+
ZERO_ARRAY[i] == NON_ZERO_ARRAY[j]; // ok, because lhs is zero regardless of i
93+
NON_ZERO_ARRAY[i] == NON_ZERO_ARRAY[j];
94+
95+
let a1: [f32; 1] = [0.0];
96+
let a2: [f32; 1] = [1.1];
97+
98+
a1 == a2;
99+
a1[0] == a2[0];
100+
80101
// no errors - comparing signums is ok
81102
let x32 = 3.21f32;
82103
1.23f32.signum() == x32.signum();

‎tests/ui/float_cmp.stderr‎

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,51 @@
11
error: strict comparison of `f32` or `f64`
2-
--> $DIR/float_cmp.rs:59:5
2+
--> $DIR/float_cmp.rs:65:5
33
|
44
LL | ONE as f64 != 2.0;
55
| ^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(ONE as f64 - 2.0).abs() > error`
66
|
77
= note: `-D clippy::float-cmp` implied by `-D warnings`
8-
note: `std::f32::EPSILON` and `std::f64::EPSILON` are available.
9-
--> $DIR/float_cmp.rs:59:5
10-
|
11-
LL | ONE as f64 != 2.0;
12-
| ^^^^^^^^^^^^^^^^^
8+
= note: `std::f32::EPSILON` and `std::f64::EPSILON` are available for the `error`
139

1410
error: strict comparison of `f32` or `f64`
15-
--> $DIR/float_cmp.rs:64:5
11+
--> $DIR/float_cmp.rs:70:5
1612
|
1713
LL | x == 1.0;
1814
| ^^^^^^^^ help: consider comparing them within some error: `(x - 1.0).abs() < error`
1915
|
20-
note: `std::f32::EPSILON` and `std::f64::EPSILON` are available.
21-
--> $DIR/float_cmp.rs:64:5
22-
|
23-
LL | x == 1.0;
24-
| ^^^^^^^^
16+
= note: `std::f32::EPSILON` and `std::f64::EPSILON` are available for the `error`
2517

2618
error: strict comparison of `f32` or `f64`
27-
--> $DIR/float_cmp.rs:67:5
19+
--> $DIR/float_cmp.rs:73:5
2820
|
2921
LL | twice(x) != twice(ONE as f64);
3022
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(twice(x) - twice(ONE as f64)).abs() > error`
3123
|
32-
note: `std::f32::EPSILON` and `std::f64::EPSILON` are available.
33-
--> $DIR/float_cmp.rs:67:5
24+
= note: `std::f32::EPSILON` and `std::f64::EPSILON` are available for the `error`
25+
26+
error: strict comparison of `f32` or `f64`
27+
--> $DIR/float_cmp.rs:93:5
3428
|
35-
LL | twice(x) != twice(ONE as f64);
36-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
29+
LL | NON_ZERO_ARRAY[i] == NON_ZERO_ARRAY[j];
30+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(NON_ZERO_ARRAY[i] - NON_ZERO_ARRAY[j]).abs() < error`
31+
|
32+
= note: `std::f32::EPSILON` and `std::f64::EPSILON` are available for the `error`
33+
34+
error: strict comparison of `f32` or `f64` arrays
35+
--> $DIR/float_cmp.rs:98:5
36+
|
37+
LL | a1 == a2;
38+
| ^^^^^^^^
39+
|
40+
= note: `std::f32::EPSILON` and `std::f64::EPSILON` are available for the `error`
41+
42+
error: strict comparison of `f32` or `f64`
43+
--> $DIR/float_cmp.rs:99:5
44+
|
45+
LL | a1[0] == a2[0];
46+
| ^^^^^^^^^^^^^^ help: consider comparing them within some error: `(a1[0] - a2[0]).abs() < error`
47+
|
48+
= note: `std::f32::EPSILON` and `std::f64::EPSILON` are available for the `error`
3749

38-
error: aborting due to 3 previous errors
50+
error: aborting due to 6 previous errors
3951

‎tests/ui/float_cmp_const.rs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,4 +46,17 @@ fn main() {
4646
v != w;
4747
v == 1.0;
4848
v != 1.0;
49+
50+
const ZERO_ARRAY: [f32; 3] = [0.0, 0.0, 0.0];
51+
const ZERO_INF_ARRAY: [f32; 3] = [0.0, ::std::f32::INFINITY, ::std::f32::NEG_INFINITY];
52+
const NON_ZERO_ARRAY: [f32; 3] = [0.0, 0.1, 0.2];
53+
const NON_ZERO_ARRAY2: [f32; 3] = [0.2, 0.1, 0.0];
54+
55+
// no errors, zero and infinity values
56+
NON_ZERO_ARRAY[0] == NON_ZERO_ARRAY2[1]; // lhs is 0.0
57+
ZERO_ARRAY == NON_ZERO_ARRAY; // lhs is all zeros
58+
ZERO_INF_ARRAY == NON_ZERO_ARRAY; // lhs is all zeros or infinities
59+
60+
// has errors
61+
NON_ZERO_ARRAY == NON_ZERO_ARRAY2;
4962
}

0 commit comments

Comments
 (0)