Skip to content

Commit

Permalink
Add new Performance/MapMethodChain cop
Browse files Browse the repository at this point in the history
This PR adds new `Performance/MapMethodChain` cop that checks if the map method is used in a chain.

```ruby
# bad
array.map(&:foo).map(&:bar)

# good
array.map { |item| item.foo.bar }
```

## 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.

```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]
```
## Benchmark

```console
$ cat map_chain.rb
#!/usr/local/bin/ruby

require 'benchmark/ips'

Benchmark.ips do |x|
ary = %i[hi bye]

x.report('map.map') { ary.map(&:to_s).map(&:length) }
x.report('map')     { ary.map { |item| item.to_s.length } }

x.compare!
end
```

```console
$ ruby -v
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-darwin19]

$ ruby map_chain.rb
Warming up --------------------------------------
map.map   215.877k i/100ms
map   319.746k i/100ms
Calculating -------------------------------------
map.map      2.116M (± 2.1%) i/s -     10.578M in   5.002379s
map      3.227M (± 2.8%) i/s -     16.307M in   5.058091s

Comparison:
map:  3226860.1 i/s
map.map:  2115551.0 i/s - 1.53x  slower
```
  • Loading branch information
koic committed Aug 11, 2023
1 parent d7a141d commit 42c78bc
Show file tree
Hide file tree
Showing 5 changed files with 173 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/new_add_new_performance_map_method_chain_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#364](https://github.com/rubocop/rubocop-performance/pull/364): Add new `Performance/MapMethodChain` cop. ([@koic][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
87 changes: 87 additions & 0 deletions lib/rubocop/cop/performance/map_method_chain.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions lib/rubocop/cop/performance_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
78 changes: 78 additions & 0 deletions spec/rubocop/cop/performance/map_method_chain_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 42c78bc

Please sign in to comment.