Merge pull request rubocop#208 from eugeneius/index_by_index_with

Add new Rails/IndexBy and Rails/IndexWith cops

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

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

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

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

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

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'

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)

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