From 074ce487a28aedc6dc27bc11710f88f48e1b30db Mon Sep 17 00:00:00 2001 From: fatkodima Date: Sat, 26 Sep 2020 15:50:09 +0300 Subject: [PATCH] Add new `Lint/HashCompareByIdentity` cop --- CHANGELOG.md | 4 ++ config/default.yml | 7 ++++ docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_lint.adoc | 36 ++++++++++++++++++ lib/rubocop.rb | 1 + .../cop/correctors/line_break_corrector.rb | 4 +- .../cop/lint/hash_compare_by_identity.rb | 37 +++++++++++++++++++ lib/rubocop/result_cache.rb | 4 +- lib/rubocop/runner.rb | 8 ++-- .../cop/lint/hash_compare_by_identity_spec.rb | 33 +++++++++++++++++ 10 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 lib/rubocop/cop/lint/hash_compare_by_identity.rb create mode 100644 spec/rubocop/cop/lint/hash_compare_by_identity_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a8e405f237..0a05e7ff2fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/config/default.yml b/config/default.yml index aabcc34890a..d5eccb35cf4 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index b4425d42bae..77f491efdc9 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -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] diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index f087a737324..616201840a6 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -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 |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 56fd0bc6c68..88984b1e257 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -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' diff --git a/lib/rubocop/cop/correctors/line_break_corrector.rb b/lib/rubocop/cop/correctors/line_break_corrector.rb index 9b07c570600..8911cf7f0d6 100644 --- a/lib/rubocop/cop/correctors/line_break_corrector.rb +++ b/lib/rubocop/cop/correctors/line_break_corrector.rb @@ -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 diff --git a/lib/rubocop/cop/lint/hash_compare_by_identity.rb b/lib/rubocop/cop/lint/hash_compare_by_identity.rb new file mode 100644 index 00000000000..31e887cc542 --- /dev/null +++ b/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 diff --git a/lib/rubocop/result_cache.rb b/lib/rubocop/result_cache.rb index 643906996df..a124c8b2ffe 100644 --- a/lib/rubocop/result_cache.rb +++ b/lib/rubocop/result_cache.rb @@ -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 diff --git a/lib/rubocop/runner.rb b/lib/rubocop/runner.rb index cd46878d099..daf1fa3e752 100644 --- a/lib/rubocop/runner.rb +++ b/lib/rubocop/runner.rb @@ -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 @@ -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 diff --git a/spec/rubocop/cop/lint/hash_compare_by_identity_spec.rb b/spec/rubocop/cop/lint/hash_compare_by_identity_spec.rb new file mode 100644 index 00000000000..8bd0fa029d1 --- /dev/null +++ b/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