Add config options to ignore constant comparisons and comparisons to constants to float_cmp

Author: Jarcho

clippy_config/src/conf.rs

@@ -587,6 +587,35 @@ define_Conf! {
     /// 2. Paths with any segment that containing the word 'prelude'
     /// are already allowed by default.
     (allowed_wildcard_imports: FxHashSet<String> = FxHashSet::default()),
+    /// 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),
 }
 
 /// Search for the configuration file.

clippy_lints/src/lib.rs

@@ -580,6 +580,8 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
         pub_underscore_fields_behavior,
         ref allowed_duplicate_crates,
         allow_comparison_to_zero,
+        float_cmp_ignore_named_constants,
+        float_cmp_ignore_constant_comparisons,
 
         blacklisted_names: _,
         cyclomatic_complexity_threshold: _,
@@ -982,6 +984,8 @@ 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,
         ))
     });
     store.register_late_pass(|_| Box::<std_instead_of_core::StdReexports>::default());

clippy_lints/src/operators/float_cmp.rs

@@ -1,35 +1,54 @@
-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::is_const_evaluatable;
+use clippy_utils::{get_item_name, is_expr_named_const, peel_hir_expr_while};
 use rustc_errors::Applicability;
-use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
+use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, UnOp};
 use rustc_lint::LateContext;
 use rustc_middle::ty;
 
-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,
-        };
-
+    if (op == BinOpKind::Eq || op == BinOpKind::Ne)
+        && is_float(cx, left)
         // Allow comparing the results of signum()
-        if is_signum(cx, left) && is_signum(cx, right) {
+        && !(is_signum(cx, left) && is_signum(cx, right))
+    {
+        let left_c = constant(cx, cx.typeck_results(), left);
+        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 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))
+        {
+            return;
+        }
+
+        let peel_expr = |e: &'tcx Expr<'tcx>| match e.kind {
+            ExprKind::Cast(e, _) | ExprKind::AddrOf(BorrowKind::Ref, _, e) => Some(e),
+            _ => None,
+        };
+        if config.ignore_named_constants
+            && (is_expr_named_const(cx, peel_hir_expr_while(left, peel_expr))
+                || is_expr_named_const(cx, peel_hir_expr_while(right, peel_expr)))
+        {
             return;
         }
 
@@ -40,8 +59,12 @@ pub(crate) fn check<'tcx>(
             }
         }
         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 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, "..");
 
@@ -62,28 +85,6 @@ 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 {
         Constant::Ref(val) => is_allowed(val),

clippy_lints/src/operators/mod.rs

@@ -768,10 +768,17 @@ declare_clippy_lint! {
     "explicit self-assignment"
 }
 
+#[derive(Clone, Copy)]
+pub struct FloatCmpConfig {
+    pub ignore_named_constants: bool,
+    pub ignore_constant_comparisons: bool,
+}
+
 pub struct Operators {
     arithmetic_context: numeric_arithmetic::Context,
     verbose_bit_mask_threshold: u64,
     modulo_arithmetic_allow_comparison_to_zero: bool,
+    float_cmp_config: FloatCmpConfig,
 }
 impl_lint_pass!(Operators => [
     ABSURD_EXTREME_COMPARISONS,
@@ -802,11 +809,20 @@ impl_lint_pass!(Operators => [
     SELF_ASSIGNMENT,
 ]);
 impl Operators {
-    pub fn new(verbose_bit_mask_threshold: u64, modulo_arithmetic_allow_comparison_to_zero: bool) -> Self {
+    pub fn new(
+        verbose_bit_mask_threshold: u64,
+        modulo_arithmetic_allow_comparison_to_zero: bool,
+        ignore_named_constants: bool,
+        ignore_constant_comparisons: bool,
+    ) -> Self {
         Self {
             arithmetic_context: numeric_arithmetic::Context::default(),
             verbose_bit_mask_threshold,
             modulo_arithmetic_allow_comparison_to_zero,
+            float_cmp_config: FloatCmpConfig {
+                ignore_named_constants,
+                ignore_constant_comparisons,
+            },
         }
     }
 }
@@ -835,7 +851,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators {
                 float_equality_without_abs::check(cx, e, op.node, lhs, rhs);
                 integer_division::check(cx, e, op.node, lhs, rhs);
                 cmp_owned::check(cx, op.node, lhs, rhs);
-                float_cmp::check(cx, e, op.node, lhs, rhs);
+                float_cmp::check(cx, self.float_cmp_config, e, op.node, lhs, rhs);
                 modulo_one::check(cx, e, op.node, rhs);
                 modulo_arithmetic::check(
                     cx,

clippy_utils/src/lib.rs

@@ -200,6 +200,16 @@ pub fn in_constant(cx: &LateContext<'_>, id: HirId) -> bool {
     cx.tcx.hir().is_inside_const_context(id)
 }
 
+/// Checks if the expression is path to either a constant or an associated constant.
+pub fn is_expr_named_const<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
+    matches!(&e.kind, ExprKind::Path(p)
+        if matches!(
+            cx.qpath_res(p, e.hir_id),
+            Res::Def(DefKind::Const | DefKind::AssocConst, _)
+        )
+    )
+}
+
 /// Checks if a `Res` refers to a constructor of a `LangItem`
 /// For example, use this to check whether a function call or a pattern is `Some(..)`.
 pub fn is_res_lang_ctor(cx: &LateContext<'_>, res: Res, lang_item: LangItem) -> bool {

tests/ui-toml/float_cmp_constant_comparisons/clippy.toml

@@ -0,0 +1 @@
+float-cmp-ignore-constant-comparisons = false

tests/ui-toml/float_cmp_constant_comparisons/test.rs

@@ -0,0 +1,18 @@
+//@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);
+    }
+}

tests/ui-toml/float_cmp_constant_comparisons/test.stderr

@@ -0,0 +1,15 @@
+error: strict comparison of `f32` or `f64`
+  --> $DIR/test.rs:16:17
+   |
+LL |         let _ = f(1.0) == f(2.0);
+   |                 ^^^^^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(f(1.0) - f(2.0)).abs() < error_margin`
+   |
+   = note: `f32::EPSILON` and `f64::EPSILON` are available for the `error_margin`
+note: the lint level is defined here
+  --> $DIR/test.rs:3:9
+   |
+LL | #![deny(clippy::float_cmp)]
+   |         ^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+

tests/ui-toml/float_cmp_named_constants/clippy.toml

@@ -0,0 +1 @@
+float-cmp-ignore-named-constants = false

tests/ui-toml/float_cmp_named_constants/test.rs

@@ -0,0 +1,18 @@
+//@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);
+    }
+}