Auto 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.

Author: bors

clippy_lints/src/consts.rs

@@ -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,
     }

clippy_lints/src/misc.rs

@@ -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<'_>) {

tests/ui/float_cmp.rs

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

tests/ui/float_cmp.stderr

@@ -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
 

tests/ui/float_cmp_const.rs

@@ -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;
 }