From b06bfaff032316dbe86ba2050fa6890a096edaaf Mon Sep 17 00:00:00 2001 From: Dave Corson-Knowles Date: Mon, 16 Sep 2024 16:43:47 -0700 Subject: [PATCH] Add new ZipForArrayWrapping cop Add new `Performance/ZipForArrayWrapping` cop that checks patterns like `.map { |id| [id] }` or `.map { [_1] }` and can safely replace them with `.zip` This is a Performance Cop for the more efficient way to generate an Array of Arrays. * Performs 40-90% faster than `.map` to iteratively wrap array contents. * Performs 5 - 55% faster on ranges, depending on size. --- changelog/new_merge_pull_request_462_from.md | 1 + config/default.yml | 5 + .../cop/performance/zip_for_array_wrapping.rb | 81 +++++ lib/rubocop/cop/performance_cops.rb | 1 + .../zip_for_array_wrapping_spec.rb | 307 ++++++++++++++++++ 5 files changed, 395 insertions(+) create mode 100644 changelog/new_merge_pull_request_462_from.md create mode 100644 lib/rubocop/cop/performance/zip_for_array_wrapping.rb create mode 100644 spec/rubocop/cop/performance/zip_for_array_wrapping_spec.rb diff --git a/changelog/new_merge_pull_request_462_from.md b/changelog/new_merge_pull_request_462_from.md new file mode 100644 index 0000000000..106ecb872b --- /dev/null +++ b/changelog/new_merge_pull_request_462_from.md @@ -0,0 +1 @@ +* [#462](https://github.com/rubocop/rubocop-performance/pull/462): Add new `Performance/ZipForArrayWrapping` cop that checks patterns like `.map { |id| [id] }` or `.map { [_1] }` and can safely replace them with `.zip`. ([@corsonknowles][]) diff --git a/config/default.yml b/config/default.yml index 95dc830898..0d1f812245 100644 --- a/config/default.yml +++ b/config/default.yml @@ -372,3 +372,8 @@ Performance/UriDefaultParser: Description: 'Use `URI::DEFAULT_PARSER` instead of `URI::Parser.new`.' Enabled: true VersionAdded: '0.50' + +Performance/ZipForArrayWrapping: + Description: 'Checks for `.map { |id| [id] }` and suggests replacing it with `.zip`.' + Enabled: pending + VersionAdded: <> diff --git a/lib/rubocop/cop/performance/zip_for_array_wrapping.rb b/lib/rubocop/cop/performance/zip_for_array_wrapping.rb new file mode 100644 index 0000000000..0a56ea980a --- /dev/null +++ b/lib/rubocop/cop/performance/zip_for_array_wrapping.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # Checks for `.map { |id| [id] }` and suggests replacing it with `.zip`. + # + # @example + # # bad + # [1, 2, 3].map { |id| [id] } + # + # # bad + # [1, 2, 3].map { [_1] } + # + # # good + # [1, 2, 3].zip + # + # @example + # # good (no offense) + # [1, 2, 3].map { |id| id } + # + # @example + # # good (no offense) + # [1, 2, 3].map { |id| [id, id] } + class ZipForArrayWrapping < Base + include RangeHelp + extend AutoCorrector + + MSG = 'Use `zip` instead of `%s`.' + RESTRICT_ON_SEND = Set.new(%i[map collect]).freeze + + # Matches regular block form `.map { |e| [e] }` + # @!method map_with_array?(node) + def_node_matcher :map_with_array?, <<~PATTERN + (block + (send _ RESTRICT_ON_SEND) + (args (arg _id)) + (array (lvar _id))) + PATTERN + + # Matches numblock form `.map { [_1] }` + # @!method map_with_array_numblock?(node) + def_node_matcher :map_with_array_numblock?, <<~PATTERN + (numblock + (send _ :map) + 1 + (array (lvar _)) + ) + PATTERN + + def on_send(node) + return unless (parent = node.parent) + return unless map_with_array?(parent) || map_with_array_numblock?(parent) + return unless (receiver = node.receiver&.source) + + register_offense(parent, receiver, node) + end + + private + + def register_offense(parent, receiver, node) + add_offense(offense_range(parent), message: message(node)) do |corrector| + autocorrect(parent, receiver, corrector) + end + end + + def message(node) + format(MSG, original_code: offense_range(node).source.lines.first.chomp) + end + + def autocorrect(parent, receiver, corrector) + corrector.replace(parent, "#{receiver}.zip") + end + + def offense_range(node) + @offense_range ||= range_between(node.children.first.loc.selector.begin_pos, node.loc.end.end_pos) + end + end + end + end +end diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 2a18f26120..b922d8cb73 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -53,3 +53,4 @@ require_relative 'performance/unfreeze_string' require_relative 'performance/uri_default_parser' require_relative 'performance/chain_array_allocation' +require_relative 'performance/zip_for_array_wrapping' diff --git a/spec/rubocop/cop/performance/zip_for_array_wrapping_spec.rb b/spec/rubocop/cop/performance/zip_for_array_wrapping_spec.rb new file mode 100644 index 0000000000..bd478486d2 --- /dev/null +++ b/spec/rubocop/cop/performance/zip_for_array_wrapping_spec.rb @@ -0,0 +1,307 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::ZipForArrayWrapping, :config do + context 'when using map with array literal' do + it 'registers an offense and corrects to use zip with no arguments' do + expect_offense(<<~RUBY) + [1, 2, 3].map { |id| [id] } + ^^^^^^^^^^^^^^^^^ Use `zip` instead of `map { |id| [id] }`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].zip + RUBY + end + end + + context 'when using map with a short iterator name' do + it 'registers an offense and corrects to use zip with no arguments' do + expect_offense(<<~RUBY) + [1, 2, 3].map { |e| [e] } + ^^^^^^^^^^^^^^^ Use `zip` instead of `map { |e| [e] }`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].zip + RUBY + end + end + + context 'when using map on a range with another iterator name' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + (1..3).map { |x| [x] } + ^^^^^^^^^^^^^^^ Use `zip` instead of `map { |x| [x] }`. + RUBY + + expect_correction(<<~RUBY) + (1..3).zip + RUBY + end + end + + context 'when using map in a do end block' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + (a..b).map do + ^^^^^^ Use `zip` instead of `map do`. + |m| [m] + end + RUBY + + expect_correction(<<~RUBY) + (a..b).zip + RUBY + end + end + + context 'when using map in a chain' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + [nil, tuple].flatten.map { |e| [e] }.call + ^^^^^^^^^^^^^^^ Use `zip` instead of `map { |e| [e] }`. + RUBY + + expect_correction(<<~RUBY) + [nil, tuple].flatten.zip.call + RUBY + end + end + + context 'when the map block does not contain an array literal' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |id| id } + RUBY + end + end + + context 'when using map with an array literal containing multiple elements' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |id| [id, id] } + RUBY + end + end + + context 'when using other iterators such as' do + context 'when using collect' do + it 'registers an offense as collect is an alias of map' do + expect_offense(<<~RUBY) + [1, 2, 3].collect { |id| [id] } + ^^^^^^^^^^^^^^^^^^^^^ Use `zip` instead of `collect { |id| [id] }`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].zip + RUBY + end + end + + context 'when using select with an array literal' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].select { |id| [id] } + RUBY + end + end + + context 'when calling filter_map' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].filter_map {|id| [id]} + RUBY + end + end + + context 'when calling flat_map' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].flat_map {|id| [id]} + RUBY + end + end + end + + context 'when using map with doubly wrapped arrays' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |id| [[id]] } + RUBY + end + end + + context 'when using map with addition' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |id| id + 1 } + RUBY + end + end + + context 'when using map with array addition' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |id| [id] + [id] } + RUBY + end + end + + context 'when using map with indexing into an array' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |id| [id][id] } + RUBY + end + end + + context 'when calling map with no arguments' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map + RUBY + end + end + + context 'when calling map with an empty block' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map {} + RUBY + end + end + + context 'with related array of array patterns' do + context 'when using [*foo] to dynamically wrap only non-arrays in the list' do + it 'does not register an offense since the map is doing useful work' do + expect_no_offenses(<<~RUBY) + [1, [2], 3].map { |id| [*id] } + RUBY + end + end + + context 'when using Array.wrap the Rails extension of the [*foo] pattern that handles Hashes' do + it 'does not register an offense since the map is doing useful work' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |id| Array.wrap(id) } + RUBY + end + end + + context 'when making an array of arrays using each_with_object' do + it 'does not register an offense since we have not included this pattern yet' do + expect_no_offenses(<<~RUBY) + [1,2,3].each_with_object([]) {|id, object| object << [id]} + RUBY + end + end + end + + context 'when using map with a numerical argument' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + [1, 2, 3].map { [_1] } + ^^^^^^^^^^^^ Use `zip` instead of `map { [_1] }`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].zip + RUBY + end + end + + context 'when using map with a numblock in a chain' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + [1, 2].sum.map { [_1] }.flatten + ^^^^^^^^^^^^ Use `zip` instead of `map { [_1] }`. + RUBY + + expect_correction(<<~RUBY) + [1, 2].sum.zip.flatten + RUBY + end + end + + context 'when using map on a range with a numblock' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + (1..3).map { [_1] } + ^^^^^^^^^^^^ Use `zip` instead of `map { [_1] }`. + RUBY + + expect_correction(<<~RUBY) + (1..3).zip + RUBY + end + end + + context 'when using map in a do end block with a numblock' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + (a..b).map do [_1] end + ^^^^^^^^^^^^^^^ Use `zip` instead of `map do [_1] end`. + RUBY + + expect_correction(<<~RUBY) + (a..b).zip + RUBY + end + end + + context 'when calling filter_map with a numblock' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].filter_map { [_1] } + RUBY + end + end + + context 'when calling map, adding, and wrapping, with a numblock' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { [_1 + 1] } + RUBY + end + end + + context 'when calling double wrapping with a numblock' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { [[_1]] } + RUBY + end + end + + context 'when calling map with an unused iterator' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { |e| [1] } + RUBY + end + end + + context 'when calling map with a static block that always returns the same value' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { [id] } + RUBY + end + end + + context 'when calling map with a static array' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].map { [] } + RUBY + end + end + + context 'when map has no receiver' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + map { |id| [id] } + RUBY + end + end +end