From 2103446025901cf3056b240d755dbc944a1ef926 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 31 Jan 2024 18:46:12 +0900 Subject: [PATCH] [Fix #437] Fix a false positive for `Performance/ChainArrayAllocation` Fixes #437. This PR fixes a false positive for `Performance/ChainArrayAllocation` when using `select` with block argument after `select`. --- ..._for_performance_chain_array_allocation.md | 1 + .../cop/performance/chain_array_allocation.rb | 11 +++++- .../chain_array_allocation_spec.rb | 34 +++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 changelog/fix_a_false_positive_for_performance_chain_array_allocation.md diff --git a/changelog/fix_a_false_positive_for_performance_chain_array_allocation.md b/changelog/fix_a_false_positive_for_performance_chain_array_allocation.md new file mode 100644 index 0000000000..c9fce523b4 --- /dev/null +++ b/changelog/fix_a_false_positive_for_performance_chain_array_allocation.md @@ -0,0 +1 @@ +* [#437](https://github.com/rubocop/rubocop-performance/issues/437): Fix a false positive for `Performance/ChainArrayAllocation` when using `select` with block argument after `select`. ([@koic][]) diff --git a/lib/rubocop/cop/performance/chain_array_allocation.rb b/lib/rubocop/cop/performance/chain_array_allocation.rb index c19bbc9fe3..456a14c87f 100644 --- a/lib/rubocop/cop/performance/chain_array_allocation.rb +++ b/lib/rubocop/cop/performance/chain_array_allocation.rb @@ -54,7 +54,7 @@ class ChainArrayAllocation < Base def_node_matcher :chain_array_allocation?, <<~PATTERN (send { (send _ $%RETURN_NEW_ARRAY_WHEN_ARGS {int lvar ivar cvar gvar send}) - (block (send _ $%ALWAYS_RETURNS_NEW_ARRAY) ...) + ({block numblock} (send _ $%ALWAYS_RETURNS_NEW_ARRAY) ...) (send _ $%RETURNS_NEW_ARRAY ...) } $%HAS_MUTATION_ALTERNATIVE ...) PATTERN @@ -62,12 +62,21 @@ class ChainArrayAllocation < Base def on_send(node) chain_array_allocation?(node) do |fm, sm| return if node.each_descendant(:send).any? { |descendant| descendant.method?(:lazy) } + return if node.method?(:select) && !enumerable_select_method?(node.receiver) range = range_between(node.loc.dot.begin_pos, node.source_range.end_pos) add_offense(range, message: format(MSG, method: fm, second_method: sm)) end end + + private + + def enumerable_select_method?(node) + # NOTE: `QueryMethods#select` in Rails accepts positional arguments, whereas `Enumerable#select` does not. + # This difference can be utilized to reduce the knowledge requirements related to `select`. + (node.block_type? || node.numblock_type?) && node.send_node.arguments.empty? + end end end end diff --git a/spec/rubocop/cop/performance/chain_array_allocation_spec.rb b/spec/rubocop/cop/performance/chain_array_allocation_spec.rb index c2f7b1dbc3..670722e4ba 100644 --- a/spec/rubocop/cop/performance/chain_array_allocation_spec.rb +++ b/spec/rubocop/cop/performance/chain_array_allocation_spec.rb @@ -53,4 +53,38 @@ RUBY end end + + describe 'when using `select` with block argument after `select` with positional arguments' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + model.select(:foo, :bar).select { |item| item.do_something } + RUBY + end + end + + describe 'when using `select` with block argument after `select` with block argument' do + it 'registers an offense' do + expect_offense(<<~RUBY) + model.select { |item| item.foo }.select { |item| item.bar } + ^^^^^^^ Use unchained `select` and `select!` (followed by `return array` if required) instead of chaining `select...select`. + RUBY + end + end + + describe 'when using `select` with block argument after `select` with numbered block argument' do + it 'registers an offense' do + expect_offense(<<~RUBY) + model.select { _1.foo }.select { |item| item.bar } + ^^^^^^^ Use unchained `select` and `select!` (followed by `return array` if required) instead of chaining `select...select`. + RUBY + end + end + + describe 'when using `select` with positional arguments after `select`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + model.select(:foo, :bar).select(:baz, :qux) + RUBY + end + end end