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

Add new Lint/HashCompareByIdentity cop #8796

Merged
merged 1 commit into from Sep 29, 2020

Conversation

fatkodima
Copy link
Contributor

This implements a second part of https://rubystyle.guide/#identity-comparison

@koic
Copy link
Member

koic commented Sep 25, 2020

Thank you for the implementation. I was also considering this style rule implementation.
I'm not sure if user can always change hash creator which compare_by_identity method should be added. So, It may be simpler to split the roles with another Lint/CompareByIdentity (?) cop. They also have different auto-correction support.

Cc @rubocop-hq/rubocop-core

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

I ❤️ it when a cop improves our own code base 🎉

Even though both cases deal with comparing by identity, I wonder if we shouldn't split this into two cops:

  1. Some people might not like compare_by_identity and want to disable that (but not the use of equal?
  2. The == case is safe (I think 😅), the second case is not quite safe: although unlikely, the hash could store object ids and other stuff that need be compared by values.

Maybe @bbatsov has an opinion on this

Edit: Just read @koic's remark, glad to see we agree about splitting this 😄

return unless receiver && argument
def_node_matcher :id_comparison?, <<~PATTERN
(send
(send $!nil? :object_id) :==
Copy link
Contributor

@marcandre marcandre Sep 25, 2020

Choose a reason for hiding this comment

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

Why the !nil?? We should still replace object_id == other.object_id with equal?(other), and other.object_id == object_id with other.equal?(self) no?


replacement = "#{receiver.source}.equal?(#{argument.source})"
def_node_matcher :id_as_hash_key?, <<~PATTERN
(send _ {:key? :has_key? :fetch :[] :[]=} (send !nil? :object_id) ...)
Copy link
Contributor

@marcandre marcandre Sep 25, 2020

Choose a reason for hiding this comment

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

Same here: hash[object_id] should be an offense

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

I ❤️ it when a cop improves our own code base 🎉

Even though both cases deal with comparing by identity, I wonder if we shouldn't split this into two cops:

  1. Some people might not like compare_by_identity and want to disable that (but not the use of equal?
  2. The == case is safe (I think 😅), the second case is not quite safe: although unlikely, the hash could store object ids and other stuff that need be compared by values.

@fatkodima fatkodima force-pushed the enhance-identity_comparison branch 2 times, most recently from d0eeb56 to 2c456dd Compare September 26, 2020 13:02
@fatkodima
Copy link
Contributor Author

Extracted into a separate cop.

@fatkodima fatkodima changed the title Enhance Lint/IdentityComparison cop with a rule for object_id as hash keys Add new Lint/CompareByIdentity cop Sep 26, 2020
@fatkodima fatkodima changed the title Add new Lint/CompareByIdentity cop Add new Lint/HashCompareByIdentity cop Sep 26, 2020
@bbatsov bbatsov merged commit cb06e4a into rubocop:master Sep 29, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 29, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants