From 9039f5019bdccbe5b41d6e8a85de578e08325802 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 8 Dec 2023 14:36:15 -0500 Subject: [PATCH] `float_cmp`: Ignore comparisons to detect if an operation changes the value. --- clippy_config/src/conf.rs | 12 ++ clippy_lints/src/lib.rs | 2 + clippy_lints/src/operators/float_cmp.rs | 110 +++++++++++++++--- clippy_lints/src/operators/mod.rs | 9 +- .../float_cmp_change_detection/clippy.toml | 1 + .../float_cmp_change_detection/test.rs | 29 +++++ .../float_cmp_change_detection/test.stderr | 20 ++++ .../float_cmp_constant_comparisons/test.rs | 6 + .../ui-toml/float_cmp_named_constants/test.rs | 6 + .../toml_unknown_key/conf_unknown_key.stderr | 2 + tests/ui/float_cmp.rs | 60 ++++++++++ tests/ui/float_cmp.stderr | 20 +++- 12 files changed, 258 insertions(+), 19 deletions(-) create mode 100644 tests/ui-toml/float_cmp_change_detection/clippy.toml create mode 100644 tests/ui-toml/float_cmp_change_detection/test.rs create mode 100644 tests/ui-toml/float_cmp_change_detection/test.stderr diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index eb7f88493cff..7bf23e890bd1 100644 --- a/clippy_config/src/conf.rs +++ b/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. diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ddc076b83b37..2f85475876a5 100644 --- a/clippy_lints/src/lib.rs +++ b/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::::default()); diff --git a/clippy_lints/src/operators/float_cmp.rs b/clippy_lints/src/operators/float_cmp.rs index 058e2ff2d4ae..a123636ba0c8 100644 --- a/clippy_lints/src/operators/float_cmp.rs +++ b/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 diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 3834d4586f37..94ce4b0a89aa 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/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, }, } } diff --git a/tests/ui-toml/float_cmp_change_detection/clippy.toml b/tests/ui-toml/float_cmp_change_detection/clippy.toml new file mode 100644 index 000000000000..c78367975726 --- /dev/null +++ b/tests/ui-toml/float_cmp_change_detection/clippy.toml @@ -0,0 +1 @@ +float-cmp-ignore-change-detection = false diff --git a/tests/ui-toml/float_cmp_change_detection/test.rs b/tests/ui-toml/float_cmp_change_detection/test.rs new file mode 100644 index 000000000000..65f6b9edecc3 --- /dev/null +++ b/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; + } + } +} diff --git a/tests/ui-toml/float_cmp_change_detection/test.stderr b/tests/ui-toml/float_cmp_change_detection/test.stderr new file mode 100644 index 000000000000..6ee387416288 --- /dev/null +++ b/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 + diff --git a/tests/ui-toml/float_cmp_constant_comparisons/test.rs b/tests/ui-toml/float_cmp_constant_comparisons/test.rs index bbd33eb0f660..65f6b9edecc3 100644 --- a/tests/ui-toml/float_cmp_constant_comparisons/test.rs +++ b/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; + } + } } diff --git a/tests/ui-toml/float_cmp_named_constants/test.rs b/tests/ui-toml/float_cmp_named_constants/test.rs index bbd33eb0f660..65f6b9edecc3 100644 --- a/tests/ui-toml/float_cmp_named_constants/test.rs +++ b/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; + } + } } diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index e165b910dd20..0bc6dca2f0c0 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/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 diff --git a/tests/ui/float_cmp.rs b/tests/ui/float_cmp.rs index 3e69151730f3..9798a20565c2 100644 --- a/tests/ui/float_cmp.rs +++ b/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) { + let _ = x.next().unwrap() == x.next().unwrap() + 1.0; + //~^ ERROR: strict comparison of `f32` or `f64` + } + } + { + use core::cell::RefCell; + + struct S(RefCell); + 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` + } + } } diff --git a/tests/ui/float_cmp.stderr b/tests/ui/float_cmp.stderr index a83cd28b3ad2..684c83f4719d 100644 --- a/tests/ui/float_cmp.stderr +++ b/tests/ui/float_cmp.stderr @@ -163,5 +163,23 @@ error: strict comparison of `f32` or `f64` LL | let _ = C[1] == x; | ^^^^^^^^^ help: consider comparing them within some margin of error: `(C[1] - x).abs() < error_margin` -error: aborting due to 27 previous errors +error: strict comparison of `f32` or `f64` + --> $DIR/float_cmp.rs:310:21 + | +LL | let _ = x.next().unwrap() == x.next().unwrap() + 1.0; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(x.next().unwrap() - (x.next().unwrap() + 1.0)).abs() < error_margin` + +error: strict comparison of `f32` or `f64` + --> $DIR/float_cmp.rs:327:21 + | +LL | let _ = x.f() + 1.0 == x.f(); + | ^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(x.f() + 1.0 - x.f()).abs() < error_margin` + +error: strict comparison of `f32` or `f64` + --> $DIR/float_cmp.rs:329:21 + | +LL | let _ = x.f() == x.f() + 1.0; + | ^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(x.f() - (x.f() + 1.0)).abs() < error_margin` + +error: aborting due to 30 previous errors