diff --git a/CHANGELOG.md b/CHANGELOG.md index b00e7c51c2..ba79425031 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### New features * [#197](https://github.com/rubocop-hq/rubocop-rails/pull/197): Add `Rails/UniqueValidationWithoutIndex` cop. ([@pocke][]) +* [#208](https://github.com/rubocop-hq/rubocop-rails/pull/208): Add new `Rails/IndexBy` and `Rails/IndexWith` cops. ([@djudd][], [@eugeneius][]) ### Bug fixes @@ -144,3 +145,4 @@ [@fidalgo]: https://github.com/fidalgo [@hanachin]: https://github.com/hanachin [@joshpencheon]: https://github.com/joshpencheon +[@djudd]: https://github.com/djudd diff --git a/config/default.yml b/config/default.yml index 199cca414e..c25b2ad9e6 100644 --- a/config/default.yml +++ b/config/default.yml @@ -268,6 +268,16 @@ Rails/IgnoredSkipActionFilterOption: Include: - app/controllers/**/*.rb +Rails/IndexBy: + Description: 'Prefer `index_by` over `each_with_object` or `map`.' + Enabled: true + VersionAdded: '2.5' + +Rails/IndexWith: + Description: 'Prefer `index_with` over `each_with_object` or `map`.' + Enabled: true + VersionAdded: '2.5' + Rails/InverseOf: Description: 'Checks for associations where the inverse cannot be determined automatically.' Enabled: true diff --git a/lib/rubocop/cop/mixin/index_method.rb b/lib/rubocop/cop/mixin/index_method.rb new file mode 100644 index 0000000000..2e8e2b5d5a --- /dev/null +++ b/lib/rubocop/cop/mixin/index_method.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Common functionality for Rails/IndexBy and Rails/IndexWith + module IndexMethod # rubocop:disable Metrics/ModuleLength + def on_block(node) + on_bad_each_with_object(node) do |*match| + handle_possible_offense(node, match, 'each_with_object') + end + end + + def on_send(node) + on_bad_map_to_h(node) do |*match| + handle_possible_offense(node, match, 'map { ... }.to_h') + end + + on_bad_hash_brackets_map(node) do |*match| + handle_possible_offense(node, match, 'Hash[map { ... }]') + end + end + + def on_csend(node) + on_bad_map_to_h(node) do |*match| + handle_possible_offense(node, match, 'map { ... }.to_h') + end + end + + def autocorrect(node) + lambda do |corrector| + correction = prepare_correction(node) + execute_correction(corrector, node, correction) + end + end + + private + + # @abstract Implemented with `def_node_matcher` + def on_bad_each_with_object(_node) + raise NotImplementedError + end + + # @abstract Implemented with `def_node_matcher` + def on_bad_map_to_h(_node) + raise NotImplementedError + end + + # @abstract Implemented with `def_node_matcher` + def on_bad_hash_brackets_map(_node) + raise NotImplementedError + end + + def handle_possible_offense(node, match, match_desc) + captures = extract_captures(match) + + return if captures.noop_transformation? + + add_offense( + node, + message: "Prefer `#{new_method_name}` over `#{match_desc}`." + ) + end + + def extract_captures(match) + argname, body_expr = *match + Captures.new(argname, body_expr) + end + + def new_method_name + raise NotImplementedError + end + + def prepare_correction(node) + if (match = on_bad_each_with_object(node)) + Autocorrection.from_each_with_object(node, match) + elsif (match = on_bad_map_to_h(node)) + Autocorrection.from_map_to_h(node, match) + elsif (match = on_bad_hash_brackets_map(node)) + Autocorrection.from_hash_brackets_map(node, match) + else + raise 'unreachable' + end + end + + def execute_correction(corrector, node, correction) + correction.strip_prefix_and_suffix(node, corrector) + correction.set_new_method_name(new_method_name, corrector) + + captures = extract_captures(correction.match) + correction.set_new_arg_name(captures.transformed_argname, corrector) + correction.set_new_body_expression( + captures.transforming_body_expr, + corrector + ) + end + + # Internal helper class to hold match data + Captures = Struct.new( + :transformed_argname, + :transforming_body_expr + ) do + def noop_transformation? + transforming_body_expr.lvar_type? && + transforming_body_expr.children == [transformed_argname] + end + end + + # Internal helper class to hold autocorrect data + Autocorrection = Struct.new(:match, :block_node, :leading, :trailing) do # rubocop:disable Metrics/BlockLength + def self.from_each_with_object(node, match) + new(match, node, 0, 0) + end + + def self.from_map_to_h(node, match) + strip_trailing_chars = node.parent&.block_type? ? 0 : '.to_h'.length + new(match, node.children.first, 0, strip_trailing_chars) + end + + def self.from_hash_brackets_map(node, match) + new(match, node.children.last, 'Hash['.length, ']'.length) + end + + def strip_prefix_and_suffix(node, corrector) + expression = node.loc.expression + corrector.remove_leading(expression, leading) + corrector.remove_trailing(expression, trailing) + end + + def set_new_method_name(new_method_name, corrector) + range = block_node.send_node.loc.selector + if (send_end = block_node.send_node.loc.end) + # If there are arguments (only true in the `each_with_object` case) + range = range.begin.join(send_end) + end + corrector.replace(range, new_method_name) + end + + def set_new_arg_name(transformed_argname, corrector) + corrector.replace( + block_node.arguments.loc.expression, + "|#{transformed_argname}|" + ) + end + + def set_new_body_expression(transforming_body_expr, corrector) + corrector.replace( + block_node.body.loc.expression, + transforming_body_expr.loc.expression.source + ) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails/index_by.rb b/lib/rubocop/cop/rails/index_by.rb new file mode 100644 index 0000000000..486927aca0 --- /dev/null +++ b/lib/rubocop/cop/rails/index_by.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop looks for uses of `each_with_object({}) { ... }`, + # `map { ... }.to_h`, and `Hash[map { ... }]` that are transforming + # an enumerable into a hash where the values are the original elements. + # Rails provides the `index_by` method for this purpose. + # + # @example + # # bad + # [1, 2, 3].each_with_object({}) { |el, h| h[foo(el)] = el } + # [1, 2, 3].map { |el| [foo(el), el] }.to_h + # Hash[[1, 2, 3].collect { |el| [foo(el), el] }] + # + # # good + # [1, 2, 3].index_by { |el| foo(el) } + class IndexBy < Cop + include IndexMethod + + def_node_matcher :on_bad_each_with_object, <<~PATTERN + (block + ({send csend} _ :each_with_object (hash)) + (args (arg $_el) (arg _memo)) + ({send csend} (lvar _memo) :[]= $_ (lvar _el))) + PATTERN + + def_node_matcher :on_bad_map_to_h, <<~PATTERN + ({send csend} + (block + ({send csend} _ {:map :collect}) + (args (arg $_el)) + (array $_ (lvar _el))) + :to_h) + PATTERN + + def_node_matcher :on_bad_hash_brackets_map, <<~PATTERN + (send + (const _ :Hash) + :[] + (block + ({send csend} _ {:map :collect}) + (args (arg $_el)) + (array $_ (lvar _el)))) + PATTERN + + private + + def new_method_name + 'index_by' + end + end + end + end +end diff --git a/lib/rubocop/cop/rails/index_with.rb b/lib/rubocop/cop/rails/index_with.rb new file mode 100644 index 0000000000..9ea2630721 --- /dev/null +++ b/lib/rubocop/cop/rails/index_with.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop looks for uses of `each_with_object({}) { ... }`, + # `map { ... }.to_h`, and `Hash[map { ... }]` that are transforming + # an enumerable into a hash where the keys are the original elements. + # Rails provides the `index_with` method for this purpose. + # + # @example + # # bad + # [1, 2, 3].each_with_object({}) { |el, h| h[el] = foo(el) } + # [1, 2, 3].map { |el| [el, foo(el)] }.to_h + # Hash[[1, 2, 3].collect { |el| [el, foo(el)] }] + # + # # good + # [1, 2, 3].index_with { |el| foo(el) } + class IndexWith < Cop + extend TargetRailsVersion + include IndexMethod + + minimum_target_rails_version 6.0 + + def_node_matcher :on_bad_each_with_object, <<~PATTERN + (block + ({send csend} _ :each_with_object (hash)) + (args (arg $_el) (arg _memo)) + ({send csend} (lvar _memo) :[]= (lvar _el) $_)) + PATTERN + + def_node_matcher :on_bad_map_to_h, <<~PATTERN + ({send csend} + (block + ({send csend} _ {:map :collect}) + (args (arg $_el)) + (array (lvar _el) $_)) + :to_h) + PATTERN + + def_node_matcher :on_bad_hash_brackets_map, <<~PATTERN + (send + (const _ :Hash) + :[] + (block + ({send csend} _ {:map :collect}) + (args (arg $_el)) + (array (lvar _el) $_))) + PATTERN + + private + + def new_method_name + 'index_with' + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 45e465d94c..58404c8e83 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative 'mixin/active_record_helper' +require_relative 'mixin/index_method' require_relative 'mixin/target_rails_version' require_relative 'rails/action_filter' @@ -33,6 +34,8 @@ require_relative 'rails/http_positional_arguments' require_relative 'rails/http_status' require_relative 'rails/ignored_skip_action_filter_option' +require_relative 'rails/index_by' +require_relative 'rails/index_with' require_relative 'rails/inverse_of' require_relative 'rails/lexically_scoped_action_filter' require_relative 'rails/link_to_blank' diff --git a/manual/cops.md b/manual/cops.md index 2c56f775a5..7d1487530d 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -31,6 +31,8 @@ * [Rails/HttpPositionalArguments](cops_rails.md#railshttppositionalarguments) * [Rails/HttpStatus](cops_rails.md#railshttpstatus) * [Rails/IgnoredSkipActionFilterOption](cops_rails.md#railsignoredskipactionfilteroption) +* [Rails/IndexBy](cops_rails.md#railsindexby) +* [Rails/IndexWith](cops_rails.md#railsindexwith) * [Rails/InverseOf](cops_rails.md#railsinverseof) * [Rails/LexicallyScopedActionFilter](cops_rails.md#railslexicallyscopedactionfilter) * [Rails/LinkToBlank](cops_rails.md#railslinktoblank) diff --git a/manual/cops_rails.md b/manual/cops_rails.md index 7c33dfab25..2ffd766f10 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -1137,6 +1137,52 @@ Include | `app/controllers/**/*.rb` | Array * [https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options](https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options) +## Rails/IndexBy + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | Yes | 2.5 | - + +This cop looks for uses of `each_with_object({}) { ... }`, +`map { ... }.to_h`, and `Hash[map { ... }]` that are transforming +an enumerable into a hash where the values are the original elements. +Rails provides the `index_by` method for this purpose. + +### Examples + +```ruby +# bad +[1, 2, 3].each_with_object({}) { |el, h| h[foo(el)] = el } +[1, 2, 3].map { |el| [foo(el), el] }.to_h +Hash[[1, 2, 3].collect { |el| [foo(el), el] }] + +# good +[1, 2, 3].index_by { |el| foo(el) } +``` + +## Rails/IndexWith + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | Yes | 2.5 | - + +This cop looks for uses of `each_with_object({}) { ... }`, +`map { ... }.to_h`, and `Hash[map { ... }]` that are transforming +an enumerable into a hash where the keys are the original elements. +Rails provides the `index_with` method for this purpose. + +### Examples + +```ruby +# bad +[1, 2, 3].each_with_object({}) { |el, h| h[el] = foo(el) } +[1, 2, 3].map { |el| [el, foo(el)] }.to_h +Hash[[1, 2, 3].collect { |el| [el, foo(el)] }] + +# good +[1, 2, 3].index_with { |el| foo(el) } +``` + ## Rails/InverseOf Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/rails/index_by_spec.rb b/spec/rubocop/cop/rails/index_by_spec.rb new file mode 100644 index 0000000000..9566a5a1d3 --- /dev/null +++ b/spec/rubocop/cop/rails/index_by_spec.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::IndexBy, :config do + subject(:cop) { described_class.new(config) } + + context 'with an inline block' do + it 'registers an offense for `each_with_object`' do + expect_offense(<<~RUBY) + x.each_with_object({}) { |el, h| h[foo(el)] = el } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `each_with_object`. + RUBY + + expect_correction(<<~RUBY) + x.index_by { |el| foo(el) } + RUBY + end + end + + context 'with a multiline block' do + it 'registers an offense for `each_with_object`' do + expect_offense(<<~RUBY) + x.each_with_object({}) do |el, memo| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `each_with_object`. + memo[el.to_sym] = el + end + RUBY + + expect_correction(<<~RUBY) + x.index_by do |el| + el.to_sym + end + RUBY + end + end + + context 'with safe navigation operator' do + it 'registers an offense for `each_with_object`' do + expect_offense(<<~RUBY) + x&.each_with_object({}) { |el, h| h[foo(el)] = el } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `each_with_object`. + RUBY + + expect_correction(<<~RUBY) + x&.index_by { |el| foo(el) } + RUBY + end + end + + context 'when values are transformed' do + it 'does not register an offense for `each_with_object`' do + expect_no_offenses(<<~RUBY) + x.each_with_object({}) { |el, h| h[el.to_sym] = foo(el) } + RUBY + end + end + + context 'when keys are not transformed' do + it 'does not register an offense for `each_with_object`' do + expect_no_offenses('x.each_with_object({}) { |el, h| h[el] = el }') + end + end + + context 'when the given hash is not used' do + it 'does not register an offense for `each_with_object`' do + expect_no_offenses(<<~RUBY) + x.each_with_object({}) { |el, h| other_h[el.to_sym] = el } + RUBY + end + end + + context 'when `to_h` is given a block' do + it 'registers an offense for `map { ... }.to_h' do + expect_offense(<<~RUBY) + x.map { |el| [el.to_sym, el] }.to_h { |k, v| [v, k] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `map { ... }.to_h`. + RUBY + + expect_correction(<<~RUBY) + x.index_by { |el| el.to_sym }.to_h { |k, v| [v, k] } + RUBY + end + end + + context 'when `to_h` is not given a block' do + it 'registers an offense for `map { ... }.to_h`' do + expect_offense(<<~RUBY) + x.map { |el| [el.to_sym, el] }.to_h + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `map { ... }.to_h`. + RUBY + + expect_correction(<<~RUBY) + x.index_by { |el| el.to_sym } + RUBY + end + end + + context 'when to_h is not called on the result' do + it 'does not register an offense for `map { ... }.to_h`' do + expect_no_offenses('x.map { |el| [el.to_sym, el] }') + end + end + + it 'registers an offense for `Hash[map { ... }]`' do + expect_offense(<<~RUBY) + Hash[x.map { |el| [el.to_sym, el] }] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `Hash[map { ... }]`. + RUBY + + expect_correction(<<~RUBY) + x.index_by { |el| el.to_sym } + RUBY + end +end diff --git a/spec/rubocop/cop/rails/index_with_spec.rb b/spec/rubocop/cop/rails/index_with_spec.rb new file mode 100644 index 0000000000..197909ec5c --- /dev/null +++ b/spec/rubocop/cop/rails/index_with_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::IndexWith, :config do + subject(:cop) { described_class.new(config) } + + context 'when using Rails 6.0 or newer', :rails60 do + context 'with an inline block' do + it 'registers an offense for `each_with_object`' do + expect_offense(<<~RUBY) + x.each_with_object({}) { |el, h| h[el] = foo(el) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `each_with_object`. + RUBY + + expect_correction(<<~RUBY) + x.index_with { |el| foo(el) } + RUBY + end + end + + context 'with a multiline block' do + it 'registers an offense for `each_with_object`' do + expect_offense(<<~RUBY) + x.each_with_object({}) do |el, memo| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `each_with_object`. + memo[el] = el.to_sym + end + RUBY + + expect_correction(<<~RUBY) + x.index_with do |el| + el.to_sym + end + RUBY + end + end + + context 'with the safe navigation operator' do + it 'registers an offense for `each_with_object`' do + expect_offense(<<~RUBY) + x&.each_with_object({}) { |el, h| h[el] = foo(el) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `each_with_object`. + RUBY + + expect_correction(<<~RUBY) + x&.index_with { |el| foo(el) } + RUBY + end + end + + context 'when keys are transformed' do + it 'does not register an offense for `each_with_object`' do + expect_no_offenses(<<~RUBY) + x.each_with_object({}) { |el, h| h[el.to_sym] = foo(el) } + RUBY + end + end + + context 'when values are not transformed' do + it 'does not register an offense for `each_with_object`' do + expect_no_offenses('x.each_with_object({}) { |el, h| h[el] = el }') + end + end + + context 'when the given hash is not used' do + it 'does not register an offense for `each_with_object`' do + expect_no_offenses(<<~RUBY) + x.each_with_object({}) { |el, h| other_h[el] = el.to_sym } + RUBY + end + end + + context 'when `to_h` is given a block' do + it 'registers an offense for `map { ... }.to_h' do + expect_offense(<<~RUBY) + x.map { |el| [el, el.to_sym] }.to_h { |k, v| [v, k] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `map { ... }.to_h`. + RUBY + + expect_correction(<<~RUBY) + x.index_with { |el| el.to_sym }.to_h { |k, v| [v, k] } + RUBY + end + end + + context 'when `to_h` is not given a block' do + it 'registers an offense for `map { ... }.to_h`' do + expect_offense(<<~RUBY) + x.map { |el| [el, el.to_sym] }.to_h + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `map { ... }.to_h`. + RUBY + + expect_correction(<<~RUBY) + x.index_with { |el| el.to_sym } + RUBY + end + end + + context 'when to_h is not called on the result' do + it 'does not register an offense for `map { ... }.to_h`' do + expect_no_offenses('x.map { |el| [el, el.to_sym] }') + end + end + + it 'registers an offense for `Hash[map { ... }]`' do + expect_offense(<<~RUBY) + Hash[x.map { |el| [el, el.to_sym] }] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `Hash[map { ... }]`. + RUBY + + expect_correction(<<~RUBY) + x.index_with { |el| el.to_sym } + RUBY + end + end + + context 'when using Rails 5.2 or older', :rails52 do + it 'does not register an offense for `each_with_object`' do + expect_no_offenses('x.each_with_object({}) { |el, h| h[el] = foo(el) }') + end + + it 'does not register an offense for `map { ... }.to_h`' do + expect_no_offenses('x.map { |el| [el, el.to_sym] }.to_h') + end + + it 'does not register an offense for `Hash[map { ... }]`' do + expect_no_offenses('Hash[x.map { |el| [el, el.to_sym] }]') + end + end +end