Add lint for comparing floats in an array #4343

briankabiro · 2019-08-06T15:04:30Z

Finishes #4277

changelog: none

clippy_lints/src/misc.rs

briankabiro · 2019-08-15T08:10:58Z

clippy_lints/src/misc.rs

@@ -519,7 +519,8 @@ fn is_signum(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
 }

 fn is_float(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
-    matches!(walk_ptrs_ty(cx.tables.expr_ty(expr)).sty, ty::Float(_))
+    let value = &walk_ptrs_ty(cx.tables.expr_ty(expr)).sty;
+    matches!(value, ty::Array(_ty, _)) || matches!(value, ty::Float(_))


Hey, @flip1995. All done with the update. This is what I have now.

You also have to match _ty.node to Float(_):

Suggested change

matches!(value, ty::Array(_ty, _)) || matches!(value, ty::Float(_))

matches!(value, ty::Array(ty, _)) && matches!(ty.node, ty::Float(_)) || matches!(value, ty::Float(_))

You may also want to walk_ptrs_ty(ty), since an Array of &Floats shouldn't be compared either.

Hey @flip1995.

I think I understand what I'm supposed to do.

matches!(value, ty::Array(ty, _)) matches an array

matches!(ty.node, ty::Float(_)) is to match when the value inside the array is a Float.

The issue I'm having right now is getting the second part, checking if it is a Float.

When I match against ty.node, I get this error: expected value, found module ty

This is what I get when print the value of walk_ptrs_ty, when it's an array.

Array(u8, Const { ty: usize, val: Scalar(0x0000000000000004) })

I'm thinking that I have to match against the value inside the Const struct.

You can ignore the Const struct, since it is only the length of the array. The Rust type system already forbids the comparison of two arrays with different lengths, so we don't have to take care about that.

You have to match the u8. Note, that the u8 is only displayed because of the pretty printing of types. In fact it is just another ty::Ty.

On the error message: You get this, because you shadow the import of ty with the variable name ty. Renaming the variable ty with arr_ty or similar should solve this.

Thanks. I will get to work on it.

flip1995 · 2019-08-15T08:45:12Z

tests/ui/eq_op.stderr

+error: strict comparison of f32 or f64
+  --> $DIR/eq_op.rs:25:5
+   |
+LL |     ([1] != [1]);


These are integers and shouldn't trigger the lint. See comment above, on how to fix this.

flip1995 · 2019-08-27T14:31:26Z

clippy_lints/src/misc.rs

-    matches!(walk_ptrs_ty(cx.tables.expr_ty(expr)).sty, ty::Float(_))
+    let value = &walk_ptrs_ty(cx.tables.expr_ty(expr)).sty;
+
+    matches!(value, ty::Array(arr_ty, _)) && matches!(value, ty::Float(_)) || matches!(value, ty::Float(_))


Suggested change

matches!(value, ty::Array(arr_ty, _)) && matches!(value, ty::Float(_)) || matches!(value, ty::Float(_))

matches!(value, ty::Array(arr_ty, _)) && matches!(&walk_ptrs_ty(arr_ty).sty, ty::Float(_)) || matches!(value, ty::Float(_))

Unfortunately, getting an error that:

`arr_ty` is not found in this scope

Could it be an issue with the macro?

In that case you have to write the match yourself. The macro seems to open a new scope for the variables.

Hey. Was caught up this week, didn't get the chance to take a look at this further.

Going to resume next week!

Finishes rust-lang#4277

flip1995

Can you add tests, please?

flip1995 · 2019-09-17T09:20:34Z

clippy_lints/src/misc.rs

+    let value = &walk_ptrs_ty(cx.tables.expr_ty(expr)).sty;
+
+    if let ty::Array(arr_ty, _) = value {
+        return matches!(arr_ty.sty, ty::Float(_));


I think we also have to walk_ptrs_ty on arr_ty, so that Arrays of &floats also get linted.

I'm having a bit of an issue getting the walk_ptrs_ty to work with arr_ty; I have updated the tests in the meanwhile. You can have a look if I'm going in the right direction so far.

flip1995 · 2019-09-20T07:33:43Z

tests/ui/float_cmp.rs

@@ -77,6 +77,13 @@ fn main() {

    assert_eq!(a, b); // no errors

+    let a1: [f32; 1] = [0.0];
+    let a2: [f32; 1] = [1.1];


Could you also add a test for the type [&f32; 1]?

Playground

Thanks for the help.

flip1995 · 2019-09-20T07:40:07Z

tests/ui/float_cmp.rs

+    let a1: [f32; 1] = [0.0];
+    let a2: [f32; 1] = [1.1];
+
+    assert_eq!(a1[0], a2[0]);


If we want to do it right, this should not trigger the lint, since everything in a1 ist constant 0.0, so a comparison is ok here.

On a more general note, this enhancement needs more tests like the base case of comparing floats. (Comparing arrays of constant 0.0, constant {NEG_}INFINITY, ...)

bors · 2019-09-26T09:46:52Z

☔ The latest upstream changes (presumably #4580) made this pull request unmergeable. Please resolve the merge conflicts.

flip1995 · 2020-03-09T13:26:20Z

I'll set this to S-inactive-closed, since someone else want to take up this issue. Thanks @briankabiro for doing the initial work!

jonboh · 2023-10-10T18:52:23Z

@rustbot label -S-inactive-closed

briankabiro commented Aug 6, 2019

View reviewed changes

clippy_lints/src/misc.rs Outdated Show resolved Hide resolved

flip1995 reviewed Aug 9, 2019

View reviewed changes

clippy_lints/src/misc.rs Outdated Show resolved Hide resolved

briankabiro force-pushed the add-lint-for-float-in-array-comparison branch from 99445f9 to 1de416b Compare August 13, 2019 18:45

briankabiro changed the title ~~Add lint when comparing floats in an array~~ Add lint for comparing floats in an array Aug 13, 2019

briankabiro marked this pull request as ready for review August 13, 2019 19:42

flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Aug 14, 2019

briankabiro commented Aug 15, 2019

View reviewed changes

flip1995 reviewed Aug 15, 2019

View reviewed changes

briankabiro force-pushed the add-lint-for-float-in-array-comparison branch from 1de416b to dc53b28 Compare August 27, 2019 14:27

flip1995 reviewed Aug 27, 2019

View reviewed changes

Add lint when comparing floats in an array

11197c5

Finishes rust-lang#4277

briankabiro force-pushed the add-lint-for-float-in-array-comparison branch from dc53b28 to 11197c5 Compare September 17, 2019 09:08

flip1995 requested changes Sep 17, 2019

View reviewed changes

briankabiro added 2 commits September 17, 2019 12:29

Merge branch 'master' into add-lint-for-float-in-array-comparison

1bf0d74

Add tests for float in array comparison

2471f90

flip1995 reviewed Sep 20, 2019

View reviewed changes

flip1995 added A-lint Area: New lints and removed A-lint Area: New lints labels Nov 25, 2019

flip1995 closed this Mar 9, 2020

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 9, 2020

flip1995 mentioned this pull request Mar 9, 2020

Float comparison lint doesn't seem to trigger on float array comparison #4277

Closed

flip1995 mentioned this pull request Mar 30, 2020

Add lint for float in array comparison #5345

Merged

rustbot removed the S-inactive-closed Status: Closed due to inactivity label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint for comparing floats in an array #4343

Add lint for comparing floats in an array #4343

briankabiro commented Aug 6, 2019

briankabiro Aug 15, 2019

flip1995 Aug 15, 2019

flip1995 Aug 15, 2019

briankabiro Aug 22, 2019 •

edited

Loading

flip1995 Aug 26, 2019

briankabiro Aug 27, 2019

flip1995 Aug 15, 2019 •

edited

Loading

flip1995 Aug 27, 2019

briankabiro Aug 27, 2019

flip1995 Aug 27, 2019

briankabiro Sep 6, 2019

flip1995 left a comment

flip1995 Sep 17, 2019

briankabiro Sep 17, 2019 •

edited

Loading

briankabiro Sep 19, 2019

flip1995 Sep 20, 2019 •

edited

Loading

briankabiro Sep 27, 2019

flip1995 Sep 20, 2019

bors commented Sep 26, 2019

flip1995 commented Mar 9, 2020

jonboh commented Oct 10, 2023

	matches!(value, ty::Array(_ty, _)) \|\| matches!(value, ty::Float(_))
	matches!(value, ty::Array(ty, _)) && matches!(ty.node, ty::Float(_)) \|\| matches!(value, ty::Float(_))

	matches!(value, ty::Array(arr_ty, _)) && matches!(value, ty::Float(_)) \|\| matches!(value, ty::Float(_))
	matches!(value, ty::Array(arr_ty, _)) && matches!(&walk_ptrs_ty(arr_ty).sty, ty::Float(_)) \|\| matches!(value, ty::Float(_))

Add lint for comparing floats in an array #4343

Add lint for comparing floats in an array #4343

Conversation

briankabiro commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briankabiro Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briankabiro Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 26, 2019

flip1995 commented Mar 9, 2020

jonboh commented Oct 10, 2023

briankabiro Aug 22, 2019 •

edited

Loading

flip1995 Aug 15, 2019 •

edited

Loading

briankabiro Sep 17, 2019 •

edited

Loading

flip1995 Sep 20, 2019 •

edited

Loading