Skip to content

Commit f616153

Browse files
JarchoJarcho
committed
float_cmp: Ignore comparisons to detect if an operation changes the value.
1 parent 0128ed7 commit f616153

File tree

12 files changed

+274
-19
lines changed

12 files changed

+274
-19
lines changed

‎clippy_config/src/conf.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -669,6 +669,18 @@ define_Conf! {
669669
/// }
670670
/// ```
671671
(float_cmp_ignore_constant_comparisons: bool = true),
672+
/// Lint: FLOAT_CMP
673+
///
674+
/// Whether to ignore comparisons which check if an operation changes the value of it's operand.
675+
///
676+
/// #### Example
677+
/// ```no_run
678+
/// fn f(x: f64) -> bool {
679+
/// // Will warn if the config is `false`
680+
/// x == x + 1.0
681+
/// }
682+
/// ```
683+
(float_cmp_ignore_change_detection: bool = true),
672684
}
673685

674686
/// Search for the configuration file.

‎clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,6 +602,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
602602
ref allow_renamed_params_for,
603603
float_cmp_ignore_named_constants,
604604
float_cmp_ignore_constant_comparisons,
605+
float_cmp_ignore_change_detection,
605606

606607
blacklisted_names: _,
607608
cyclomatic_complexity_threshold: _,
@@ -1029,6 +1030,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
10291030
allow_comparison_to_zero,
10301031
float_cmp_ignore_named_constants,
10311032
float_cmp_ignore_constant_comparisons,
1033+
float_cmp_ignore_change_detection,
10321034
))
10331035
});
10341036
store.register_late_pass(|_| Box::<std_instead_of_core::StdReexports>::default());

‎clippy_lints/src/operators/float_cmp.rs

Lines changed: 96 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
use clippy_utils::consts::{constant, Constant};
22
use clippy_utils::diagnostics::span_lint_and_then;
33
use clippy_utils::sugg::Sugg;
4-
use clippy_utils::visitors::is_const_evaluatable;
5-
use clippy_utils::{get_item_name, is_expr_named_const, peel_hir_expr_while};
4+
use clippy_utils::visitors::{for_each_expr_without_closures, is_const_evaluatable};
5+
use clippy_utils::{get_item_name, is_expr_named_const, peel_hir_expr_while, SpanlessEq};
6+
use core::ops::ControlFlow;
67
use rustc_errors::Applicability;
7-
use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, UnOp};
8+
use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, Safety, UnOp};
89
use rustc_lint::LateContext;
9-
use rustc_middle::ty;
10+
use rustc_middle::ty::{self, Ty, TypeFlags, TypeVisitableExt};
1011

1112
use super::{FloatCmpConfig, FLOAT_CMP};
1213

@@ -24,33 +25,40 @@ pub(crate) fn check<'tcx>(
2425
};
2526

2627
if matches!(op, BinOpKind::Eq | BinOpKind::Ne)
27-
&& let left = peel_hir_expr_while(left, peel_expr)
28-
&& let right = peel_hir_expr_while(right, peel_expr)
29-
&& is_float(cx, left)
28+
&& let left_reduced = peel_hir_expr_while(left, peel_expr)
29+
&& let right_reduced = peel_hir_expr_while(right, peel_expr)
30+
&& is_float(cx, left_reduced)
3031
// Don't lint literal comparisons
31-
&& !(matches!(left.kind, ExprKind::Lit(_)) && matches!(right.kind, ExprKind::Lit(_)))
32+
&& !(matches!(left_reduced.kind, ExprKind::Lit(_)) && matches!(right_reduced.kind, ExprKind::Lit(_)))
3233
// Allow comparing the results of signum()
33-
&& !(is_signum(cx, left) && is_signum(cx, right))
34+
&& !(is_signum(cx, left_reduced) && is_signum(cx, right_reduced))
3435
{
35-
let left_c = constant(cx, cx.typeck_results(), left);
36+
let left_c = constant(cx, cx.typeck_results(), left_reduced);
3637
let is_left_const = left_c.is_some();
3738
if left_c.is_some_and(|c| is_allowed(&c)) {
3839
return;
3940
}
40-
let right_c = constant(cx, cx.typeck_results(), right);
41+
let right_c = constant(cx, cx.typeck_results(), right_reduced);
4142
let is_right_const = right_c.is_some();
4243
if right_c.is_some_and(|c| is_allowed(&c)) {
4344
return;
4445
}
4546

4647
if config.ignore_constant_comparisons
47-
&& (is_left_const || is_const_evaluatable(cx, left))
48-
&& (is_right_const || is_const_evaluatable(cx, right))
48+
&& (is_left_const || is_const_evaluatable(cx, left_reduced))
49+
&& (is_right_const || is_const_evaluatable(cx, right_reduced))
4950
{
5051
return;
5152
}
5253

53-
if config.ignore_named_constants && (is_expr_named_const(cx, left) || is_expr_named_const(cx, right)) {
54+
if config.ignore_named_constants && (is_expr_named_const(cx, left_reduced) || is_expr_named_const(cx, right_reduced)) {
55+
return;
56+
}
57+
58+
if config.ignore_change_detection
59+
&& ((is_pure_expr(cx, left_reduced) && contains_expr(cx, right, left))
60+
|| (is_pure_expr(cx, right_reduced) && contains_expr(cx, left, right)))
61+
{
5462
return;
5563
}
5664

@@ -60,7 +68,7 @@ pub(crate) fn check<'tcx>(
6068
return;
6169
}
6270
}
63-
let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
71+
let is_comparing_arrays = is_array(cx, left_reduced) || is_array(cx, right_reduced);
6472
let msg = if is_comparing_arrays {
6573
"strict comparison of `f32` or `f64` arrays"
6674
} else {
@@ -105,6 +113,79 @@ fn is_allowed(val: &Constant<'_>) -> bool {
105113
}
106114
}
107115

