[Fix #8630] Fix some hash transformation false positives

We can't know for certain when the receiver is a hash, which is why these cops are marked as unsafe. However there are some false positives that we can detect. Array literals and `each_with_index` were already handled; here I'm adding `with_index` and `zip`.
rubocop · Sep 5, 2020 · f641f6e · f641f6e
1 parent 69700a5
commit f641f6e
Show file tree

Hide file tree

Showing 6 changed files with 196 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@
 
 * [#8627](https://github.com/rubocop-hq/rubocop/issues/8627): Fix a false positive for `Lint/DuplicateRequire` when same feature argument but different require method. ([@koic][])
 * [#8572](https://github.com/rubocop-hq/rubocop/issues/8572): Fix a false positive for `Style/RedundantParentheses` when parentheses are used like method argument parentheses. ([@koic][])
+* [#8630](https://github.com/rubocop-hq/rubocop/issues/8630): Fix some false positives for `Style/HashTransformKeys` and `Style/HashTransformValues` when the receiver is an array. ([@eugeneius][])
 * [#8653](https://github.com/rubocop-hq/rubocop/pull/8653): Fix a false positive for `Layout/DefEndAlignment` when using refinements and `private def`. ([@koic][])
 
 ### Changes

diff --git a/lib/rubocop/cop/mixin/hash_transform_method.rb b/lib/rubocop/cop/mixin/hash_transform_method.rb
@@ -4,9 +4,15 @@ module RuboCop
   module Cop
     # Common functionality for Style/HashTransformKeys and
     # Style/HashTransformValues
-    module HashTransformMethod
+    module HashTransformMethod # rubocop:disable Metrics/ModuleLength
+      extend NodePattern::Macros
+
       RESTRICT_ON_SEND = %i[[] to_h].freeze
 
+      def_node_matcher :array_receiver?, <<~PATTERN
+        {(array ...) (send _ :each_with_index) (send _ :with_index _ ?) (send _ :zip ...)}
+      PATTERN
+
       def on_block(node)
         on_bad_each_with_object(node) do |*match|
           handle_possible_offense(node, match, 'each_with_object')

diff --git a/lib/rubocop/cop/style/hash_transform_keys.rb b/lib/rubocop/cop/style/hash_transform_keys.rb
@@ -34,9 +34,7 @@ class HashTransformKeys < Base
 
         def_node_matcher :on_bad_each_with_object, <<~PATTERN
           (block
-            ({send csend}
-              !{(send _ :each_with_index) (array ...)}
-              :each_with_object (hash))
+            ({send csend} !#array_receiver? :each_with_object (hash))
             (args
               (mlhs
                 (arg $_)
@@ -50,7 +48,7 @@ class HashTransformKeys < Base
             (const _ :Hash)
             :[]
             (block
-              ({send csend} !(send _ :each_with_index) {:map :collect})
+              ({send csend} !#array_receiver? {:map :collect})
               (args
                 (arg $_)
                 (arg _val))
@@ -60,9 +58,7 @@ class HashTransformKeys < Base
         def_node_matcher :on_bad_map_to_h, <<~PATTERN
           ({send csend}
             (block
-              ({send csend}
-                !{(send _ :each_with_index) (array ...)}
-                {:map :collect})
+              ({send csend} !#array_receiver? {:map :collect})
               (args
                 (arg $_)
                 (arg _val))
@@ -72,9 +68,7 @@ class HashTransformKeys < Base
 
         def_node_matcher :on_bad_to_h, <<~PATTERN
           (block
-            ({send csend}
-              !{(send _ :each_with_index) (array ...)}
-              :to_h)
+            ({send csend} !#array_receiver? :to_h)
             (args
               (arg $_)
               (arg _val))

diff --git a/lib/rubocop/cop/style/hash_transform_values.rb b/lib/rubocop/cop/style/hash_transform_values.rb
@@ -31,9 +31,7 @@ class HashTransformValues < Base
 
         def_node_matcher :on_bad_each_with_object, <<~PATTERN
           (block
-            ({send csend}
-              !{(send _ :each_with_index) (array ...)}
-              :each_with_object (hash))
+            ({send csend} !#array_receiver? :each_with_object (hash))
             (args
               (mlhs
                 (arg _key)
@@ -47,7 +45,7 @@ class HashTransformValues < Base
             (const _ :Hash)
             :[]
             (block
-              ({send csend} !(send _ :each_with_index) {:map :collect})
+              ({send csend} !#array_receiver? {:map :collect})
               (args
                 (arg _key)
                 (arg $_))
@@ -57,9 +55,7 @@ class HashTransformValues < Base
         def_node_matcher :on_bad_map_to_h, <<~PATTERN
           ({send csend}
             (block
-              ({send csend}
-                !{(send _ :each_with_index) (array ...)}
-                 {:map :collect})
+              ({send csend} !#array_receiver? {:map :collect})
               (args
                 (arg _key)
                 (arg $_))
@@ -69,9 +65,7 @@ class HashTransformValues < Base
 
         def_node_matcher :on_bad_to_h, <<~PATTERN
           (block
-            ({send csend}
-              !{(send _ :each_with_index) (array ...)}
-              :to_h)
+            ({send csend} !#array_receiver? :to_h)
             (args
               (arg _key)
               (arg $_))

diff --git a/spec/rubocop/cop/style/hash_transform_keys_spec.rb b/spec/rubocop/cop/style/hash_transform_keys_spec.rb
@@ -71,6 +71,24 @@
       RUBY
     end
 
+    it 'does not flag `each_with_object` when its receiver is `each_with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each_with_index.each_with_object({}) { |(k, v), h| h[k.to_sym] = v }
+      RUBY
+    end
+
+    it 'does not flag `each_with_object` when its receiver is `with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each.with_index.each_with_object({}) { |(k, v), h| h[k.to_sym] = v }
+      RUBY
+    end
+
+    it 'does not flag `each_with_object` when its receiver is `zip`' do
+      expect_no_offenses(<<~RUBY)
+        %i[a b c].zip([1, 2, 3]).each_with_object({}) { |(k, v), h| h[k.to_sym] = v }
+      RUBY
+    end
+
     it 'flags _.map{...}.to_h when transform_keys could be used' do
       expect_offense(<<~RUBY)
         x.map {|k, v| [k.to_sym, v]}.to_h
@@ -124,6 +142,24 @@
       RUBY
     end
 
+    it 'does not flag `_.map{...}.to_h` when its receiver is `each_with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each_with_index.map { |k, v| [k.to_sym, v] }.to_h
+      RUBY
+    end
+
+    it 'does not flag `_.map{...}.to_h` when its receiver is `with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each.with_index.map { |k, v| [k.to_sym, v] }.to_h
+      RUBY
+    end
+
+    it 'does not flag `_.map{...}.to_h` when its receiver is `zip`' do
+      expect_no_offenses(<<~RUBY)
+        %i[a b c].zip([1, 2, 3]).map { |k, v| [k.to_sym, v] }.to_h
+      RUBY
+    end
+
     it 'correctly autocorrects _.map{...}.to_h without block' do
       expect_offense(<<~RUBY)
         {a: 1, b: 2}.map do |k, v|
@@ -164,6 +200,30 @@
         end
       RUBY
     end
+
+    it 'does not flag `Hash[_.map{...}]` when its receiver is an array literal' do
+      expect_no_offenses(<<~RUBY)
+        Hash[[1, 2, 3].map { |k, v| [k.to_sym, v] }]
+      RUBY
+    end
+
+    it 'does not flag `Hash[_.map{...}]` when its receiver is `each_with_index`' do
+      expect_no_offenses(<<~RUBY)
+        Hash[[1, 2, 3].each_with_index.map { |k, v| [k.to_sym, v] }]
+      RUBY
+    end
+
+    it 'does not flag `Hash[_.map{...}]` when its receiver is `with_index`' do
+      expect_no_offenses(<<~RUBY)
+        Hash[[1, 2, 3].each.with_index.map { |k, v| [k.to_sym, v] }]
+      RUBY
+    end
+
+    it 'does not flag `Hash[_.map{...}]` when its receiver is `zip`' do
+      expect_no_offenses(<<~RUBY)
+        Hash[%i[a b c].zip([1, 2, 3]).map { |k, v| [k.to_sym, v] }]
+      RUBY
+    end
   end
 
   context 'below Ruby 2.5', :ruby24 do
@@ -183,6 +243,36 @@
         x.transform_keys {|k| k.to_sym}
       RUBY
     end
+
+    it 'does not flag `_.to_h{...}` when both key & value are transformed' do
+      expect_no_offenses(<<~RUBY)
+        x.to_h { |k, v| [k.to_sym, foo(v)] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is an array literal' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].to_h { |k, v| [k.to_sym, v] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is `each_with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each_with_index.to_h { |k, v| [k.to_sym, v] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is `with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each.with_index.to_h { |k, v| [k.to_sym, v] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is `zip`' do
+      expect_no_offenses(<<~RUBY)
+        %i[a b c].zip([1, 2, 3]).to_h { |k, v| [k.to_sym, v] }
+      RUBY
+    end
   end
 
   context 'below Ruby 2.6', :ruby25 do

diff --git a/spec/rubocop/cop/style/hash_transform_values_spec.rb b/spec/rubocop/cop/style/hash_transform_values_spec.rb
@@ -70,6 +70,24 @@
     RUBY
   end
 
+  it 'does not flag `each_with_object` when its receiver is `each_with_index`' do
+    expect_no_offenses(<<~RUBY)
+      [1, 2, 3].each_with_index.each_with_object({}) { |(k, v), h| h[k] = foo(v) }
+    RUBY
+  end
+
+  it 'does not flag `each_with_object` when its receiver is `with_index`' do
+    expect_no_offenses(<<~RUBY)
+      [1, 2, 3].each.with_index.each_with_object({}) { |(k, v), h| h[k] = foo(v) }
+    RUBY
+  end
+
+  it 'does not flag `each_with_object` when its receiver is `zip`' do
+    expect_no_offenses(<<~RUBY)
+      %i[a b c].zip([1, 2, 3]).each_with_object({}) { |(k, v), h| h[k] = foo(v) }
+    RUBY
+  end
+
   it 'flags _.map {...}.to_h when transform_values could be used' do
     expect_offense(<<~RUBY)
       x.map {|k, v| [k, foo(v)]}.to_h
@@ -123,6 +141,24 @@
     RUBY
   end
 
+  it 'does not flag `_.map{...}.to_h` when its receiver is `each_with_index`' do
+    expect_no_offenses(<<~RUBY)
+      [1, 2, 3].each_with_index.map { |k, v| [k, foo(v)] }.to_h
+    RUBY
+  end
+
+  it 'does not flag `_.map{...}.to_h` when its receiver is `with_index`' do
+    expect_no_offenses(<<~RUBY)
+      [1, 2, 3].each.with_index.map { |k, v| [k, foo(v)] }.to_h
+    RUBY
+  end
+
+  it 'does not flag `_.map{...}.to_h` when its receiver is `zip`' do
+    expect_no_offenses(<<~RUBY)
+      %i[a b c].zip([1, 2, 3]).map { |k, v| [k, foo(v)] }.to_h
+    RUBY
+  end
+
   it 'correctly autocorrects _.map{...}.to_h with block' do
     expect_offense(<<~RUBY)
       {a: 1, b: 2}.map {|k, v| [k, foo(v)]}.to_h {|k, v| [v, k]}
@@ -134,6 +170,30 @@
     RUBY
   end
 
+  it 'does not flag `Hash[_.map{...}]` when its receiver is an array literal' do
+    expect_no_offenses(<<~RUBY)
+      Hash[[1, 2, 3].map { |k, v| [k, foo(v)] }]
+    RUBY
+  end
+
+  it 'does not flag `Hash[_.map{...}]` when its receiver is `each_with_index`' do
+    expect_no_offenses(<<~RUBY)
+      Hash[[1, 2, 3].each_with_index.map { |k, v| [k, foo(v)] }]
+    RUBY
+  end
+
+  it 'does not flag `Hash[_.map{...}]` when its receiver is `with_index`' do
+    expect_no_offenses(<<~RUBY)
+      Hash[[1, 2, 3].each.with_index.map { |k, v| [k, foo(v)] }]
+    RUBY
+  end
+
+  it 'does not flag `Hash[_.map{...}]` when its receiver is `zip`' do
+    expect_no_offenses(<<~RUBY)
+      Hash[%i[a b c].zip([1, 2, 3]).map { |k, v| [k, foo(v)] }]
+    RUBY
+  end
+
   context 'when using Ruby 2.6 or newer', :ruby26 do
     it 'flags _.to_h{...} when transform_values could be used' do
       expect_offense(<<~RUBY)
@@ -145,6 +205,36 @@
         x.transform_values {|v| foo(v)}
       RUBY
     end
+
+    it 'does not flag `_.to_h{...}` when both key & value are transformed' do
+      expect_no_offenses(<<~RUBY)
+        x.to_h { |k, v| [k.to_sym, foo(v)] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is an array literal' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].to_h { |k, v| [k, foo(v)] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is `each_with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each_with_index.to_h { |k, v| [k, foo(v)] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is `with_index`' do
+      expect_no_offenses(<<~RUBY)
+        [1, 2, 3].each.with_index.to_h { |k, v| [k, foo(v)] }
+      RUBY
+    end
+
+    it 'does not flag `_.to_h{...}` when its receiver is `zip`' do
+      expect_no_offenses(<<~RUBY)
+        %i[a b c].zip([1, 2, 3]).to_h { |k, v| [k, foo(v)] }
+      RUBY
+    end
   end
 
   context 'below Ruby 2.6', :ruby25 do