Auto merge of #11948 - Jarcho:float_cmp, r=xFrednet

`float_cmp` changes

fixes #2834
fixes #6816

---

changelog: This:
* Deprecated `float_cmp_const` in favor of a config option on [`float_cmp`].
  [#11948](#11948)
* [`float_cmp`]: Don't lint literal and self comparisons.
  [#11948](#11948)
* [`float_cmp`] [#11948](#11948)
    * Add the [`float-cmp-ignore-named-constants`] configuration to ignore comparisons to named constants.
    * Add the [`float-cmp-ignore-change-detection`] configuration to ignore const-evaluatabled values.
    * Add the [`float-cmp-ignore-constant-comparisons`] configuration to ignore comparisons to the modification of an operand (e.g. `x == f(x)`).

Author: bors

clippy_config/src/conf.rs

@@ -645,6 +645,47 @@ define_Conf! {
     ///
     /// Whether to also emit warnings for unsafe blocks with metavariable expansions in **private** macros.
     (warn_unsafe_macro_metavars_in_private_macros: bool = false),
+    /// Lint: FLOAT_CMP
+    ///
+    /// Whether to ignore comparisons to a named constnat
+    ///
+    /// #### Example
+    /// ```no_run
+    /// const VALUE: f64 = 1.0;
+    /// fn is_value(x: f64) -> bool {
+    ///     // Will warn if the config is `false`
+    ///     x == VALUE
+    /// }
+    /// ```
+    (float_cmp_ignore_named_constants: bool = true),
+    /// Lint: FLOAT_CMP
+    ///
+    /// Whether to ignore comparisons which have a constant result.
+    ///
+    /// #### Example
+    /// ```no_run
+    /// const fn f(x: f64) -> f64 {
+    ///     todo!()
+    /// }
+    ///
+    /// // Will warn if the config is `false`
+    /// if f(1.0) == f(2.0) {
+    ///     // ...
+    /// }
+    /// ```
+    (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/declared_lints.rs

@@ -570,7 +570,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::operators::ERASING_OP_INFO,
     crate::operators::FLOAT_ARITHMETIC_INFO,
     crate::operators::FLOAT_CMP_INFO,
-    crate::operators::FLOAT_CMP_CONST_INFO,
     crate::operators::FLOAT_EQUALITY_WITHOUT_ABS_INFO,
     crate::operators::IDENTITY_OP_INFO,
     crate::operators::IMPOSSIBLE_COMPARISONS_INFO,

clippy_lints/src/deprecated_lints.rs

@@ -241,3 +241,14 @@ declare_deprecated_lint! {
     pub MISMATCHED_TARGET_OS,
     "this lint has been replaced by `unexpected_cfgs`"
 }
+
+declare_deprecated_lint! {
+    /// ### What it does
+    /// Nothing. This lint has been deprecated.
+    ///
+    /// ### Deprecation reason
+    /// `float_cmp` handles this via config options
+    #[clippy::version = "1.76.0"]
+    pub FLOAT_CMP_CONST,
+    "`float_cmp` handles this via config options"
+}

clippy_lints/src/lib.deprecated.rs

@@ -75,4 +75,8 @@
         "clippy::mismatched_target_os",
         "this lint has been replaced by `unexpected_cfgs`",
     );
+    store.register_removed(
+        "clippy::float_cmp_const",
+        "`float_cmp` handles this via config options",
+    );
 }

clippy_lints/src/lib.rs

@@ -608,6 +608,9 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
         allow_comparison_to_zero,
         ref allowed_prefixes,
         ref allow_renamed_params_for,
+        float_cmp_ignore_named_constants,
+        float_cmp_ignore_constant_comparisons,
+        float_cmp_ignore_change_detection,
 
         blacklisted_names: _,
         cyclomatic_complexity_threshold: _,
@@ -1031,6 +1034,9 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
         Box::new(operators::Operators::new(
             verbose_bit_mask_threshold,
             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,47 +1,87 @@
-use clippy_utils::consts::{constant_with_source, Constant};
+use clippy_utils::consts::{constant, Constant};
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::get_item_name;
 use clippy_utils::sugg::Sugg;
+use clippy_utils::visitors::{for_each_expr_without_closures, is_const_evaluatable};
+use clippy_utils::{get_item_name, is_expr_named_const, path_res, peel_hir_expr_while, SpanlessEq};
+use core::ops::ControlFlow;
 use rustc_errors::Applicability;
-use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
+use rustc_hir::def::Res;
+use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, Safety, UnOp};
 use rustc_lint::LateContext;
-use rustc_middle::ty;
+use rustc_middle::ty::{self, Ty, TypeFlags, TypeVisitableExt};
 
-use super::{FLOAT_CMP, FLOAT_CMP_CONST};
+use super::{FloatCmpConfig, FLOAT_CMP};
 
 pub(crate) fn check<'tcx>(
     cx: &LateContext<'tcx>,
+    config: FloatCmpConfig,
     expr: &'tcx Expr<'_>,
     op: BinOpKind,
     left: &'tcx Expr<'_>,
     right: &'tcx Expr<'_>,
 ) {
-    if (op == BinOpKind::Eq || op == BinOpKind::Ne) && is_float(cx, left) {
-        let left_is_local = match constant_with_source(cx, cx.typeck_results(), left) {
-            Some((c, s)) if !is_allowed(&c) => s.is_local(),
-            Some(_) => return,
-            None => true,
-        };
-        let right_is_local = match constant_with_source(cx, cx.typeck_results(), right) {
-            Some((c, s)) if !is_allowed(&c) => s.is_local(),
-            Some(_) => return,
-            None => true,
-        };
+    let peel_expr = |e: &'tcx Expr<'tcx>| match e.kind {
+        ExprKind::Cast(e, _) | ExprKind::AddrOf(BorrowKind::Ref, _, e) | ExprKind::Unary(UnOp::Neg, e) => Some(e),
+        _ => None,
+    };
 
+    if matches!(op, BinOpKind::Eq | BinOpKind::Ne)
+        && let left_reduced = peel_hir_expr_while(left, peel_expr)
+        && let right_reduced = peel_hir_expr_while(right, peel_expr)
+        && is_float(cx, left_reduced)
+        // Don't lint literal comparisons
+        && !(matches!(left_reduced.kind, ExprKind::Lit(_)) && matches!(right_reduced.kind, ExprKind::Lit(_)))
         // Allow comparing the results of signum()
-        if is_signum(cx, left) && is_signum(cx, right) {
+        && !(is_signum(cx, left_reduced) && is_signum(cx, right_reduced))
+        && match (path_res(cx, left_reduced), path_res(cx, right_reduced)) {
+            (Res::Err, _) | (_, Res::Err) => true,
+            (left, right) => left != right,
+        }
+    {
+        let left_c = constant(cx, cx.typeck_results(), left_reduced);
+        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_reduced);
+        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_reduced))
+            && (is_right_const || is_const_evaluatable(cx, right_reduced))
+        {
+            return;
+        }
+
+        if config.ignore_named_constants
+            && (is_expr_named_const(cx, left_reduced) || is_expr_named_const(cx, right_reduced))
+        {
+            return;
+        }
+
+        if config.ignore_change_detection
+            && ((is_pure_expr(cx, left_reduced) && contains_expr(cx, right, left))
+                || (is_pure_expr(cx, right_reduced) && contains_expr(cx, left, right)))
+        {
             return;
         }
 
         if let Some(name) = get_item_name(cx, expr) {
             let name = name.as_str();
-            if name == "eq" || name == "ne" || name == "is_nan" || name.starts_with("eq_") || name.ends_with("_eq") {
+            if name == "eq" || name == "ne" || name.starts_with("eq_") || name.ends_with("_eq") {
                 return;
             }
         }
-        let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
-        let (lint, msg) = get_lint_and_message(left_is_local && right_is_local, is_comparing_arrays);
-        span_lint_and_then(cx, lint, expr.span, msg, |diag| {
+        let is_comparing_arrays = is_array(cx, left_reduced) || is_array(cx, right_reduced);
+        let msg = if is_comparing_arrays {
+            "strict comparison of `f32` or `f64` arrays"
+        } else {
+            "strict comparison of `f32` or `f64`"
+        };
+        span_lint_and_then(cx, FLOAT_CMP, expr.span, msg, |diag| {
             let lhs = Sugg::hir(cx, left, "..");
             let rhs = Sugg::hir(cx, right, "..");
 
@@ -61,54 +101,105 @@ pub(crate) fn check<'tcx>(
     }
 }
 
-fn get_lint_and_message(is_local: bool, is_comparing_arrays: bool) -> (&'static rustc_lint::Lint, &'static str) {
-    if is_local {
-        (
-            FLOAT_CMP,
-            if is_comparing_arrays {
-                "strict comparison of `f32` or `f64` arrays"
-            } else {
-                "strict comparison of `f32` or `f64`"
-            },
-        )
-    } else {
-        (
-            FLOAT_CMP_CONST,
-            if is_comparing_arrays {
-                "strict comparison of `f32` or `f64` constant arrays"
-            } else {
-                "strict comparison of `f32` or `f64` constant"
-            },
-        )
-    }
-}
-
 fn is_allowed(val: &Constant<'_>) -> bool {
     match val {
         // FIXME(f16_f128): add when equality check is available on all platforms
+        Constant::Ref(val) => is_allowed(val),
         &Constant::F32(f) => f == 0.0 || f.is_infinite(),
         &Constant::F64(f) => f == 0.0 || f.is_infinite(),
-        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(),
+        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,
         }),
+        Constant::Repeat(val, _) => match **val {
+            Constant::F32(f) => f == 0.0 || f.is_infinite(),
+            Constant::F64(f) => f == 0.0 || f.is_infinite(),
+            _ => false,
+        },
         _ => false,
     }
 }
 
-// Return true if `expr` is the result of `signum()` invoked on a float value.
-fn is_signum(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    // The negation of a signum is still a signum
-    if let ExprKind::Unary(UnOp::Neg, child_expr) = expr.kind {
-        return is_signum(cx, child_expr);
+// 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().safety, Safety::Safe))
+                && 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_pure_fn_ty(cx, cx.typeck_results().expr_ty_adjusted(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_pure_fn_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    let sig = match *ty.peel_refs().kind() {
+        ty::FnDef(did, _) => cx.tcx.fn_sig(did).skip_binder(),
+        ty::FnPtr(sig) => sig,
+        ty::Closure(_, args) => {
+            return args.as_closure().upvar_tys().iter().all(|ty| is_pure_arg_ty(cx, ty));
+        },
+        _ => return false,
+    };
+    matches!(sig.skip_binder().safety, Safety::Safe)
+}
 
+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_without_closures(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
         && sym!(signum) == method_name.ident.name
-    // Check that the receiver of the signum() is a float (expressions[0] is the receiver of
-    // the method call)
     {
+        // Check that the receiver of the signum() is a float
         return is_float(cx, self_arg);
     }
     false