Skip to content

Commit cd6fc09

Browse files
JarchoJarcho
committed
float_cmp: Ignore comparisons to detect if an operation changes the value.
1 parent 9d77810 commit cd6fc09

File tree

11 files changed

+267
-17
lines changed

11 files changed

+267
-17
lines changed

‎clippy_config/src/conf.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -674,6 +674,18 @@ define_Conf! {
674674
/// }
675675
/// ```
676676
(float_cmp_ignore_constant_comparisons: bool = true),
677+
/// Lint: FLOAT_CMP
678+
///
679+
/// Whether to ignore comparisons which check if an operation changes the value of it's operand.
680+
///
681+
/// #### Example
682+
/// ```no_run
683+
/// fn f(x: f64) -> bool {
684+
/// // Will warn if the config is `false`
685+
/// x == x + 1.0
686+
/// }
687+
/// ```
688+
(float_cmp_ignore_change_detection: bool = true),
677689
}
678690

679691
/// Search for the configuration file.

‎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 {
@@ -106,6 +114,79 @@ fn is_allowed(val: &Constant<'_>) -> bool {
106114
}
107115
}
108116

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

‎clippy_lints/src/operators/mod.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -778,6 +778,7 @@ declare_clippy_lint! {
778778
struct FloatCmpConfig {
779779
ignore_named_constants: bool,
780780
ignore_constant_comparisons: bool,
781+
ignore_change_detection: bool,
781782
}
782783

783784
pub struct Operators {
@@ -795,6 +796,7 @@ impl Operators {
795796
float_cmp_config: FloatCmpConfig {
796797
ignore_named_constants: conf.float_cmp_ignore_named_constants,
797798
ignore_constant_comparisons: conf.float_cmp_ignore_constant_comparisons,
799+
ignore_change_detection: conf.float_cmp_ignore_change_detection,
798800
},
799801
}
800802
}
@@ -827,7 +829,6 @@ impl_lint_pass!(Operators => [
827829
PTR_EQ,
828830
SELF_ASSIGNMENT,
829831
]);
830-
831832
impl<'tcx> LateLintPass<'tcx> for Operators {
832833
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
833834
eq_op::check_assert(cx, e);

‎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

‎tests/ui/float_cmp.rs‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,4 +272,72 @@ fn main() {
272272
let _ = x == y;
273273
}
274274
}
275+
276+
// modified operands
277+
{
278+
fn f1(x: f32) -> f32 {
279+
x + 1.0
280+
}
281+
282+
fn f2(x: f32, y: f32) -> f32 {
283+
x + y
284+
}
285+
286+
fn _f(x: f32, y: f32) {
287+
let _ = x == x + 1.0;
288+
let _ = x + 1.0 == x;
289+
let _ = -x == -x + 1.0;
290+
let _ = -x + 1.0 == -x;
291+
let _ = x == f1(x);
292+
let _ = f1(x) == x;
293+
let _ = x == f2(x, y);
294+
let _ = f2(x, y) == x;
295+
let _ = f1(f1(x)) == f1(x);
296+
let _ = f1(x) == f1(f1(x));
297+
298+
let z = (x, y);
299+
let _ = z.0 == z.0 + 1.0;
300+
let _ = z.0 + 1.0 == z.0;
301+
}
302+
303+
fn _f2(x: &f32) {
304+
let _ = *x + 1.0 == *x;
305+
let _ = *x == *x + 1.0;
306+
let _ = *x == f1(*x);
307+
let _ = f1(*x) == *x;
308+
}
309+
}
310+
{
311+
fn _f(mut x: impl Iterator<Item = f32>) {
312+
let _ = x.next().unwrap() == x.next().unwrap() + 1.0;
313+
//~^ ERROR: strict comparison of `f32` or `f64`
314+
}
315+
}
316+
{
317+
use core::cell::RefCell;
318+
319+
struct S(RefCell<f32>);
320+
impl S {
321+
fn f(&self) -> f32 {
322+
let x = *self.0.borrow();
323+
*self.0.borrow_mut() *= 2.0;
324+
x
325+
}
326+
}
327+
328+
fn _f(x: S) {
329+
let _ = x.f() + 1.0 == x.f();
330+
//~^ ERROR: strict comparison of `f32` or `f64`
331+
let _ = x.f() == x.f() + 1.0;
332+
//~^ ERROR: strict comparison of `f32` or `f64`
333+
}
334+
}
335+
{
336+
let f = |x: f32| -> f32 { x };
337+
let _ = f(1.0) == f(1.0) + 1.0;
338+
339+
let mut x = 1.0;
340+
let mut f = |y: f32| -> f32 { core::mem::replace(&mut x, y) };
341+
let _ = f(1.0) == f(1.0) + 1.0; //~ float_cmp
342+
}
275343
}

0 commit comments

Comments
 (0)