From a60cafa988b93f354c9488ab3d161a1c4da85d38 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 12 Aug 2022 22:09:00 +0900 Subject: [PATCH] Support numbered parameter for `RedundantSortBlock`, `SortReverse`, and `TimesMap` This PR support numbered parameter for `Performance/RedundantSortBlock`, `Performance/SortReverse`, and `Performance/TimesMap` cops. --- changelog/new_support_numbered_parameter.md | 1 + lib/rubocop/cop/mixin/sort_block.rb | 7 +++++ .../cop/performance/redundant_sort_block.rb | 24 +++++++++++------ lib/rubocop/cop/performance/sort_reverse.rb | 27 ++++++++++++------- lib/rubocop/cop/performance/times_map.rb | 3 ++- .../performance/redundant_sort_block_spec.rb | 27 ++++++++++++++++++- .../cop/performance/sort_reverse_spec.rb | 27 ++++++++++++++++++- .../rubocop/cop/performance/times_map_spec.rb | 13 +++++++++ 8 files changed, 109 insertions(+), 20 deletions(-) create mode 100644 changelog/new_support_numbered_parameter.md diff --git a/changelog/new_support_numbered_parameter.md b/changelog/new_support_numbered_parameter.md new file mode 100644 index 0000000000..c88196ddfe --- /dev/null +++ b/changelog/new_support_numbered_parameter.md @@ -0,0 +1 @@ +* [#305](https://github.com/rubocop/rubocop-performance/pull/305): Support numbered parameter for `Performance/RedundantSortBlock`, `Performance/SortReverse`, and `Performance/TimesMap` cops. ([@koic][]) diff --git a/lib/rubocop/cop/mixin/sort_block.rb b/lib/rubocop/cop/mixin/sort_block.rb index 8a287b9c4d..cc62dc1683 100644 --- a/lib/rubocop/cop/mixin/sort_block.rb +++ b/lib/rubocop/cop/mixin/sort_block.rb @@ -14,6 +14,13 @@ module SortBlock $send) PATTERN + def_node_matcher :sort_with_numblock?, <<~PATTERN + (numblock + $(send _ :sort) + $_arg_count + $send) + PATTERN + def_node_matcher :replaceable_body?, <<~PATTERN (send (lvar %1) :<=> (lvar %2)) PATTERN diff --git a/lib/rubocop/cop/performance/redundant_sort_block.rb b/lib/rubocop/cop/performance/redundant_sort_block.rb index 55c3af49cb..ca5481039d 100644 --- a/lib/rubocop/cop/performance/redundant_sort_block.rb +++ b/lib/rubocop/cop/performance/redundant_sort_block.rb @@ -16,25 +16,33 @@ class RedundantSortBlock < Base include SortBlock extend AutoCorrector - MSG = 'Use `sort` instead of `%s`.' + MSG = 'Use `sort` without block.' def on_block(node) return unless (send, var_a, var_b, body = sort_with_block?(node)) replaceable_body?(body, var_a, var_b) do - range = sort_range(send, node) + register_offense(send, node) + end + end + + def on_numblock(node) + return unless (send, arg_count, body = sort_with_numblock?(node)) + return unless arg_count == 2 - add_offense(range, message: message(var_a, var_b)) do |corrector| - corrector.replace(range, 'sort') - end + replaceable_body?(body, :_1, :_2) do + register_offense(send, node) end end private - def message(var_a, var_b) - bad_method = "sort { |#{var_a}, #{var_b}| #{var_a} <=> #{var_b} }" - format(MSG, bad_method: bad_method) + def register_offense(send, node) + range = sort_range(send, node) + + add_offense(range) do |corrector| + corrector.replace(range, 'sort') + end end end end diff --git a/lib/rubocop/cop/performance/sort_reverse.rb b/lib/rubocop/cop/performance/sort_reverse.rb index 301906804e..524e03b23d 100644 --- a/lib/rubocop/cop/performance/sort_reverse.rb +++ b/lib/rubocop/cop/performance/sort_reverse.rb @@ -17,27 +17,36 @@ class SortReverse < Base include SortBlock extend AutoCorrector - MSG = 'Use `sort.reverse` instead of `%s`.' + MSG = 'Use `sort.reverse` instead.' def on_block(node) sort_with_block?(node) do |send, var_a, var_b, body| replaceable_body?(body, var_b, var_a) do - range = sort_range(send, node) + register_offense(send, node) + end + end + end - add_offense(range, message: message(var_a, var_b)) do |corrector| - replacement = 'sort.reverse' + def on_numblock(node) + sort_with_numblock?(node) do |send, arg_count, body| + next unless arg_count == 2 - corrector.replace(range, replacement) - end + replaceable_body?(body, :_2, :_1) do + register_offense(send, node) end end end private - def message(var_a, var_b) - bad_method = "sort { |#{var_a}, #{var_b}| #{var_b} <=> #{var_a} }" - format(MSG, bad_method: bad_method) + def register_offense(send, node) + range = sort_range(send, node) + + add_offense(range) do |corrector| + replacement = 'sort.reverse' + + corrector.replace(range, replacement) + end end end end diff --git a/lib/rubocop/cop/performance/times_map.rb b/lib/rubocop/cop/performance/times_map.rb index 4b0458c9ea..18584e2c52 100644 --- a/lib/rubocop/cop/performance/times_map.rb +++ b/lib/rubocop/cop/performance/times_map.rb @@ -43,6 +43,7 @@ def on_send(node) def on_block(node) check(node) end + alias on_numblock on_block private @@ -66,7 +67,7 @@ def message(map_or_collect, count) end def_node_matcher :times_map_call, <<~PATTERN - {(block $(send (send $!nil? :times) {:map :collect}) ...) + {({block numblock} $(send (send $!nil? :times) {:map :collect}) ...) $(send (send $!nil? :times) {:map :collect} (block_pass ...))} PATTERN end diff --git a/spec/rubocop/cop/performance/redundant_sort_block_spec.rb b/spec/rubocop/cop/performance/redundant_sort_block_spec.rb index c602a1687d..234a6ee4e0 100644 --- a/spec/rubocop/cop/performance/redundant_sort_block_spec.rb +++ b/spec/rubocop/cop/performance/redundant_sort_block_spec.rb @@ -4,7 +4,7 @@ it 'registers an offense and corrects when sorting in direct order' do expect_offense(<<~RUBY) array.sort { |a, b| a <=> b } - ^^^^^^^^^^^^^^^^^^^^^^^ Use `sort` instead of `sort { |a, b| a <=> b }`. + ^^^^^^^^^^^^^^^^^^^^^^^ Use `sort` without block. RUBY expect_correction(<<~RUBY) @@ -29,4 +29,29 @@ array.sort RUBY end + + context 'when using numbered parameter', :ruby27 do + it 'registers an offense and corrects when sorting in direct order' do + expect_offense(<<~RUBY) + array.sort { _1 <=> _2 } + ^^^^^^^^^^^^^^^^^^ Use `sort` without block. + RUBY + + expect_correction(<<~RUBY) + array.sort + RUBY + end + + it 'does not register an offense when sorting in reverse order' do + expect_no_offenses(<<~RUBY) + array.sort { _2 <=> _1 } + RUBY + end + + it 'does not register an offense when sorting in direct order by some property' do + expect_no_offenses(<<~RUBY) + array.sort { _1.x <=> _2.x } + RUBY + end + end end diff --git a/spec/rubocop/cop/performance/sort_reverse_spec.rb b/spec/rubocop/cop/performance/sort_reverse_spec.rb index 37f199189d..6ae2441bea 100644 --- a/spec/rubocop/cop/performance/sort_reverse_spec.rb +++ b/spec/rubocop/cop/performance/sort_reverse_spec.rb @@ -9,7 +9,7 @@ it 'registers an offense and corrects when sorting in reverse order' do expect_offense(<<~RUBY) array.sort { |a, b| b <=> a } - ^^^^^^^^^^^^^^^^^^^^^^^ Use `sort.reverse` instead of `sort { |a, b| b <=> a }`. + ^^^^^^^^^^^^^^^^^^^^^^^ Use `sort.reverse` instead. RUBY expect_correction(<<~RUBY) @@ -17,6 +17,31 @@ RUBY end + context 'when using numbered parameter', :ruby27 do + it 'registers an offense and corrects when sorting in reverse order' do + expect_offense(<<~RUBY) + array.sort { _2 <=> _1 } + ^^^^^^^^^^^^^^^^^^ Use `sort.reverse` instead. + RUBY + + expect_correction(<<~RUBY) + array.sort.reverse + RUBY + end + + it 'does not register an offense when sorting in direct order' do + expect_no_offenses(<<~RUBY) + array.sort { _1 <=> _2 } + RUBY + end + + it 'does not register an offense when sorting in reverse order by some property' do + expect_no_offenses(<<~RUBY) + array.sort { _2.x <=> _1.x } + RUBY + end + end + it 'does not register an offense when sorting in direct order' do expect_no_offenses(<<~RUBY) array.sort { |a, b| a <=> b } diff --git a/spec/rubocop/cop/performance/times_map_spec.rb b/spec/rubocop/cop/performance/times_map_spec.rb index 7bd9c38d99..9fd99368fa 100644 --- a/spec/rubocop/cop/performance/times_map_spec.rb +++ b/spec/rubocop/cop/performance/times_map_spec.rb @@ -57,6 +57,19 @@ RUBY end end + + context 'when using numbered parameter', :ruby27 do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, method: method) + 4.times.#{method} { _1.to_s } + ^^^^^^^^^{method}^^^^^^^^^^^^ Use `Array.new(4)` with a block instead of `.times.#{method}`. + RUBY + + expect_correction(<<~RUBY) + Array.new(4) { _1.to_s } + RUBY + end + end end end