Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

skip float_cmp check if lhs is a custom type #11385

Merged
merged 1 commit into from Aug 28, 2023
Merged

skip float_cmp check if lhs is a custom type #11385

merged 1 commit into from Aug 28, 2023

Conversation

markhuang1212
Copy link
Contributor

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [float_cmp]: allow float eq comparison when lhs is a custom type that implements PartialEq<f32/f64>

If the lhs of a comparison is not float, it means there is a user implemented PartialEq, and the caller is invoking that custom version of ==, instead of the default floating point equal comparison.

People may wrap f32 with a struct (say MyF32) and implement its PartialEq that will do the is_close() check, so that MyF32 can be compared with either f32 or MyF32.

@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 23, 2023
@blyxyas
Copy link
Member

blyxyas commented Aug 23, 2023

This would fail if the lhs doesn't implement PartialEq, so we should use the implements_trait function if the lhs implements it.

You can check this example for reference, we should also add a test where the lhs doesn't implement PartialEq<f32|f64> to check that it doesn't lint.

@markhuang1212
Copy link
Contributor Author

Not sure I understand this correctly, we already checked that lhs is a float before we enter the lint, if lhs doesn't implement PartialEq, nothing should happen. Before this pr lint will be performed if either lhs or rhs is float

@blyxyas
Copy link
Member

blyxyas commented Aug 24, 2023

Oh I get it now! If the second value doesn't implement PartialEq, it will have a compiler error before linting. Right?

@markhuang1212
Copy link
Contributor Author

Oh I get it now! If the second value doesn't implement PartialEq, it will have a compiler error before linting. Right?

Yes!

blyxyas
blyxyas approved these changes Aug 25, 2023
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think this is good then ❤️.
Could you squash these 2 commits into a single one?

@blyxyas
Copy link
Member

blyxyas commented Aug 26, 2023

meow 🐱

@bors
Copy link
Collaborator

bors commented Aug 26, 2023

📌 Commit e43c234 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 26, 2023

⌛ Testing commit e43c234 with merge 2920d04...

bors added a commit that referenced this pull request Aug 26, 2023
@bors
Auto merge of #11385 - markhuang1212:master, r=blyxyas
2920d04 
allow float_cmp with lhs is a custom type

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`float_cmp`]: allow float eq comparison when lhs is a custom type that implements PartialEq<f32/f64>

If the lhs of a comparison is not float, it means there is a user implemented PartialEq, and the caller is invoking that custom version of `==`, instead of the default floating point equal comparison.

People may wrap f32 with a struct (say `MyF32`) and implement its PartialEq that will do the `is_close()` check, so that `MyF32` can be compared with either f32 or `MyF32`.
@blyxyas
Copy link
Member

blyxyas commented Aug 26, 2023 •
edited

@bors r-

I just noticed, this should account for cases where the custom type isn't on the right side, but also if it only on the left side. And that is exactly what the original code did.

It checked if either the lhs or the rhs are floats, it isn't a &&, it's a ||. I'm not sure if this PR really adds functionality, as the current version of the lint already takes into account that case.

@bors
Copy link
Collaborator

bors commented Aug 26, 2023

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 2920d04 (2920d04a9be2173aaf768bea15e10374bf887d0c)

@markhuang1212 markhuang1212 changed the title allow float_cmp with lhs is a custom type skip float_cmp check if lhs is a custom type Aug 26, 2023
@markhuang1212
Copy link
Contributor Author

@blyxyas I think this pr might caused some confusion due to a previous typo in the title. Anyway, my motivation is this: in a project I'm working on, we use a custom type to wrap f64 to add some functionality, which include a impl PartialEq<f64> that returns true if (a-b).abs() < episolon, and when I'm using it, it triggers a warning. From my understanding, if people is intentionally invoking a custom PartiqlEq, they are trying to acheive something that they have customized, and therefore should not be warned against. The rationale of this change is somewhat similar to the existing feature where the check is not enforced if the function name containes eq.

@blyxyas
Copy link
Member

blyxyas commented Aug 27, 2023

I'll do some more testing on this patch. I'm not sure it really is caused because of this if statement. The logic would be the same as the || is inclusive. If is_float(cx, left) is true, then it doesn't matter the result is_float(cx, right). idk 🤷‍♀️

@blyxyas
Copy link
Member

blyxyas commented Aug 28, 2023

Mmmmmm... I just checked it, it does change the logic. I find it pretty weird that this inclusive operator changes the outcome in exclusive-like behaviours.

LGTM! Thanks ❤️
@bors r+

@bors
Copy link
Collaborator

bors commented Aug 28, 2023

📌 Commit e43c234 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 28, 2023

⌛ Testing commit e43c234 with merge 5cc5f27...

@bors
Copy link
Collaborator

bors commented Aug 28, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 5cc5f27 to master...

@bors bors merged commit 5cc5f27 into rust-lang:master Aug 28, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blyxyas blyxyas blyxyas approved these changes

Assignees

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@markhuang1212 @rustbot @blyxyas @bors