Skip to content

Commit

Permalink
Auto merge of #5345 - Toxyxer:add-lint-for-float-in-array-comparison,…
Browse files Browse the repository at this point in the history
… r=flip1995

Add lint for float in array comparison

Fixes #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.
  • Loading branch information
bors committed Apr 11, 2020
2 parents 97aa8dc + 4449cc7 commit 2e6ff6e
Show file tree
Hide file tree
Showing 6 changed files with 200 additions and 71 deletions.
61 changes: 61 additions & 0 deletions clippy_lints/src/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
}
}
},
ExprKind::Index(ref arr, ref index) => self.index(arr, index),
// TODO: add other expressions.
_ => None,
}
Expand Down Expand Up @@ -345,6 +346,31 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
}
}

fn index(&mut self, lhs: &'_ Expr<'_>, index: &'_ Expr<'_>) -> Option<Constant> {
let lhs = self.expr(lhs);
let index = self.expr(index);

match (lhs, index) {
(Some(Constant::Vec(vec)), Some(Constant::Int(index))) => match vec[index as usize] {
Constant::F32(x) => Some(Constant::F32(x)),
Constant::F64(x) => Some(Constant::F64(x)),
_ => None,
},
(Some(Constant::Vec(vec)), _) => {
if !vec.is_empty() && vec.iter().all(|x| *x == vec[0]) {
match vec[0] {
Constant::F32(x) => Some(Constant::F32(x)),
Constant::F64(x) => Some(Constant::F64(x)),
_ => None,
}
} else {
None
}
},
_ => None,
}
}

/// A block can only yield a constant if it only has one constant expression.
fn block(&mut self, block: &Block<'_>) -> Option<Constant> {
if block.stmts.is_empty() {
Expand Down Expand Up @@ -492,6 +518,41 @@ pub fn miri_to_const(result: &ty::Const<'_>) -> Option<Constant> {
},
_ => None,
},
ty::ConstKind::Value(ConstValue::ByRef { alloc, offset: _ }) => match result.ty.kind {
ty::Array(sub_type, len) => match sub_type.kind {
ty::Float(FloatTy::F32) => match miri_to_const(len) {
Some(Constant::Int(len)) => alloc
.inspect_with_undef_and_ptr_outside_interpreter(0..(4 * len as usize))
.to_owned()
.chunks(4)
.map(|chunk| {
Some(Constant::F32(f32::from_le_bytes(
chunk.try_into().expect("this shouldn't happen"),
)))
})
.collect::<Option<Vec<Constant>>>()
.map(Constant::Vec),
_ => None,
},
ty::Float(FloatTy::F64) => match miri_to_const(len) {
Some(Constant::Int(len)) => alloc
.inspect_with_undef_and_ptr_outside_interpreter(0..(8 * len as usize))
.to_owned()
.chunks(8)
.map(|chunk| {
Some(Constant::F64(f64::from_le_bytes(
chunk.try_into().expect("this shouldn't happen"),
)))
})
.collect::<Option<Vec<Constant>>>()
.map(Constant::Vec),
_ => None,
},
// FIXME: implement other array type conversions.
_ => None,
},
_ => None,
},
// FIXME: implement other conversions.
_ => None,
}
Expand Down
76 changes: 59 additions & 17 deletions clippy_lints/src/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,26 +369,28 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints {
return;
}
}
let (lint, msg) = if is_named_constant(cx, left) || is_named_constant(cx, right) {
(FLOAT_CMP_CONST, "strict comparison of `f32` or `f64` constant")
} else {
(FLOAT_CMP, "strict comparison of `f32` or `f64`")
};
let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
let (lint, msg) = get_lint_and_message(
is_named_constant(cx, left) || is_named_constant(cx, right),
is_comparing_arrays,
);
span_lint_and_then(cx, lint, expr.span, msg, |db| {
let lhs = Sugg::hir(cx, left, "..");
let rhs = Sugg::hir(cx, right, "..");

db.span_suggestion(
expr.span,
"consider comparing them within some error",
format!(
"({}).abs() {} error",
lhs - rhs,
if op == BinOpKind::Eq { '<' } else { '>' }
),
Applicability::HasPlaceholders, // snippet
);
db.span_note(expr.span, "`f32::EPSILON` and `f64::EPSILON` are available.");
if !is_comparing_arrays {
db.span_suggestion(
expr.span,
"consider comparing them within some error",
format!(
"({}).abs() {} error",
lhs - rhs,
if op == BinOpKind::Eq { '<' } else { '>' }
),
Applicability::HasPlaceholders, // snippet
);
}
db.note("`f32::EPSILON` and `f64::EPSILON` are available for the `error`");
});
} else if op == BinOpKind::Rem && is_integer_const(cx, right, 1) {
span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0");
Expand Down Expand Up @@ -440,6 +442,31 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints {
}
}

