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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### New features

* [#8796](https://github.com/rubocop-hq/rubocop/pull/8796): Add new `Lint/HashCompareByIdentity` cop. ([@fatkodima][])

## 0.92.0 (2020-09-25)

### New features
Expand Down
7 changes: 7 additions & 0 deletions config/default.yml
Expand Up @@ -1546,6 +1546,13 @@ Lint/FormatParameterMismatch:
Enabled: true
VersionAdded: '0.33'

Lint/HashCompareByIdentity:
Description: 'Prefer using `Hash#compare_by_identity` than using `object_id` for keys.'
StyleGuide: '#identity-comparison'
Enabled: pending
Safe: false
VersionAdded: '0.93'

Lint/HeredocMethodCallPosition:
Description: >-
Checks for the ordering of a method call where
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -216,6 +216,7 @@ In the following section you find all available cops:
* xref:cops_lint.adoc#lintfloatcomparison[Lint/FloatComparison]
* xref:cops_lint.adoc#lintfloatoutofrange[Lint/FloatOutOfRange]
* xref:cops_lint.adoc#lintformatparametermismatch[Lint/FormatParameterMismatch]
* xref:cops_lint.adoc#linthashcomparebyidentity[Lint/HashCompareByIdentity]
* xref:cops_lint.adoc#lintheredocmethodcallposition[Lint/HeredocMethodCallPosition]
* xref:cops_lint.adoc#lintidentitycomparison[Lint/IdentityComparison]
* xref:cops_lint.adoc#lintimplicitstringconcatenation[Lint/ImplicitStringConcatenation]
Expand Down
36 changes: 36 additions & 0 deletions docs/modules/ROOT/pages/cops_lint.adoc
Expand Up @@ -1620,6 +1620,42 @@ format('Unnumbered format: %s and numbered: %2$s', a_value, another)
format('Numbered format: %1$s and numbered %2$s', a_value, another)
----

== Lint/HashCompareByIdentity

|===
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Pending
| No
| No
| 0.93
| -
|===

Prefer using `Hash#compare_by_identity` than using `object_id` for hash keys.

This cop is marked as unsafe as a hash possibly can contain other keys
besides `object_id`s.

=== Examples

[source,ruby]
----
# bad
hash = {}
hash[foo.object_id] = :bar
hash.key?(baz.object_id)
# good
hash = {}.compare_by_identity
hash[foo] = :bar
hash.key?(baz)
----

=== References

* https://rubystyle.guide#identity-comparison

== Lint/HeredocMethodCallPosition

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -279,6 +279,7 @@
require_relative 'rubocop/cop/lint/float_comparison'
require_relative 'rubocop/cop/lint/float_out_of_range'
require_relative 'rubocop/cop/lint/format_parameter_mismatch'
require_relative 'rubocop/cop/lint/hash_compare_by_identity'
require_relative 'rubocop/cop/lint/heredoc_method_call_position'
require_relative 'rubocop/cop/lint/identity_comparison'
require_relative 'rubocop/cop/lint/implicit_string_concatenation'
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/correctors/line_break_corrector.rb
Expand Up @@ -52,8 +52,8 @@ def remove_semicolon(node, corrector)
end

def semicolon(node)
@semicolon ||= {}
@semicolon[node.object_id] ||= processed_source.tokens_within(node).find(&:semicolon?)
@semicolon ||= {}.compare_by_identity
@semicolon[node] ||= processed_source.tokens_within(node).find(&:semicolon?)
end
end
end
Expand Down
37 changes: 37 additions & 0 deletions lib/rubocop/cop/lint/hash_compare_by_identity.rb
@@ -0,0 +1,37 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Lint
# Prefer using `Hash#compare_by_identity` than using `object_id` for hash keys.
#
# This cop is marked as unsafe as a hash possibly can contain other keys
# besides `object_id`s.
#
# @example
# # bad
# hash = {}
# hash[foo.object_id] = :bar
# hash.key?(baz.object_id)
#
# # good
# hash = {}.compare_by_identity
# hash[foo] = :bar
# hash.key?(baz)
#
class HashCompareByIdentity < Base
RESTRICT_ON_SEND = %i[key? has_key? fetch [] []=].freeze

MSG = 'Use `Hash#compare_by_identity` instead of using `object_id` for keys.'

def_node_matcher :id_as_hash_key?, <<~PATTERN
(send _ {:key? :has_key? :fetch :[] :[]=} (send _ :object_id) ...)
PATTERN

def on_send(node)
add_offense(node) if id_as_hash_key?(node)
end
end
end
end
end
4 changes: 2 additions & 2 deletions lib/rubocop/result_cache.rb
Expand Up @@ -209,8 +209,8 @@ def relevant_options_digest(options)
# The external dependency checksums are cached per RuboCop team so that
# the checksums don't need to be recomputed for each file.
def team_checksum(team)
@checksum_by_team ||= {}
@checksum_by_team[team.object_id] ||= team.external_dependency_checksum
@checksum_by_team ||= {}.compare_by_identity
@checksum_by_team[team] ||= team.external_dependency_checksum
end

# We combine team and options into a single "context" checksum to avoid
Expand Down
8 changes: 4 additions & 4 deletions lib/rubocop/runner.rb
Expand Up @@ -324,8 +324,8 @@ def mobilize_team(processed_source)
end

def mobilized_cop_classes(config)
@mobilized_cop_classes ||= {}
@mobilized_cop_classes[config.object_id] ||= begin
@mobilized_cop_classes ||= {}.compare_by_identity
@mobilized_cop_classes[config] ||= begin
cop_classes = Cop::Registry.all

OptionsValidator.new(@options).validate_cop_options
Expand Down Expand Up @@ -399,8 +399,8 @@ def get_processed_source(file)
# otherwise dormant team that can be used for config- and option-
# level caching in ResultCache.
def standby_team(config)
@team_by_config ||= {}
@team_by_config[config.object_id] ||=
@team_by_config ||= {}.compare_by_identity
@team_by_config[config] ||=
Cop::Team.mobilize(mobilized_cop_classes(config), config, @options)
end
end
Expand Down
33 changes: 33 additions & 0 deletions spec/rubocop/cop/lint/hash_compare_by_identity_spec.rb
@@ -0,0 +1,33 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Lint::HashCompareByIdentity do
subject(:cop) { described_class.new }

it 'registers an offense when using hash methods with `object_id` on receiver as a key' do
expect_offense(<<~RUBY)
hash.key?(foo.object_id)
^^^^^^^^^^^^^^^^^^^^^^^^ Use `Hash#compare_by_identity` instead of using `object_id` for keys.
hash.has_key?(foo.object_id)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Hash#compare_by_identity` instead of using `object_id` for keys.
hash[foo.object_id] = bar
^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Hash#compare_by_identity` instead of using `object_id` for keys.
hash[foo.object_id]
^^^^^^^^^^^^^^^^^^^ Use `Hash#compare_by_identity` instead of using `object_id` for keys.
hash.fetch(foo.object_id, 42)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Hash#compare_by_identity` instead of using `object_id` for keys.
RUBY
end

it 'registers an offense when using hash method with `object_id` as a key' do
expect_offense(<<~RUBY)
hash.key?(object_id)
^^^^^^^^^^^^^^^^^^^^ Use `Hash#compare_by_identity` instead of using `object_id` for keys.
RUBY
end

it 'does not register an offense for hash methods without `object_id` as key' do
expect_no_offenses(<<~RUBY)
hash.key?(foo)
RUBY
end
end