Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lint for float in array comparison #5345

Merged
merged 15 commits into from
Apr 15, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions clippy_lints/src/consts.rs
Original file line number Diff line number Diff line change
@@ -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,
}
@@ -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() {
@@ -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,
}
76 changes: 59 additions & 17 deletions clippy_lints/src/misc.rs
Original file line number Diff line number Diff line change
@@ -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");
@@ -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);
@@ -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,
}
}
@@ -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<'_>) {
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;

@@ -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();
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
@@ -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