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.

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

Performance/RedundantEqualityComparisonBlock suggest a change that alters the result #217

Closed
jenskdsgn opened this issue Mar 1, 2021 · 2 comments · Fixed by #218 or ministryofjustice/Claim-for-Crown-Court-Defence#3815
Labels
bug Something isn't working

Comments

@jenskdsgn
Copy link

After upgrading to 1.10.0 with the introduction of the new cop Performance/RedundantEqualityComparisonBlock,
I fixed all the offenses that are related to this cop and noticed failing unit tests because the change did not only changed the style of code but the logic as well.

The underlying problem here is that internally when doing .any?(Integer) for example it does evaluate to Integer === block_argument. This is fine and works as expected, however the operation with === is not commutative, meaning block_argument === Integer can have a different output. Now, in an edge case where you want to test if a certain value is any type of a list of classes, the arguments of === are actually reversed. The cop will suggest to simplify, which then results in different behavior (see following example). I actually did not see that coming and only good unit testing saved me from breaking our code.

Furthermore, I think that the suggested change makes the code shorter but also less intuitive.
Let me know how I can help going further.


Given the example offending code:

value = "test"
[String, NilClass].any? { |klass| value.is_a?(klass) }

Expected behavior

Do not suggest to replace .any? { |klass| value.is_a?(klass) } with .any?(value) because the result is different.

Actual behavior

It prints out an offense and auto-fix will result in different result.

Steps to reproduce the problem

see example

RuboCop version

$ rubocop -V
1.10.0 (using Parser 3.0.0.0, rubocop-ast 1.4.1, running on ruby 2.7.2 x86_64-darwin19)
  - rubocop-performance 1.10.0
  - rubocop-rails 2.9.1
  - rubocop-rake 0.5.1
@koic koic added the bug Something isn't working label Mar 1, 2021
@koic
Copy link
Member

koic commented Mar 1, 2021

Thank you for the feedback. This is a false positive and I will fix it.

@jenskdsgn
Copy link
Author

Very cool thank you!

koic added a commit to koic/rubocop-performance that referenced this issue Mar 1, 2021
…lityComparisonBlock`

Fixes rubocop#217.

This PR fixes a false positive for `Performance/RedundantEqualityComparisonBlock`
when using block argument is used for an argument of `is_a`.
koic added a commit to koic/rubocop-performance that referenced this issue Mar 1, 2021
koic added a commit to koic/rubocop-performance that referenced this issue Mar 1, 2021
…lityComparisonBlock`

Fixes rubocop#217.

This PR fixes a false positive for `Performance/RedundantEqualityComparisonBlock`
when using block argument is used for an argument of `is_a`.
koic added a commit to koic/rubocop-performance that referenced this issue Mar 1, 2021
…lityComparisonBlock`

Fixes rubocop#217.

This PR fixes a false positive for `Performance/RedundantEqualityComparisonBlock`
when using block argument is used for an argument of `is_a`.
koic added a commit to koic/rubocop-performance that referenced this issue Mar 1, 2021
…lityComparisonBlock`

Fixes rubocop#217.

This PR fixes a false positive for `Performance/RedundantEqualityComparisonBlock`
when using block argument is used for an argument of `is_a`.
koic added a commit to koic/rubocop-performance that referenced this issue Mar 1, 2021
…lityComparisonBlock`

Fixes rubocop#217.

This PR fixes a false positive for `Performance/RedundantEqualityComparisonBlock`
when using block argument is used for an argument of `is_a`.
@koic koic closed this as completed in #218 Mar 1, 2021
koic added a commit that referenced this issue Mar 1, 2021
…equality_comparison_block

[Fix #217] Fix a false positive for `Performance/RedundantEqualityComparisonBlock`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants