Merge pull request #364 from koic/add_new_performance_map_method_chai…

…n_cop

Add new `Performance/MapMethodChain` cop

changelog/new_add_new_performance_map_method_chain_cop.md

@@ -0,0 +1 @@
+* [#364](https://github.com/rubocop/rubocop-performance/pull/364): Add new `Performance/MapMethodChain` cop. ([@koic][])

config/default.yml

@@ -193,6 +193,12 @@ Performance/MapCompact:
   SafeAutoCorrect: false
   VersionAdded: '1.11'
 
+Performance/MapMethodChain:
+  Description: 'Checks if the `map` method is used in a chain.'
+  Enabled: pending
+  Safe: false
+  VersionAdded: '<<next>>'
+
 Performance/MethodObjectAsBlock:
   Description: 'Use block explicitly instead of block-passing a method object.'
   Reference: 'https://github.com/JuanitoFatas/fast-ruby#normal-way-to-apply-method-vs-method-code'

lib/rubocop/cop/performance/map_method_chain.rb

@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Performance
+      # Checks if the map method is used in a chain.
+      #
+      # Autocorrection is not supported because an appropriate block variable name cannot be determined automatically.
+      #
+      # @safety
+      #   This cop is unsafe because false positives occur if the number of times the first method is executed
+      #   affects the return value of subsequent methods.
+      #
+      # [source,ruby]
+      # ----
+      # class X
+      #   def initialize
+      #     @@num = 0
+      #   end
+      #
+      #   def foo
+      #     @@num += 1
+      #     self
+      #   end
+      #
+      #   def bar
+      #     @@num * 2
+      #   end
+      # end
+      #
+      # [X.new, X.new].map(&:foo).map(&:bar) # => [4, 4]
+      # [X.new, X.new].map { |x| x.foo.bar } # => [2, 4]
+      # ----
+      #
+      # @example
+      #
+      #   # bad
+      #   array.map(&:foo).map(&:bar)
+      #
+      #   # good
+      #   array.map { |item| item.foo.bar }
+      #
+      class MapMethodChain < Base
+        include IgnoredNode
+
+        MSG = 'Use `%<method_name>s { |x| x.%<map_args>s }` instead of `%<method_name>s` method chain.'
+        RESTRICT_ON_SEND = %i[map collect].freeze
+
+        def_node_matcher :block_pass_with_symbol_arg?, <<~PATTERN
+          (:block_pass (:sym $_))
+        PATTERN
+
+        def on_send(node)
+          return if part_of_ignored_node?(node)
+          return unless (map_arg = block_pass_with_symbol_arg?(node.first_argument))
+
+          map_args = [map_arg]
+
+          return unless (begin_of_chained_map_method = find_begin_of_chained_map_method(node, map_args))
+
+          range = begin_of_chained_map_method.loc.selector.begin.join(node.source_range.end)
+          message = format(MSG, method_name: begin_of_chained_map_method.method_name, map_args: map_args.join('.'))
+
+          add_offense(range, message: message)
+
+          ignore_node(node)
+        end
+
+        private
+
+        def find_begin_of_chained_map_method(node, map_args)
+          return unless (chained_map_method = node.receiver)
+          return if !chained_map_method.call_type? || !RESTRICT_ON_SEND.include?(chained_map_method.method_name)
+          return unless (map_arg = block_pass_with_symbol_arg?(chained_map_method.first_argument))
+
+          map_args.unshift(map_arg)
+
+          receiver = chained_map_method.receiver
+
+          return chained_map_method unless receiver.call_type? && block_pass_with_symbol_arg?(receiver.first_argument)
+
+          find_begin_of_chained_map_method(chained_map_method, map_args)
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/performance_cops.rb

@@ -25,6 +25,7 @@
 require_relative 'performance/flat_map'
 require_relative 'performance/inefficient_hash_search'
 require_relative 'performance/map_compact'
+require_relative 'performance/map_method_chain'
 require_relative 'performance/method_object_as_block'
 require_relative 'performance/open_struct'
 require_relative 'performance/range_include'

spec/rubocop/cop/performance/map_method_chain_spec.rb

@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Performance::MapMethodChain, :config do
+  it 'registers an offense when using `map` method chain and receiver is a method call' do
+    expect_offense(<<~RUBY)
+      array.map(&:foo).map(&:bar)
+            ^^^^^^^^^^^^^^^^^^^^^ Use `map { |x| x.foo.bar }` instead of `map` method chain.
+    RUBY
+  end
+
+  it 'registers an offense when using `collect` method chain and receiver is a method call' do
+    expect_offense(<<~RUBY)
+      array.collect(&:foo).collect(&:bar)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `collect { |x| x.foo.bar }` instead of `collect` method chain.
+    RUBY
+  end
+
+  it 'registers an offense when using `map` and `collect` method chain and receiver is a method call' do
+    expect_offense(<<~RUBY)
+      array.map(&:foo).collect(&:bar)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map { |x| x.foo.bar }` instead of `map` method chain.
+    RUBY
+  end
+
+  it 'registers an offense when using `map` method chain and receiver is a variable' do
+    expect_offense(<<~RUBY)
+      array = create_array
+      array.map(&:foo).map(&:bar)
+            ^^^^^^^^^^^^^^^^^^^^^ Use `map { |x| x.foo.bar }` instead of `map` method chain.
+    RUBY
+  end
+
+  it 'registers an offense when using `map` method chain repeated three times' do
+    expect_offense(<<~RUBY)
+      array.map(&:foo).map(&:bar).map(&:baz)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map { |x| x.foo.bar.baz }` instead of `map` method chain.
+    RUBY
+  end
+
+  it 'registers an offense when using `map` method chain repeated three times with safe navigation' do
+    expect_offense(<<~RUBY)
+      array&.map(&:foo).map(&:bar).map(&:baz)
+             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map { |x| x.foo.bar.baz }` instead of `map` method chain.
+    RUBY
+  end
+
+  it 'does not register an offense when using `do_something` method chain and receiver is a method call' do
+    expect_no_offenses(<<~RUBY)
+      array.do_something(&:foo).do_something(&:bar)
+    RUBY
+  end
+
+  it 'does not register an offense when there is a method call between `map` method chain' do
+    expect_no_offenses(<<~RUBY)
+      array.map(&:foo).do_something.map(&:bar)
+    RUBY
+  end
+
+  it 'does not register an offense when using `flat_map` and `map` method chain and receiver is a method call' do
+    expect_no_offenses(<<~RUBY)
+      array.flat_map(&:foo).map(&:bar)
+    RUBY
+  end
+
+  it 'does not register an offense when using `map(&:foo).join(', ')`' do
+    expect_no_offenses(<<~RUBY)
+      array = create_array
+      array.map(&:foo).join(', ')
+    RUBY
+  end
+
+  it 'does not register an offense when using `map(&:foo).join(', ')` with safe navigation' do
+    expect_no_offenses(<<~RUBY)
+      array = create_array
+      array.map(&:foo).join(', ')
+    RUBY
+  end
+end