From 11d3c61927e26c63c8fe8cbf20123a6f2e80fe19 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 15 Oct 2020 13:44:23 +0900 Subject: [PATCH] [Fix #178] Add new `Performance/FilterMap` cop Fixes #178. This PR adds new `Performance/FilterMap` cop. In Ruby 2.7, `Enumerable#filter_map` has been added. This cop identifies places where `select.map` can be replaced by `filter_map`. And it allows `map { ... }.compact` that is not compatible with `filter_map`. ```ruby [true, false, nil].compact #=> [true, false] [true, false, nil].filter_map(&:itself) #=> [true] ``` The following is good and bad cases. ```ruby # bad ary.select(&:foo).map(&:bar) ary.filter(&:foo).map(&:bar) # good ary.filter_map { |o| o.bar if o.foo } ary.map(&:foo).compact # Consider whether you can safely replace it with `filter_map`. ``` --- CHANGELOG.md | 1 + config/default.yml | 5 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_performance.adoc | 39 +++++++ lib/rubocop/cop/performance/filter_map.rb | 69 +++++++++++ lib/rubocop/cop/performance_cops.rb | 1 + .../cop/performance/filter_map_spec.rb | 110 ++++++++++++++++++ 7 files changed, 226 insertions(+) create mode 100644 lib/rubocop/cop/performance/filter_map.rb create mode 100644 spec/rubocop/cop/performance/filter_map_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index a73d4c37e8..e4aa4d7691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### New features * [#190](https://github.com/rubocop-hq/rubocop-performance/pull/190): Add new `Performance/RedundantSplitRegexpArgument` cop. ([@mfbmina][]) +* [#178](https://github.com/rubocop-hq/rubocop-performance/issues/178): Add new `Performance/FilterMap` cop. ([@koic][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index cdfe2bffad..1aad268591 100644 --- a/config/default.yml +++ b/config/default.yml @@ -143,6 +143,11 @@ Performance/EndWith: VersionAdded: '0.36' VersionChanged: '1.6' +Performance/FilterMap: + Description: 'Use `Enumerable#filter_map`.' + Enabled: pending + VersionAdded: '1.10' + Performance/FixedSize: Description: 'Do not compute the size of statically sized objects except in constants.' Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 53be13465b..55da6c41c5 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -30,6 +30,7 @@ Performance cops optimization analysis for your projects. * xref:cops_performance.adoc#performancedetect[Performance/Detect] * xref:cops_performance.adoc#performancedoublestartendwith[Performance/DoubleStartEndWith] * xref:cops_performance.adoc#performanceendwith[Performance/EndWith] +* xref:cops_performance.adoc#performancefiltermap[Performance/FilterMap] * xref:cops_performance.adoc#performancefixedsize[Performance/FixedSize] * xref:cops_performance.adoc#performanceflatmap[Performance/FlatMap] * xref:cops_performance.adoc#performanceinefficienthashsearch[Performance/InefficientHashSearch] diff --git a/docs/modules/ROOT/pages/cops_performance.adoc b/docs/modules/ROOT/pages/cops_performance.adoc index 340c0e5b20..d57abf7ace 100644 --- a/docs/modules/ROOT/pages/cops_performance.adoc +++ b/docs/modules/ROOT/pages/cops_performance.adoc @@ -849,6 +849,45 @@ for receiver is multiline string. * https://github.com/JuanitoFatas/fast-ruby#stringmatch-vs-stringstart_withstringend_with-code-start-code-end +== Performance/FilterMap + +NOTE: Required Ruby version: 2.7 + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| No +| 1.10 +| - +|=== + +In Ruby 2.7, `Enumerable#filter_map` has been added. + +This cop identifies places where `select.map` can be replaced by `filter_map`. + +And it allows `map { ... }.compact` that is not compatible with `filter_map`. + +[source,ruby] +---- +[true, false, nil].compact #=> [true, false] +[true, false, nil].filter_map(&:itself) #=> [true] +---- + +=== Examples + +[source,ruby] +---- +# bad +ary.select(&:foo).map(&:bar) +ary.filter(&:foo).map(&:bar) + +# good +ary.filter_map { |o| o.bar if o.foo } +ary.map(&:foo).compact # Consider whether you can safely replace it with `filter_map`. +---- + == Performance/FixedSize |=== diff --git a/lib/rubocop/cop/performance/filter_map.rb b/lib/rubocop/cop/performance/filter_map.rb new file mode 100644 index 0000000000..4978efcaed --- /dev/null +++ b/lib/rubocop/cop/performance/filter_map.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # In Ruby 2.7, `Enumerable#filter_map` has been added. + # + # This cop identifies places where `select.map` can be replaced by `filter_map`. + # + # And it allows `map { ... }.compact` that is not compatible with `filter_map`. + # + # [source,ruby] + # ---- + # [true, false, nil].compact #=> [true, false] + # [true, false, nil].filter_map(&:itself) #=> [true] + # ---- + # + # @example + # # bad + # ary.select(&:foo).map(&:bar) + # ary.filter(&:foo).map(&:bar) + # + # # good + # ary.filter_map { |o| o.bar if o.foo } + # ary.map(&:foo).compact # Consider whether you can safely replace it with `filter_map`. + # + class FilterMap < Base + include RangeHelp + extend TargetRubyVersion + + minimum_target_ruby_version 2.7 + + MSG = 'Use `filter_map` instead of `%s.map`.' + RESTRICT_ON_SEND = %i[select filter].freeze + + def_node_matcher :bad_method?, <<~PATTERN + (send nil? :bad_method ...) + PATTERN + + def on_send(node) + return if (first_argument = node.first_argument) && !first_argument.block_pass_type? + return unless (send_node = map_method_candidate(node)) + return unless send_node.method?(:map) + + map_method = send_node.parent&.block_type? ? send_node.parent : send_node + + range = offense_range(node, map_method) + add_offense(range, message: format(MSG, method_name: node.method_name)) + end + + private + + def map_method_candidate(node) + return unless (parent = node.parent) + + if parent.block_type? && parent.parent&.send_type? + parent.parent + elsif parent.send_type? + parent + end + end + + def offense_range(node, map_method) + range_between(node.loc.selector.begin_pos, map_method.loc.expression.end_pos) + end + end + end + end +end diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 8e58611610..80eb0d7091 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -20,6 +20,7 @@ require_relative 'performance/detect' require_relative 'performance/double_start_end_with' require_relative 'performance/end_with' +require_relative 'performance/filter_map' require_relative 'performance/fixed_size' require_relative 'performance/flat_map' require_relative 'performance/inefficient_hash_search' diff --git a/spec/rubocop/cop/performance/filter_map_spec.rb b/spec/rubocop/cop/performance/filter_map_spec.rb new file mode 100644 index 0000000000..fccb2a2c00 --- /dev/null +++ b/spec/rubocop/cop/performance/filter_map_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::FilterMap, :config do + context 'TargetRubyVersion >= 2.7', :ruby27 do + it 'registers an offense when using `select(&:...).map(&:...)`' do + expect_offense(<<~RUBY) + ary.select(&:present?).map(&:to_i) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `filter_map` instead of `select.map`. + RUBY + end + + it 'registers an offense when using `filter(&:...).map(&:...)`' do + expect_offense(<<~RUBY) + ary.filter(&:present?).map(&:to_i) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `filter_map` instead of `filter.map`. + RUBY + end + + it 'registers an offense when using `select { ... }.map { ... }`' do + expect_offense(<<~RUBY) + ary.select { |o| o.present? }.map { |o| o.to_i } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `filter_map` instead of `select.map`. + RUBY + end + + it 'registers an offense when using `select { ... }.map(&:...)`' do + expect_offense(<<~RUBY) + ary.select { |o| o.present? }.map(&:to_i) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `filter_map` instead of `select.map`. + RUBY + end + + it 'registers an offense when using `select(&:...).map { ... }`' do + expect_offense(<<~RUBY) + ary.select(&:present?).map { |o| o.to_i } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `filter_map` instead of `select.map`. + RUBY + end + + it 'registers an offense when using `select(&:...).map(&:...)` in method chain' do + expect_offense(<<~RUBY) + ary.do_something.select(&:present?).map(&:to_i).max + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `filter_map` instead of `select.map`. + RUBY + end + + it 'registers an offense when using `select(&:...).map(&:...)` without receiver' do + expect_offense(<<~RUBY) + select(&:present?).map(&:to_i) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `filter_map` instead of `select.map`. + RUBY + end + + it 'does not register an offense when using `filter_map`' do + expect_no_offenses(<<~RUBY) + ary.filter_map { |o| o.to_i if o.present? } + RUBY + end + + it 'does not register an offense when using `select`' do + expect_no_offenses(<<~RUBY) + ary.select(&:present?) + RUBY + end + + it 'does not register an offense when using `select(key: value).map`' do + expect_no_offenses(<<~RUBY) + ary.do_something.select(key: value).map(&:to_i) + RUBY + end + + it 'does not register an offense when using `select` with assignment' do + expect_no_offenses(<<~RUBY) + ret = select + RUBY + end + + it 'does not register an offense when using `select(&:...).stranger.map(&:...)`' do + expect_no_offenses(<<~RUBY) + ary.do_something.select(&:present?).stranger.map(&:to_i).max + RUBY + end + + it 'does not register an offense when using `select { ... }.stranger.map { ... }`' do + expect_no_offenses(<<~RUBY) + ary.select { |o| o.present? }.stranger.map { |o| o.to_i } + RUBY + end + + # + # `compact` is not compatible with `filter_map`. + # + # [true, false, nil].compact #=> [true, false] + # [true, false, nil].filter_map(&:itself) #=> [true] + # + it 'does not register an offense when using `map(&:...).compact`' do + expect_no_offenses(<<~RUBY) + ary.map(&:to_i).compact + RUBY + end + end + + context 'TargetRubyVersion <= 2.6', :ruby26 do + it 'does not register an offense when using `select.map`' do + expect_no_offenses(<<~RUBY) + ary.select(&:present?).map(&:to_i) + RUBY + end + end +end