116+
// This is a best effort guess and may have false positives and negatives.
117+
fn is_pure_expr<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
118+
match e.kind {
119+
ExprKind::Path(_) | ExprKind::Lit(_) => true,
120+
ExprKind::Field(e, _) | ExprKind::Cast(e, _) | ExprKind::Repeat(e, _) => is_pure_expr(cx, e),
121+
ExprKind::Tup(args) => args.iter().all(|arg| is_pure_expr(cx, arg)),
122+
ExprKind::Struct(_, fields, base) => {
123+
base.map_or(true, |base| is_pure_expr(cx, base)) && fields.iter().all(|f| is_pure_expr(cx, f.expr))
124+
},
125+
126+
// Since rust doesn't actually have the concept of a pure function we
127+
// have to guess whether it's likely pure from the signature of the
128+
// function.
129+
ExprKind::Unary(_, e) => is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(e)) && is_pure_expr(cx, e),
130+
ExprKind::Binary(_, x, y) | ExprKind::Index(x, y, _) => {
131+
is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(x))
132+
&& is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(y))
133+
&& is_pure_expr(cx, x)
134+
&& is_pure_expr(cx, y)
135+
},
136+
ExprKind::MethodCall(_, recv, args, _) => {
137+
is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(recv))
138+
&& is_pure_expr(cx, recv)
139+
&& cx
140+
.typeck_results()
141+
.type_dependent_def_id(e.hir_id)
142+
.is_some_and(|did| matches!(cx.tcx.fn_sig(did).skip_binder().skip_binder().safety, Safety::Safe))
143+
&& args
144+
.iter()
145+
.all(|arg| is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(arg)) && is_pure_expr(cx, arg))
146+
},
147+
ExprKind::Call(f, args @ [_, ..]) => {
148+
is_pure_expr(cx, f)
149+
&& is_pure_fn_ty(cx, cx.typeck_results().expr_ty_adjusted(f))
150+
&& args
151+
.iter()
152+
.all(|arg| is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(arg)) && is_pure_expr(cx, arg))
153+
},
154+
155+
_ => false,
156+
}
157+
}
158+
159+
fn is_pure_fn_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
160+
let sig = match *ty.peel_refs().kind() {
161+
ty::FnDef(did, _) => cx.tcx.fn_sig(did).skip_binder(),
162+
ty::FnPtr(sig) => sig,
163+
ty::Closure(_, args) => {
164+
return args.as_closure().upvar_tys().iter().all(|ty| is_pure_arg_ty(cx, ty));
165+
},
166+
_ => return false,
167+
};
168+
matches!(sig.skip_binder().safety, Safety::Safe)
169+
}
170+
171+
fn is_pure_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
172+
!ty.is_mutable_ptr()
173+
&& ty.is_copy_modulo_regions(cx.tcx, cx.param_env)
174+
&& (ty.peel_refs().is_freeze(cx.tcx, cx.param_env)
175+
|| !ty.has_type_flags(TypeFlags::HAS_FREE_REGIONS | TypeFlags::HAS_RE_ERASED | TypeFlags::HAS_RE_BOUND))
176+
}
177+
178+
fn contains_expr<'tcx>(cx: &LateContext<'tcx>, corpus: &'tcx Expr<'tcx>, e: &'tcx Expr<'tcx>) -> bool {
179+
for_each_expr_without_closures(corpus, |corpus| {
180+
if SpanlessEq::new(cx).eq_expr(corpus, e) {
181+
ControlFlow::Break(())
182+
} else {
183+
ControlFlow::Continue(())
184+
}
185+
})
186+
.is_some()
187+
}
188+
108189
// Return true if `expr` is the result of `signum()` invoked on a float value.
109190
fn is_signum(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
110191
if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind

‎clippy_lints/src/operators/mod.rs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -734,9 +734,10 @@ declare_clippy_lint! {
734734
}
735735

736736
#[derive(Clone, Copy)]
737-
pub struct FloatCmpConfig {
738-
pub ignore_named_constants: bool,
739-
pub ignore_constant_comparisons: bool,
737+
struct FloatCmpConfig {
738+
ignore_named_constants: bool,
739+
ignore_constant_comparisons: bool,
740+
ignore_change_detection: bool,
740741
}
741742

742743
pub struct Operators {
@@ -773,11 +774,13 @@ impl_lint_pass!(Operators => [
773774
SELF_ASSIGNMENT,
774775
]);
775776
impl Operators {
777+
#[expect(clippy::fn_params_excessive_bools)]
776778
pub fn new(
777779
verbose_bit_mask_threshold: u64,
778780
modulo_arithmetic_allow_comparison_to_zero: bool,
779781
ignore_named_constants: bool,
780782
ignore_constant_comparisons: bool,
783+
ignore_change_detection: bool,
781784
) -> Self {
782785
Self {
783786
arithmetic_context: numeric_arithmetic::Context::default(),
@@ -786,6 +789,7 @@ impl Operators {
786789
float_cmp_config: FloatCmpConfig {
787790
ignore_named_constants,
788791
ignore_constant_comparisons,
792+
ignore_change_detection,
789793
},
790794
}
791795
}

‎tests/ui-toml/float_cmp_change_detection/clippy.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
float-cmp-ignore-change-detection = false

‎tests/ui-toml/float_cmp_change_detection/test.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
//@no-rustfix
2+
3+
#![deny(clippy::float_cmp)]
4+
5+
fn main() {
6+
{
7+
const C: f64 = 1.0;
8+
fn f(x: f64) {
9+
let _ = x == C;
10+
}
11+
}
12+
{
13+
const fn f(x: f64) -> f64 {
14+
todo!()
15+
}
16+
let _ = f(1.0) == f(2.0);
17+
}
18+
{
19+
let _ = 1.0f32 == 2.0f32;
20+
let _ = -1.0f32 == -2.0f32;
21+
let _ = 1.0f64 == 2.0f64;
22+
}
23+
{
24+
fn f(x: f32) {
25+
let _ = x + 1.0 == x;
26+
let _ = x == x + 1.0;
27+
}
28+
}
29+
}

‎tests/ui-toml/float_cmp_change_detection/test.stderr

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
error: strict comparison of `f32` or `f64`
2+
--> tests/ui-toml/float_cmp_change_detection/test.rs:25:21
3+
|
4+
LL | let _ = x + 1.0 == x;
5+
| ^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(x + 1.0 - x).abs() < error_margin`
6+
|
7+
note: the lint level is defined here
8+
--> tests/ui-toml/float_cmp_change_detection/test.rs:3:9
9+
|
10+
LL | #![deny(clippy::float_cmp)]
11+
| ^^^^^^^^^^^^^^^^^
12+
13+
error: strict comparison of `f32` or `f64`
14+
--> tests/ui-toml/float_cmp_change_detection/test.rs:26:21
15+
|
16+
LL | let _ = x == x + 1.0;
17+
| ^^^^^^^^^^^^ help: consider comparing them within some margin of error: `(x - (x + 1.0)).abs() < error_margin`
18+
19+
error: aborting due to 2 previous errors
20+

‎tests/ui-toml/float_cmp_constant_comparisons/test.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,10 @@ fn main() {
2020
let _ = -1.0f32 == -2.0f32;
2121
let _ = 1.0f64 == 2.0f64;
2222
}
23+
{
24+
fn f(x: f32) {
25+
let _ = x + 1.0 == x;
26+
let _ = x == x + 1.0;
27+
}
28+
}
2329
}

‎tests/ui-toml/float_cmp_named_constants/test.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,10 @@ fn main() {
2020
let _ = -1.0f32 == -2.0f32;
2121
let _ = 1.0f64 == 2.0f64;
2222
}
23+
{
24+
fn f(x: f32) {
25+
let _ = x + 1.0 == x;
26+
let _ = x == x + 1.0;
27+
}
28+
}
2329
}

‎tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
4242
enum-variant-name-threshold
4343
enum-variant-size-threshold
4444
excessive-nesting-threshold
45+
float-cmp-ignore-change-detection
4546
float-cmp-ignore-constant-comparisons
4647
float-cmp-ignore-named-constants
4748
future-size-threshold
@@ -128,6 +129,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
128129
enum-variant-name-threshold
129130
enum-variant-size-threshold
130131
excessive-nesting-threshold
132+
float-cmp-ignore-change-detection
131133
float-cmp-ignore-constant-comparisons
132134
float-cmp-ignore-named-constants
133135
future-size-threshold

0 commit comments

Comments
 (0)