From 42c78bc62e4537a0fbd6e8c04628d42e7afcb312 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 12 Aug 2023 02:19:22 +0900 Subject: [PATCH] Add new `Performance/MapMethodChain` cop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ``` --- ...dd_new_performance_map_method_chain_cop.md | 1 + config/default.yml | 6 ++ .../cop/performance/map_method_chain.rb | 87 +++++++++++++++++++ lib/rubocop/cop/performance_cops.rb | 1 + .../cop/performance/map_method_chain_spec.rb | 78 +++++++++++++++++ 5 files changed, 173 insertions(+) create mode 100644 changelog/new_add_new_performance_map_method_chain_cop.md create mode 100644 lib/rubocop/cop/performance/map_method_chain.rb create mode 100644 spec/rubocop/cop/performance/map_method_chain_spec.rb diff --git a/changelog/new_add_new_performance_map_method_chain_cop.md b/changelog/new_add_new_performance_map_method_chain_cop.md new file mode 100644 index 0000000000..c66d071e06 --- /dev/null +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index 6993c89521..68c498dc15 100644 --- a/config/default.yml +++ b/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: '<>' + 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' diff --git a/lib/rubocop/cop/performance/map_method_chain.rb b/lib/rubocop/cop/performance/map_method_chain.rb new file mode 100644 index 0000000000..db49180f0b --- /dev/null +++ b/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 `%s { |x| x.%s }` instead of `%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 diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 02a64a6300..2a18f26120 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/performance/map_method_chain_spec.rb b/spec/rubocop/cop/performance/map_method_chain_spec.rb new file mode 100644 index 0000000000..1e1dadf722 --- /dev/null +++ b/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