Skip to content

Commit

Permalink
Make some cops aware of safe navigation operator
Browse files Browse the repository at this point in the history
Fixes #403, #404, #405, #406, #408, #411, #412, #413, #415, #416, and #417.

This PR makes `Performance/Count`, `Performance/FixedSize`, `Performance/FlatMap`,
`Performance/InefficientHashSearch`, `Performance/RangeInclude`, `Performance/RedundantSortBlock`,
`Performance/ReverseFirst`, `Performance/SelectMap`, `Performance/Size` `Performance/SortReverse`,
and `Performance/TimesMap` cops aware of safe navigation operator.
  • Loading branch information
koic committed Dec 7, 2023
1 parent 76911d0 commit 93cd79c
Show file tree
Hide file tree
Showing 23 changed files with 208 additions and 35 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#419](https://github.com/rubocop/rubocop-performance/pull/419): Make `Performance/Count`, `Performance/FixedSize`, `Performance/FlatMap`, `Performance/InefficientHashSearch`, `Performance/RangeInclude`, `Performance/RedundantSortBlock`, `Performance/ReverseFirst`, `Performance/SelectMap`, `Performance/Size`, `Performance/SortReverse`, and `Performance/TimesMap` cops aware of safe navigation operator. ([@koic][])
4 changes: 2 additions & 2 deletions lib/rubocop/cop/mixin/sort_block.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ module SortBlock

def_node_matcher :sort_with_block?, <<~PATTERN
(block
$(send _ :sort)
$(call _ :sort)
(args (arg $_a) (arg $_b))
$send)
PATTERN

def_node_matcher :sort_with_numblock?, <<~PATTERN
(numblock
$(send _ :sort)
$(call _ :sort)
$_arg_count
$send)
PATTERN
Expand Down
7 changes: 4 additions & 3 deletions lib/rubocop/cop/performance/count.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ class Count < Base

def_node_matcher :count_candidate?, <<~PATTERN
{
(send (block $(send _ ${:select :filter :find_all :reject}) ...) ${:count :length :size})
(send $(send _ ${:select :filter :find_all :reject} (:block_pass _)) ${:count :length :size})
(call (block $(call _ ${:select :filter :find_all :reject}) ...) ${:count :length :size})
(call $(call _ ${:select :filter :find_all :reject} (:block_pass _)) ${:count :length :size})
}
PATTERN

Expand All @@ -72,6 +72,7 @@ def on_send(node)
end
end
end
alias on_csend on_send

private

Expand Down Expand Up @@ -100,7 +101,7 @@ def source_starting_at(node)
end

def negate_reject(corrector, node)
if node.receiver.send_type?
if node.receiver.call_type?
negate_block_pass_reject(corrector, node)
else
negate_block_reject(corrector, node)
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/performance/fixed_size.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class FixedSize < Base
RESTRICT_ON_SEND = %i[count length size].freeze

def_node_matcher :counter, <<~MATCHER
(send ${array hash str sym} {:count :length :size} $...)
(call ${array hash str sym} {:count :length :size} $...)
MATCHER

def on_send(node)
Expand All @@ -62,6 +62,7 @@ def on_send(node)
add_offense(node)
end
end
alias on_csend on_send

private

Expand Down
7 changes: 4 additions & 3 deletions lib/rubocop/cop/performance/flat_map.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ class FlatMap < Base
'multiple levels.'