fn get_lint_and_message(
is_comparing_constants: bool,
is_comparing_arrays: bool,
) -> (&'static rustc_lint::Lint, &'static str) {
if is_comparing_constants {
(
FLOAT_CMP_CONST,
if is_comparing_arrays {
"strict comparison of `f32` or `f64` constant arrays"
} else {
"strict comparison of `f32` or `f64` constant"
},
)
} else {
(
FLOAT_CMP,
if is_comparing_arrays {
"strict comparison of `f32` or `f64` arrays"
} else {
"strict comparison of `f32` or `f64`"
},
)
}
}

fn check_nan(cx: &LateContext<'_, '_>, expr: &Expr<'_>, cmp_expr: &Expr<'_>) {
if_chain! {
if !in_constant(cx, cmp_expr.hir_id);
Expand Down Expand Up @@ -475,6 +502,11 @@ fn is_allowed<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) -> boo
match constant(cx, cx.tables, expr) {
Some((Constant::F32(f), _)) => f == 0.0 || f.is_infinite(),
Some((Constant::F64(f), _)) => f == 0.0 || f.is_infinite(),
Some((Constant::Vec(vec), _)) => vec.iter().all(|f| match f {
Constant::F32(f) => *f == 0.0 || (*f).is_infinite(),
Constant::F64(f) => *f == 0.0 || (*f).is_infinite(),
_ => false,
}),
_ => false,
}
}
Expand All @@ -499,7 +531,17 @@ fn is_signum(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
}

fn is_float(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
matches!(walk_ptrs_ty(cx.tables.expr_ty(expr)).kind, ty::Float(_))
let value = &walk_ptrs_ty(cx.tables.expr_ty(expr)).kind;

if let ty::Array(arr_ty, _) = value {
return matches!(arr_ty.kind, ty::Float(_));
};

matches!(value, ty::Float(_))
}

fn is_array(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
matches!(&walk_ptrs_ty(cx.tables.expr_ty(expr)).kind, ty::Array(_, _))
}

fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) {
Expand Down
23 changes: 22 additions & 1 deletion tests/ui/float_cmp.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
#![warn(clippy::float_cmp)]
#![allow(unused, clippy::no_effect, clippy::unnecessary_operation, clippy::cast_lossless)]
#![allow(
unused,
clippy::no_effect,
clippy::unnecessary_operation,
clippy::cast_lossless,
clippy::many_single_char_names
)]

use std::ops::Add;

