Merge pull request #324 from eugeneius/index_by_index_with_to_h_with_…

…block

Handle to_h with block in Rails/IndexBy and Rails/IndexWith

CHANGELOG.md

@@ -20,6 +20,7 @@
 * [#294](https://github.com/rubocop-hq/rubocop-rails/pull/294): Update `Rails/ReversibleMigration` to register offenses for `remove_columns` and `remove_index`. ([@philcoggins][])
 * [#310](https://github.com/rubocop-hq/rubocop-rails/issues/310): Add `EnforcedStyle` to `Rails/PluckInWhere`. By default, it does not register an offense if `pluck` method's receiver is a variable. ([@koic][])
 * [#320](https://github.com/rubocop-hq/rubocop-rails/pull/320): Mark `Rails/UniqBeforePluck` as unsafe. ([@kunitoo][])
+* [#324](https://github.com/rubocop-hq/rubocop-rails/pull/324): Make `Rails/IndexBy` and `Rails/IndexWith` aware of `to_h` with block. ([@eugeneius][])
 
 ## 2.7.1 (2020-07-26)
 

config/default.yml

@@ -321,14 +321,16 @@ Rails/IgnoredSkipActionFilterOption:
     - app/controllers/**/*.rb
 
 Rails/IndexBy:
-  Description: 'Prefer `index_by` over `each_with_object` or `map`.'
+  Description: 'Prefer `index_by` over `each_with_object`, `to_h`, or `map`.'
   Enabled: true
   VersionAdded: '2.5'
+  VersionChanged: '2.8'
 
 Rails/IndexWith:
-  Description: 'Prefer `index_with` over `each_with_object` or `map`.'
+  Description: 'Prefer `index_with` over `each_with_object`, `to_h`, or `map`.'
   Enabled: true
   VersionAdded: '2.5'
+  VersionChanged: '2.8'
 
 Rails/Inquiry:
   Description: "Prefer Ruby's comparison operators over Active Support's `Array#inquiry` and `String#inquiry`."

docs/modules/ROOT/pages/cops_rails.adoc

@@ -1696,7 +1696,7 @@ end
 | Yes
 | Yes
 | 2.5
-| -
+| 2.8
 |===
 
 This cop looks for uses of `each_with_object({}) { ... }`,
@@ -1710,6 +1710,7 @@ Rails provides the `index_by` method for this purpose.
 ----
 # bad
 [1, 2, 3].each_with_object({}) { |el, h| h[foo(el)] = el }
+[1, 2, 3].to_h { |el| [foo(el), el] }
 [1, 2, 3].map { |el| [foo(el), el] }.to_h
 Hash[[1, 2, 3].collect { |el| [foo(el), el] }]
 
@@ -1726,7 +1727,7 @@ Hash[[1, 2, 3].collect { |el| [foo(el), el] }]
 | Yes
 | Yes
 | 2.5
-| -
+| 2.8
 |===
 
 This cop looks for uses of `each_with_object({}) { ... }`,
@@ -1740,6 +1741,7 @@ Rails provides the `index_with` method for this purpose.
 ----
 # bad
 [1, 2, 3].each_with_object({}) { |el, h| h[el] = foo(el) }
+[1, 2, 3].to_h { |el| [el, foo(el)] }
 [1, 2, 3].map { |el| [el, foo(el)] }.to_h
 Hash[[1, 2, 3].collect { |el| [el, foo(el)] }]
 

lib/rubocop/cop/mixin/index_method.rb

@@ -8,6 +8,12 @@ def on_block(node)
         on_bad_each_with_object(node) do |*match|
           handle_possible_offense(node, match, 'each_with_object')
         end
+
+        return if target_ruby_version < 2.6
+
+        on_bad_to_h(node) do |*match|
+          handle_possible_offense(node, match, 'to_h { ... }')
+        end
       end
 
       def on_send(node)
@@ -40,6 +46,11 @@ def on_bad_each_with_object(_node)
         raise NotImplementedError
       end
 
+      # @abstract Implemented with `def_node_matcher`
+      def on_bad_to_h(_node)
+        raise NotImplementedError
+      end
+
       # @abstract Implemented with `def_node_matcher`
       def on_bad_map_to_h(_node)
         raise NotImplementedError
@@ -73,6 +84,8 @@ def new_method_name
       def prepare_correction(node)
         if (match = on_bad_each_with_object(node))
           Autocorrection.from_each_with_object(node, match)
+        elsif (match = on_bad_to_h(node))
+          Autocorrection.from_to_h(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))
@@ -111,6 +124,10 @@ def self.from_each_with_object(node, match)
           new(match, node, 0, 0)
         end
 
+        def self.from_to_h(node, match)
+          new(match, node, 0, 0)
+        end
+
         def self.from_map_to_h(node, match)
           strip_trailing_chars = 0
 

lib/rubocop/cop/rails/index_by.rb

@@ -11,6 +11,7 @@ module Rails
       # @example
       #   # bad
       #   [1, 2, 3].each_with_object({}) { |el, h| h[foo(el)] = el }
+      #   [1, 2, 3].to_h { |el| [foo(el), el] }
       #   [1, 2, 3].map { |el| [foo(el), el] }.to_h
       #   Hash[[1, 2, 3].collect { |el| [foo(el), el] }]
       #
@@ -26,6 +27,13 @@ class IndexBy < Cop
             ({send csend} (lvar _memo) :[]= $_ (lvar _el)))
         PATTERN
 
+        def_node_matcher :on_bad_to_h, <<~PATTERN
+          (block
+            ({send csend} _ :to_h)
+            (args (arg $_el))
+            (array $_ (lvar _el)))
+        PATTERN
+
         def_node_matcher :on_bad_map_to_h, <<~PATTERN
           ({send csend}
             (block

lib/rubocop/cop/rails/index_with.rb

@@ -11,6 +11,7 @@ module Rails
       # @example
       #   # bad
       #   [1, 2, 3].each_with_object({}) { |el, h| h[el] = foo(el) }
+      #   [1, 2, 3].to_h { |el| [el, foo(el)] }
       #   [1, 2, 3].map { |el| [el, foo(el)] }.to_h
       #   Hash[[1, 2, 3].collect { |el| [el, foo(el)] }]
       #
@@ -29,6 +30,13 @@ class IndexWith < Cop
             ({send csend} (lvar _memo) :[]= (lvar _el) $_))
         PATTERN
 
+        def_node_matcher :on_bad_to_h, <<~PATTERN
+          (block
+            ({send csend} _ :to_h)
+            (args (arg $_el))
+            (array (lvar _el) $_))
+        PATTERN
+
         def_node_matcher :on_bad_map_to_h, <<~PATTERN
           ({send csend}
             (block

spec/rubocop/cop/rails/index_by_spec.rb

@@ -69,7 +69,7 @@
   end
 
   context 'when `to_h` is given a block' do
-    it 'registers an offense for `map { ... }.to_h' 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`.
@@ -138,4 +138,25 @@
       x.index_by { |el| el.to_sym }
     RUBY
   end
+
+  context 'when using Ruby 2.6 or newer', :ruby26 do
+    it 'registers an offense for `to_h { ... }`' do
+      expect_offense(<<~RUBY)
+        x.to_h { |el| [el.to_sym, el] }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `to_h { ... }`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        x.index_by { |el| el.to_sym }
+      RUBY
+    end
+  end
+
+  context 'when using Ruby 2.5 or older', :ruby25 do
+    it 'does not register an offense for `to_h { ... }`' do
+      expect_no_offenses(<<~RUBY)
+        x.to_h { |el| [el.to_sym, el] }
+      RUBY
+    end
+  end
 end

spec/rubocop/cop/rails/index_with_spec.rb

@@ -70,7 +70,7 @@
     end
 
     context 'when `to_h` is given a block' do
-      it 'registers an offense for `map { ... }.to_h' 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`.
@@ -111,13 +111,40 @@
         x.index_with { |el| el.to_sym }
       RUBY
     end
+
+    context 'when using Ruby 2.6 or newer', :ruby26 do
+      it 'registers an offense for `to_h { ... }`' do
+        expect_offense(<<~RUBY)
+          x.to_h { |el| [el, el.to_sym] }
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `to_h { ... }`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          x.index_with { |el| el.to_sym }
+        RUBY
+      end
+    end
+
+    context 'when using Ruby 2.5 or older', :ruby25 do
+      it 'does not register an offense for `to_h { ... }`' do
+        expect_no_offenses(<<~RUBY)
+          x.to_h { |el| [el, el.to_sym] }
+        RUBY
+      end
+    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
 
+    context 'when using Ruby 2.6 or newer', :ruby26 do
+      it 'does not register an offense for `to_h { ... }`' do
+        expect_no_offenses('x.to_h { |el| [el, el.to_sym] }')
+      end
+    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