def_node_matcher :flat_map_candidate?, <<~PATTERN
(send
(call
{
$(block (send _ ${:collect :map}) ...)
$(send _ ${:collect :map} (block_pass _))
$(block (call _ ${:collect :map}) ...)
$(call _ ${:collect :map} (block_pass _))
}
${:flatten :flatten!}
$...
Expand All @@ -46,6 +46,7 @@ def on_send(node)
end
end
end
alias on_csend on_send

private

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/performance/inefficient_hash_search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class InefficientHashSearch < Base
RESTRICT_ON_SEND = %i[include?].freeze

def_node_matcher :inefficient_include?, <<~PATTERN
(send (call $_ {:keys :values}) :include? _)
(call (call $_ {:keys :values}) :include? _)
PATTERN

def on_send(node)
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/performance/range_include.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class RangeInclude < Base
# (We don't even catch it if the Range is in double parens)

def_node_matcher :range_include, <<~PATTERN
(send {irange erange (begin {irange erange})} ${:include? :member?} ...)
(call {irange erange (begin {irange erange})} ${:include? :member?} ...)
PATTERN

def on_send(node)
Expand All @@ -50,6 +50,7 @@ def on_send(node)
end
end
end
alias on_csend on_send
end
end
end
Expand Down
18 changes: 5 additions & 13 deletions lib/rubocop/cop/performance/reverse_first.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ class ReverseFirst < Base
RESTRICT_ON_SEND = %i[first].freeze

def_node_matcher :reverse_first_candidate?, <<~PATTERN
(send $(call _ :reverse) :first (int _)?)
(call $(call _ :reverse) :first (int _)?)
PATTERN

def on_send(node)
reverse_first_candidate?(node) do |receiver|
range = correction_range(receiver, node)
message = build_message(node)
message = build_message(node, range)

add_offense(range, message: message) do |corrector|
replacement = build_good_method(node)
Expand All @@ -47,27 +47,19 @@ def correction_range(receiver, node)
range_between(receiver.loc.selector.begin_pos, node.source_range.end_pos)
end

def build_message(node)
def build_message(node, range)
good_method = build_good_method(node)
bad_method = build_bad_method(node)
bad_method = range.source
format(MSG, good_method: good_method, bad_method: bad_method)
end

def build_good_method(node)
if node.arguments?
"last(#{node.first_argument.source}).reverse"
"last(#{node.first_argument.source})#{node.loc.dot.source}reverse"
else
'last'
end
end

def build_bad_method(node)
if node.arguments?
"reverse.first(#{node.first_argument.source})"
else
'reverse.first'
end
end
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/performance/select_map.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ def on_send(node)
def map_method_candidate(node)
return unless (parent = node.parent)

if parent.block_type? && parent.parent&.send_type?
if parent.block_type? && parent.parent&.call_type?
parent.parent
elsif parent.send_type?
elsif parent.call_type?
parent
end
end
Expand Down
7 changes: 4 additions & 3 deletions lib/rubocop/cop/performance/size.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class Size < Base
def_node_matcher :array?, <<~PATTERN
{
[!nil? array_type?]
(send _ :to_a)
(call _ :to_a)
(send (const nil? :Array) :[] _)
(send nil? :Array _)
}
Expand All @@ -52,14 +52,14 @@ class Size < Base
def_node_matcher :hash?, <<~PATTERN
{
[!nil? hash_type?]
(send _ :to_h)
(call _ :to_h)
(send (const nil? :Hash) :[] _)
(send nil? :Hash _)
}
PATTERN

def_node_matcher :count?, <<~PATTERN
(send {#array? #hash?} :count)
(call {#array? #hash?} :count)
PATTERN

def on_send(node)
Expand All @@ -69,6 +69,7 @@ def on_send(node)
corrector.replace(node.loc.selector, 'size')
end
end
alias on_csend on_send
end
end
end
Expand Down
9 changes: 5 additions & 4 deletions lib/rubocop/cop/performance/sort_reverse.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class SortReverse < Base
include SortBlock
extend AutoCorrector

MSG = 'Use `sort.reverse` instead.'
MSG = 'Use `%<prefer>s` instead.'

def on_block(node)
sort_with_block?(node) do |send, var_a, var_b, body|
Expand All @@ -41,11 +41,12 @@ def on_numblock(node)

def register_offense(send, node)
range = sort_range(send, node)
prefer = "sort#{send.loc.dot.source}reverse"

add_offense(range) do |corrector|
replacement = 'sort.reverse'
message = format(MSG, prefer: prefer)

corrector.replace(range, replacement)
add_offense(range, message: message) do |corrector|
corrector.replace(range, prefer)
end
end
end
Expand Down
7 changes: 5 additions & 2 deletions lib/rubocop/cop/performance/times_map.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class TimesMap < Base
def on_send(node)
check(node)
end
alias on_csend on_send

def on_block(node)
check(node)
Expand Down Expand Up @@ -67,8 +68,10 @@ def message(map_or_collect, count)
end

def_node_matcher :times_map_call, <<~PATTERN
{({block numblock} $(send (send $!nil? :times) {:map :collect}) ...)
$(send (send $!nil? :times) {:map :collect} (block_pass ...))}
{
({block numblock} $(call (call $!nil? :times) {:map :collect}) ...)
$(call (call $!nil? :times) {:map :collect} (block_pass ...))
}
PATTERN
end
end
Expand Down
14 changes: 14 additions & 0 deletions spec/rubocop/cop/performance/count_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@
RUBY
end

it "registers an offense for using array&.#{selector}...size" do
expect_offense(<<~RUBY, selector: selector)
[1, 2, 3]&.#{selector} { |e| e.even? }&.size
^{selector}^^^^^^^^^^^^^^^^^^^^^^ Use `count` instead of `#{selector}...size`.
RUBY
end

it "registers an offense for using hash.#{selector}...size" do
expect_offense(<<~RUBY, selector: selector)
{a: 1, b: 2, c: 3}.#{selector} { |e| e == :a }.size
Expand Down Expand Up @@ -72,6 +79,13 @@
RUBY
end

it "registers an offense for #{selector}(&:something)&.count" do
expect_offense(<<~RUBY, selector: selector)
foo&.#{selector}(&:something)&.count
^{selector}^^^^^^^^^^^^^^^^^^^^ Use `count` instead of `#{selector}...count`.
RUBY
end

it "registers an offense for #{selector}(&:something).count " \
'when called as an instance method on its own class' do
expect_offense(<<~RUBY, selector: selector)
Expand Down
7 changes: 7 additions & 0 deletions spec/rubocop/cop/performance/fixed_size_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
RUBY
end

it "registers an offense when safe navigation calling #{method} on a single quoted string" do
expect_offense(<<~RUBY, method: method)
'a'&.#{method}
^^^^^^{method} Do not compute the size of statically sized objects.
RUBY
end

it "registers an offense when calling #{method} on a double quoted string" do
expect_offense(<<~RUBY, method: method)
"a".#{method}
Expand Down
22 changes: 22 additions & 0 deletions spec/rubocop/cop/performance/flat_map_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@
RUBY
end

it "registers an offense and corrects when safe navigation calling #{method}...#{flatten}(1)" do
expect_offense(<<~RUBY, method: method, flatten: flatten)
[1, 2, 3, 4]&.#{method} { |e| [e, e] }&.#{flatten}(1)
^{method}^^^^^^^^^^^^^^^^^^{flatten}^^^ Use `flat_map` instead of `#{method}...#{flatten}`.
RUBY

expect_correction(<<~RUBY)
[1, 2, 3, 4]&.flat_map { |e| [e, e] }
RUBY
end

it "registers an offense and corrects when calling #{method}...#{flatten}(1) on separate lines" do
expect_offense(<<~RUBY, method: method, flatten: flatten)
[1, 2, 3, 4]
Expand Down Expand Up @@ -40,6 +51,17 @@
RUBY
end

it "registers an offense and corrects when safe navigation calling #{method}(&:foo).#{flatten}(1)" do
expect_offense(<<~RUBY, method: method, flatten: flatten)
[1, 2, 3, 4]&.#{method}(&:foo).#{flatten}(1)
^{method}^^^^^^^^^{flatten}^^^ Use `flat_map` instead of `#{method}...#{flatten}`.
RUBY

expect_correction(<<~RUBY)
[1, 2, 3, 4]&.flat_map(&:foo)
RUBY
end

it "registers an offense and corrects when calling #{method}(&:foo).#{flatten}(1) on separate lines" do
expect_offense(<<~RUBY, method: method, flatten: flatten)
[1, 2, 3, 4]
Expand Down
7 changes: 7 additions & 0 deletions spec/rubocop/cop/performance/inefficient_hash_search_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@
RUBY
end

it 'registers an offense when a hash literal receives `&.keys&.include?`' do
expect_offense(<<~RUBY)
{ a: 1 }&.keys&.include? 1
^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `##{expected_key_method}` instead of `#keys.include?`.
RUBY
end

it 'registers an offense when an existing hash receives `keys.include?`' do
expect_offense(<<~RUBY)
h = { a: 1 }; h.keys.include? 1
Expand Down
5 changes: 5 additions & 0 deletions spec/rubocop/cop/performance/range_include_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
expect(new_source).to eq '(a..b).cover? 1'
end

it "autocorrects (a..b)&.#{method} without parens" do
new_source = autocorrect_source("(a..b)&.#{method} 1")
expect(new_source).to eq '(a..b)&.cover? 1'
end

it "autocorrects (a...b).#{method} without parens" do
new_source = autocorrect_source("(a...b).#{method} 1")
expect(new_source).to eq '(a...b).cover? 1'
Expand Down
22 changes: 22 additions & 0 deletions spec/rubocop/cop/performance/redundant_sort_block_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@
RUBY
end

it 'registers an offense and corrects when sorting in direct order with safe navigation' do
expect_offense(<<~RUBY)
array&.sort { |a, b| a <=> b }
^^^^^^^^^^^^^^^^^^^^^^^ 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 { |a, b| b <=> a }
Expand Down Expand Up @@ -42,6 +53,17 @@
RUBY
end

it 'registers an offense and corrects when sorting in direct order with safe navigation' 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 }
Expand Down
Loading

0 comments on commit 93cd79c

Please sign in to comment.