float_cmp: Ignore comparisons to detect if an operation changes the value.

Author: Jarcho

clippy_config/src/conf.rs

@@ -616,6 +616,18 @@ define_Conf! {
     /// }
     /// ```
     (float_cmp_ignore_constant_comparisons: bool = true),
+    /// Lint: FLOAT_CMP
+    ///
+    /// Whether to ignore comparisons which check if an operation changes the value of it's operand.
+    ///
+    /// #### Example
+    /// ```no_run
+    /// fn f(x: f64) -> bool {
+    ///     // Will warn if the config is `false`
+    ///     x == x + 1.0
+    /// }
+    /// ```
+    (float_cmp_ignore_change_detection: bool = true),
 }
 
 /// Search for the configuration file.

clippy_lints/src/lib.rs

@@ -582,6 +582,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
         allow_comparison_to_zero,
         float_cmp_ignore_named_constants,
         float_cmp_ignore_constant_comparisons,
+        float_cmp_ignore_change_detection,
 
         blacklisted_names: _,
         cyclomatic_complexity_threshold: _,
@@ -986,6 +987,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
             allow_comparison_to_zero,
             float_cmp_ignore_named_constants,
             float_cmp_ignore_constant_comparisons,
+            float_cmp_ignore_change_detection,
         ))
     });
     store.register_late_pass(|_| Box::<std_instead_of_core::StdReexports>::default());

clippy_lints/src/operators/float_cmp.rs

@@ -1,12 +1,13 @@
 use clippy_utils::consts::{constant, Constant};
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::sugg::Sugg;
-use clippy_utils::visitors::is_const_evaluatable;
-use clippy_utils::{get_item_name, is_expr_named_const, peel_hir_expr_while};
+use clippy_utils::visitors::{for_each_expr, is_const_evaluatable};
+use clippy_utils::{get_item_name, is_expr_named_const, peel_hir_expr_while, SpanlessEq};
+use core::ops::ControlFlow;
 use rustc_errors::Applicability;
-use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, UnOp};
+use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, UnOp, Unsafety};
 use rustc_lint::LateContext;
-use rustc_middle::ty;
+use rustc_middle::ty::{self, Ty, TypeFlags, TypeVisitableExt};
 
 use super::{FloatCmpConfig, FLOAT_CMP};
 
@@ -24,33 +25,40 @@ pub(crate) fn check<'tcx>(
     };
 
     if matches!(op, BinOpKind::Eq | BinOpKind::Ne)
-        && let left = peel_hir_expr_while(left, peel_expr)
-        && let right = peel_hir_expr_while(right, peel_expr)
-        && is_float(cx, left)
+        && let left_red = peel_hir_expr_while(left, peel_expr)
+        && let right_red = peel_hir_expr_while(right, peel_expr)
+        && is_float(cx, left_red)
         // Don't lint literal comparisons
-        && !(matches!(left.kind, ExprKind::Lit(_)) && matches!(right.kind, ExprKind::Lit(_)))
+        && !(matches!(left_red.kind, ExprKind::Lit(_)) && matches!(right_red.kind, ExprKind::Lit(_)))
         // Allow comparing the results of signum()
-        && !(is_signum(cx, left) && is_signum(cx, right))
+        && !(is_signum(cx, left_red) && is_signum(cx, right_red))
     {
-        let left_c = constant(cx, cx.typeck_results(), left);
+        let left_c = constant(cx, cx.typeck_results(), left_red);
         let is_left_const = left_c.is_some();
         if left_c.is_some_and(|c| is_allowed(&c)) {
             return;
         }
-        let right_c = constant(cx, cx.typeck_results(), right);
+        let right_c = constant(cx, cx.typeck_results(), right_red);
         let is_right_const = right_c.is_some();
         if right_c.is_some_and(|c| is_allowed(&c)) {
             return;
         }
 
         if config.ignore_constant_comparisons
-            && (is_left_const || is_const_evaluatable(cx, left))
-            && (is_right_const || is_const_evaluatable(cx, right))
+            && (is_left_const || is_const_evaluatable(cx, left_red))
+            && (is_right_const || is_const_evaluatable(cx, right_red))
         {
             return;
         }
 
-        if config.ignore_named_constants && (is_expr_named_const(cx, left) || is_expr_named_const(cx, right)) {
+        if config.ignore_named_constants && (is_expr_named_const(cx, left_red) || is_expr_named_const(cx, right_red)) {
+            return;
+        }
+
+        if config.ignore_change_detection
+            && ((is_pure_expr(cx, left_red) && contains_expr(cx, right, left))
+                || (is_pure_expr(cx, right_red) && contains_expr(cx, left, right)))
+        {
             return;
         }
 
@@ -60,7 +68,7 @@ pub(crate) fn check<'tcx>(
                 return;
             }
         }