Expand Down Expand Up @@ -77,6 +83,21 @@ fn main() {

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

const ZERO_ARRAY: [f32; 2] = [0.0, 0.0];
const NON_ZERO_ARRAY: [f32; 2] = [0.0, 0.1];

let i = 0;
let j = 1;

ZERO_ARRAY[i] == NON_ZERO_ARRAY[j]; // ok, because lhs is zero regardless of i
NON_ZERO_ARRAY[i] == NON_ZERO_ARRAY[j];

let a1: [f32; 1] = [0.0];
let a2: [f32; 1] = [1.1];

a1 == a2;
a1[0] == a2[0];

// no errors - comparing signums is ok
let x32 = 3.21f32;
1.23f32.signum() == x32.signum();
Expand Down
48 changes: 30 additions & 18 deletions tests/ui/float_cmp.stderr
Original file line number Diff line number Diff line change
@@ -1,39 +1,51 @@
error: strict comparison of `f32` or `f64`
--> $DIR/float_cmp.rs:59:5
--> $DIR/float_cmp.rs:65:5
|
LL | ONE as f64 != 2.0;
| ^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(ONE as f64 - 2.0).abs() > error`
|
= note: `-D clippy::float-cmp` implied by `-D warnings`
note: `f32::EPSILON` and `f64::EPSILON` are available.
--> $DIR/float_cmp.rs:59:5
|
LL | ONE as f64 != 2.0;
| ^^^^^^^^^^^^^^^^^
= note: `f32::EPSILON` and `f64::EPSILON` are available for the `error`

error: strict comparison of `f32` or `f64`
--> $DIR/float_cmp.rs:64:5
--> $DIR/float_cmp.rs:70:5
|
LL | x == 1.0;
| ^^^^^^^^ help: consider comparing them within some error: `(x - 1.0).abs() < error`
|
note: `f32::EPSILON` and `f64::EPSILON` are available.
--> $DIR/float_cmp.rs:64:5
|
LL | x == 1.0;
| ^^^^^^^^
= note: `f32::EPSILON` and `f64::EPSILON` are available for the `error`

error: strict comparison of `f32` or `f64`
--> $DIR/float_cmp.rs:67:5
--> $DIR/float_cmp.rs:73:5
|
LL | twice(x) != twice(ONE as f64);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(twice(x) - twice(ONE as f64)).abs() > error`
|
note: `f32::EPSILON` and `f64::EPSILON` are available.
--> $DIR/float_cmp.rs:67:5
= note: `f32::EPSILON` and `f64::EPSILON` are available for the `error`

error: strict comparison of `f32` or `f64`
--> $DIR/float_cmp.rs:93:5
|
LL | twice(x) != twice(ONE as f64);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | NON_ZERO_ARRAY[i] == NON_ZERO_ARRAY[j];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(NON_ZERO_ARRAY[i] - NON_ZERO_ARRAY[j]).abs() < error`
|
= note: `f32::EPSILON` and `f64::EPSILON` are available for the `error`

error: strict comparison of `f32` or `f64` arrays
--> $DIR/float_cmp.rs:98:5
|
LL | a1 == a2;
| ^^^^^^^^
|
= note: `f32::EPSILON` and `f64::EPSILON` are available for the `error`

error: strict comparison of `f32` or `f64`
--> $DIR/float_cmp.rs:99:5
|
LL | a1[0] == a2[0];
| ^^^^^^^^^^^^^^ help: consider comparing them within some error: `(a1[0] - a2[0]).abs() < error`
|
= note: `f32::EPSILON` and `f64::EPSILON` are available for the `error`

error: aborting due to 3 previous errors
error: aborting due to 6 previous errors

13 changes: 13 additions & 0 deletions tests/ui/float_cmp_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,17 @@ fn main() {
v != w;
v == 1.0;
v != 1.0;

const ZERO_ARRAY: [f32; 3] = [0.0, 0.0, 0.0];
const ZERO_INF_ARRAY: [f32; 3] = [0.0, ::std::f32::INFINITY, ::std::f32::NEG_INFINITY];
const NON_ZERO_ARRAY: [f32; 3] = [0.0, 0.1, 0.2];
const NON_ZERO_ARRAY2: [f32; 3] = [0.2, 0.1, 0.0];

// no errors, zero and infinity values
NON_ZERO_ARRAY[0] == NON_ZERO_ARRAY2[1]; // lhs is 0.0
ZERO_ARRAY == NON_ZERO_ARRAY; // lhs is all zeros
ZERO_INF_ARRAY == NON_ZERO_ARRAY; // lhs is all zeros or infinities

// has errors
NON_ZERO_ARRAY == NON_ZERO_ARRAY2;
}
Loading

0 comments on commit 2e6ff6e

Please sign in to comment.