-        let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
+        let is_comparing_arrays = is_array(cx, left_red) || is_array(cx, right_red);
         let msg = if is_comparing_arrays {
             "strict comparison of `f32` or `f64` arrays"
         } else {
@@ -105,6 +113,78 @@ fn is_allowed(val: &Constant<'_>) -> bool {
     }
 }
 
+// This is a best effort guess and may have false positives and negatives.
+fn is_pure_expr<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
+    match e.kind {
+        ExprKind::Path(_) | ExprKind::Lit(_) => true,
+        ExprKind::Field(e, _) | ExprKind::Cast(e, _) | ExprKind::Repeat(e, _) => is_pure_expr(cx, e),
+        ExprKind::Tup(args) => args.iter().all(|arg| is_pure_expr(cx, arg)),
+        ExprKind::Struct(_, fields, base) => {
+            base.map_or(true, |base| is_pure_expr(cx, base)) && fields.iter().all(|f| is_pure_expr(cx, f.expr))
+        },
+
+        // Since rust doesn't actually have the concept of a pure function we
+        // have to guess whether it's likely pure from the signature of the
+        // function.
+        ExprKind::Unary(_, e) => is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(e)) && is_pure_expr(cx, e),
+        ExprKind::Binary(_, x, y) | ExprKind::Index(x, y, _) => {
+            is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(x))
+                && is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(y))
+                && is_pure_expr(cx, x)
+                && is_pure_expr(cx, y)
+        },
+        ExprKind::MethodCall(_, recv, args, _) => {
+            is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(recv))
+                && is_pure_expr(cx, recv)
+                && cx.typeck_results().type_dependent_def_id(e.hir_id).is_some_and(|did| {
+                    matches!(
+                        cx.tcx.fn_sig(did).skip_binder().skip_binder().unsafety,
+                        Unsafety::Normal
+                    )
+                })
+                && args
+                    .iter()
+                    .all(|arg| is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(arg)) && is_pure_expr(cx, arg))
+        },
+        ExprKind::Call(f, args @ [_, ..]) => {
+            is_pure_expr(cx, f)
+                && is_safe_fn(cx, f)
+                && args
+                    .iter()
+                    .all(|arg| is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(arg)) && is_pure_expr(cx, arg))
+        },
+
+        _ => false,
+    }
+}
+
+fn is_safe_fn<'tcx>(cx: &LateContext<'tcx>, e: &Expr<'tcx>) -> bool {
+    let sig = match *cx.typeck_results().expr_ty(e).kind() {
+        ty::FnDef(did, _) => cx.tcx.fn_sig(did).skip_binder(),
+        ty::FnPtr(sig) => sig,
+        _ => return true,
+    };
+    matches!(sig.skip_binder().unsafety, Unsafety::Normal)
+}
+
+fn is_pure_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    !ty.is_mutable_ptr()
+        && ty.is_copy_modulo_regions(cx.tcx, cx.param_env)
+        && (ty.peel_refs().is_freeze(cx.tcx, cx.param_env)
+            || !ty.has_type_flags(TypeFlags::HAS_FREE_REGIONS | TypeFlags::HAS_RE_ERASED | TypeFlags::HAS_RE_BOUND))
+}
+
+fn contains_expr<'tcx>(cx: &LateContext<'tcx>, corpus: &'tcx Expr<'tcx>, e: &'tcx Expr<'tcx>) -> bool {
+    for_each_expr(corpus, |corpus| {
+        if SpanlessEq::new(cx).eq_expr(corpus, e) {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
+        }
+    })
+    .is_some()
+}
+
 // Return true if `expr` is the result of `signum()` invoked on a float value.
 fn is_signum(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind

clippy_lints/src/operators/mod.rs

@@ -735,9 +735,10 @@ declare_clippy_lint! {
 
 #[expect(clippy::struct_field_names)]
 #[derive(Clone, Copy)]
-pub struct FloatCmpConfig {
-    pub ignore_named_constants: bool,
-    pub ignore_constant_comparisons: bool,
+struct FloatCmpConfig {
+    ignore_named_constants: bool,
+    ignore_constant_comparisons: bool,
+    ignore_change_detection: bool,
 }
 
 pub struct Operators {
@@ -779,6 +780,7 @@ impl Operators {
         modulo_arithmetic_allow_comparison_to_zero: bool,
         ignore_named_constants: bool,
         ignore_constant_comparisons: bool,
+        ignore_change_detection: bool,
     ) -> Self {
         Self {
             arithmetic_context: numeric_arithmetic::Context::default(),
@@ -787,6 +789,7 @@ impl Operators {
             float_cmp_config: FloatCmpConfig {
                 ignore_named_constants,
                 ignore_constant_comparisons,
+                ignore_change_detection,
             },
         }
     }

tests/ui-toml/float_cmp_change_detection/clippy.toml

@@ -0,0 +1 @@
+float-cmp-ignore-change-detection = false

tests/ui-toml/float_cmp_change_detection/test.rs

@@ -0,0 +1,29 @@
+//@no-rustfix
+
+#![deny(clippy::float_cmp)]
+
+fn main() {
+    {
+        const C: f64 = 1.0;
+        fn f(x: f64) {
+            let _ = x == C;
+        }
+    }
+    {
+        const fn f(x: f64) -> f64 {
+            todo!()
+        }
+        let _ = f(1.0) == f(2.0);
+    }
+    {
+        let _ = 1.0f32 == 2.0f32;
+        let _ = -1.0f32 == -2.0f32;
+        let _ = 1.0f64 == 2.0f64;
+    }
+    {
+        fn f(x: f32) {
+            let _ = x + 1.0 == x;
+            let _ = x == x + 1.0;
+        }
+    }
+}

tests/ui-toml/float_cmp_change_detection/test.stderr

@@ -0,0 +1,20 @@
+error: strict comparison of `f32` or `f64`
+  --> $DIR/test.rs:25:21
+   |
+LL |             let _ = x + 1.0 == x;
+   |                     ^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(x + 1.0 - x).abs() < error_margin`
+   |
+note: the lint level is defined here
+  --> $DIR/test.rs:3:9
+   |
+LL | #![deny(clippy::float_cmp)]
+   |         ^^^^^^^^^^^^^^^^^
+
+error: strict comparison of `f32` or `f64`
+  --> $DIR/test.rs:26:21
+   |
+LL |             let _ = x == x + 1.0;
+   |                     ^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(x - (x + 1.0)).abs() < error_margin`
+
+error: aborting due to 2 previous errors
+

tests/ui-toml/float_cmp_constant_comparisons/test.rs

@@ -20,4 +20,10 @@ fn main() {
         let _ = -1.0f32 == -2.0f32;
         let _ = 1.0f64 == 2.0f64;
     }
+    {
+        fn f(x: f32) {
+            let _ = x + 1.0 == x;
+            let _ = x == x + 1.0;
+        }
+    }
 }

tests/ui-toml/float_cmp_named_constants/test.rs

@@ -20,4 +20,10 @@ fn main() {
         let _ = -1.0f32 == -2.0f32;
         let _ = 1.0f64 == 2.0f64;
     }
+    {
+        fn f(x: f32) {
+            let _ = x + 1.0 == x;
+            let _ = x == x + 1.0;
+        }
+    }
 }

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -38,6 +38,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
            enum-variant-name-threshold
            enum-variant-size-threshold
            excessive-nesting-threshold
+           float-cmp-ignore-change-detection
            float-cmp-ignore-constant-comparisons
            float-cmp-ignore-named-constants
            future-size-threshold
@@ -119,6 +120,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
            enum-variant-name-threshold
            enum-variant-size-threshold
            excessive-nesting-threshold
+           float-cmp-ignore-change-detection
            float-cmp-ignore-constant-comparisons
            float-cmp-ignore-named-constants
            future-size-threshold

tests/ui/float_cmp.rs

@@ -270,4 +270,64 @@ fn main() {
             let _ = x == y;
         }
     }
+
+    // modified operands
+    {
+        fn f1(x: f32) -> f32 {
+            x + 1.0
+        }
+
+        fn f2(x: f32, y: f32) -> f32 {
+            x + y
+        }
+
+        fn _f(x: f32, y: f32) {
+            let _ = x == x + 1.0;
+            let _ = x + 1.0 == x;
+            let _ = -x == -x + 1.0;
+            let _ = -x + 1.0 == -x;
+            let _ = x == f1(x);
+            let _ = f1(x) == x;
+            let _ = x == f2(x, y);
+            let _ = f2(x, y) == x;
+            let _ = f1(f1(x)) == f1(x);
+            let _ = f1(x) == f1(f1(x));
+
+            let z = (x, y);
+            let _ = z.0 == z.0 + 1.0;
+            let _ = z.0 + 1.0 == z.0;
+        }
+
+        fn _f2(x: &f32) {
+            let _ = *x + 1.0 == *x;
+            let _ = *x == *x + 1.0;
+            let _ = *x == f1(*x);
+            let _ = f1(*x) == *x;
+        }
+    }
+    {
+        fn _f(mut x: impl Iterator<Item = f32>) {
+            let _ = x.next().unwrap() == x.next().unwrap() + 1.0;
+            //~^ ERROR: strict comparison of `f32` or `f64`
+        }
+    }
+    {
+        use core::cell::RefCell;
+
+        struct S(RefCell<f32>);
+        impl S {
+            fn f(&self) -> f32 {
+                let x = *self.0.borrow();
+                *self.0.borrow_mut() *= 2.0;
+                x
+            }
+        }
+
+        fn _f(x: S) {
+            let _ = x.f() + 1.0 == x.f();
+            //~^ ERROR: strict comparison of `f32` or `f64`
+            let _ = x.f() == x.f() + 1.0;
+            //~^ ERROR: strict comparison of `f32` or `f64`
+        }
+    }
 }