diff --git a/changelog/fix_make_some_cops_aware_of_safe_navigation_operator.md b/changelog/fix_make_some_cops_aware_of_safe_navigation_operator.md new file mode 100644 index 0000000000..ab31a7bbd1 --- /dev/null +++ b/changelog/fix_make_some_cops_aware_of_safe_navigation_operator.md @@ -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][]) diff --git a/lib/rubocop/cop/mixin/sort_block.rb b/lib/rubocop/cop/mixin/sort_block.rb index cc62dc1683..6e9a7e48f5 100644 --- a/lib/rubocop/cop/mixin/sort_block.rb +++ b/lib/rubocop/cop/mixin/sort_block.rb @@ -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 diff --git a/lib/rubocop/cop/performance/count.rb b/lib/rubocop/cop/performance/count.rb index 2eada38ef0..0eb13e52af 100644 --- a/lib/rubocop/cop/performance/count.rb +++ b/lib/rubocop/cop/performance/count.rb @@ -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 @@ -72,6 +72,7 @@ def on_send(node) end end end + alias on_csend on_send private @@ -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) diff --git a/lib/rubocop/cop/performance/fixed_size.rb b/lib/rubocop/cop/performance/fixed_size.rb index 0eff8fcb48..cf7ef63c9d 100644 --- a/lib/rubocop/cop/performance/fixed_size.rb +++ b/lib/rubocop/cop/performance/fixed_size.rb @@ -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) @@ -62,6 +62,7 @@ def on_send(node) add_offense(node) end end + alias on_csend on_send private diff --git a/lib/rubocop/cop/performance/flat_map.rb b/lib/rubocop/cop/performance/flat_map.rb index 529bf13870..6c4d54aa65 100644 --- a/lib/rubocop/cop/performance/flat_map.rb +++ b/lib/rubocop/cop/performance/flat_map.rb @@ -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!} $... @@ -46,6 +46,7 @@ def on_send(node) end end end + alias on_csend on_send private diff --git a/lib/rubocop/cop/performance/inefficient_hash_search.rb b/lib/rubocop/cop/performance/inefficient_hash_search.rb index 1a951c32b6..99e4e2c0f1 100644 --- a/lib/rubocop/cop/performance/inefficient_hash_search.rb +++ b/lib/rubocop/cop/performance/inefficient_hash_search.rb @@ -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) diff --git a/lib/rubocop/cop/performance/range_include.rb b/lib/rubocop/cop/performance/range_include.rb index a03721aed3..d56afc2262 100644 --- a/lib/rubocop/cop/performance/range_include.rb +++ b/lib/rubocop/cop/performance/range_include.rb @@ -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) @@ -50,6 +50,7 @@ def on_send(node) end end end + alias on_csend on_send end end end diff --git a/lib/rubocop/cop/performance/reverse_first.rb b/lib/rubocop/cop/performance/reverse_first.rb index c3b48c3107..ba5328329b 100644 --- a/lib/rubocop/cop/performance/reverse_first.rb +++ b/lib/rubocop/cop/performance/reverse_first.rb @@ -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) @@ -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 diff --git a/lib/rubocop/cop/performance/select_map.rb b/lib/rubocop/cop/performance/select_map.rb index 3dbf0d7b2d..589bb619de 100644 --- a/lib/rubocop/cop/performance/select_map.rb +++ b/lib/rubocop/cop/performance/select_map.rb @@ -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 diff --git a/lib/rubocop/cop/performance/size.rb b/lib/rubocop/cop/performance/size.rb index b107949a78..84b0847b83 100644 --- a/lib/rubocop/cop/performance/size.rb +++ b/lib/rubocop/cop/performance/size.rb @@ -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 _) } @@ -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) @@ -69,6 +69,7 @@ def on_send(node) corrector.replace(node.loc.selector, 'size') end end + alias on_csend on_send end end end diff --git a/lib/rubocop/cop/performance/sort_reverse.rb b/lib/rubocop/cop/performance/sort_reverse.rb index 524e03b23d..fe551b7170 100644 --- a/lib/rubocop/cop/performance/sort_reverse.rb +++ b/lib/rubocop/cop/performance/sort_reverse.rb @@ -17,7 +17,7 @@ class SortReverse < Base include SortBlock extend AutoCorrector - MSG = 'Use `sort.reverse` instead.' + MSG = 'Use `%s` instead.' def on_block(node) sort_with_block?(node) do |send, var_a, var_b, body| @@ -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 diff --git a/lib/rubocop/cop/performance/times_map.rb b/lib/rubocop/cop/performance/times_map.rb index 58e76cbe62..9b9c120638 100644 --- a/lib/rubocop/cop/performance/times_map.rb +++ b/lib/rubocop/cop/performance/times_map.rb @@ -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) @@ -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 diff --git a/spec/rubocop/cop/performance/count_spec.rb b/spec/rubocop/cop/performance/count_spec.rb index af2167cba5..2e6bf293b5 100644 --- a/spec/rubocop/cop/performance/count_spec.rb +++ b/spec/rubocop/cop/performance/count_spec.rb @@ -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 @@ -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) diff --git a/spec/rubocop/cop/performance/fixed_size_spec.rb b/spec/rubocop/cop/performance/fixed_size_spec.rb index 40c3ab183c..51331ca64b 100644 --- a/spec/rubocop/cop/performance/fixed_size_spec.rb +++ b/spec/rubocop/cop/performance/fixed_size_spec.rb @@ -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} diff --git a/spec/rubocop/cop/performance/flat_map_spec.rb b/spec/rubocop/cop/performance/flat_map_spec.rb index 2a4e4ce4a4..9a1f2f2bb9 100644 --- a/spec/rubocop/cop/performance/flat_map_spec.rb +++ b/spec/rubocop/cop/performance/flat_map_spec.rb @@ -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] @@ -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] diff --git a/spec/rubocop/cop/performance/inefficient_hash_search_spec.rb b/spec/rubocop/cop/performance/inefficient_hash_search_spec.rb index fe6c61eab9..d7d282bc55 100644 --- a/spec/rubocop/cop/performance/inefficient_hash_search_spec.rb +++ b/spec/rubocop/cop/performance/inefficient_hash_search_spec.rb @@ -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 diff --git a/spec/rubocop/cop/performance/range_include_spec.rb b/spec/rubocop/cop/performance/range_include_spec.rb index a9bf53a5fc..f8e02c8bff 100644 --- a/spec/rubocop/cop/performance/range_include_spec.rb +++ b/spec/rubocop/cop/performance/range_include_spec.rb @@ -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' diff --git a/spec/rubocop/cop/performance/redundant_sort_block_spec.rb b/spec/rubocop/cop/performance/redundant_sort_block_spec.rb index 234a6ee4e0..43ecb2d10c 100644 --- a/spec/rubocop/cop/performance/redundant_sort_block_spec.rb +++ b/spec/rubocop/cop/performance/redundant_sort_block_spec.rb @@ -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 } @@ -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 } diff --git a/spec/rubocop/cop/performance/reverse_first_spec.rb b/spec/rubocop/cop/performance/reverse_first_spec.rb index 7f4d4a3649..b96c96739e 100644 --- a/spec/rubocop/cop/performance/reverse_first_spec.rb +++ b/spec/rubocop/cop/performance/reverse_first_spec.rb @@ -12,6 +12,28 @@ RUBY end + it 'registers an offense and corrects when using `#reverse&.first(5)`' do + expect_offense(<<~RUBY) + array.reverse&.first(5) + ^^^^^^^^^^^^^^^^^ Use `last(5)&.reverse` instead of `reverse&.first(5)`. + RUBY + + expect_correction(<<~RUBY) + array.last(5)&.reverse + RUBY + end + + it 'registers an offense and corrects when using `&.reverse&.first(5)`' do + expect_offense(<<~RUBY) + array&.reverse&.first(5) + ^^^^^^^^^^^^^^^^^ Use `last(5)&.reverse` instead of `reverse&.first(5)`. + RUBY + + expect_correction(<<~RUBY) + array&.last(5)&.reverse + RUBY + end + it 'registers an offense and corrects when using `#reverse.first`' do expect_offense(<<~RUBY) array.reverse.first diff --git a/spec/rubocop/cop/performance/select_map_spec.rb b/spec/rubocop/cop/performance/select_map_spec.rb index 60fe0a39b5..f472fd0149 100644 --- a/spec/rubocop/cop/performance/select_map_spec.rb +++ b/spec/rubocop/cop/performance/select_map_spec.rb @@ -9,6 +9,13 @@ RUBY end + 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) @@ -37,6 +44,13 @@ 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) diff --git a/spec/rubocop/cop/performance/size_spec.rb b/spec/rubocop/cop/performance/size_spec.rb index 666e5bb8b6..dd793cc250 100644 --- a/spec/rubocop/cop/performance/size_spec.rb +++ b/spec/rubocop/cop/performance/size_spec.rb @@ -17,6 +17,13 @@ RUBY end + it 'registers an offense when safe navigation calling count' do + expect_offense(<<~RUBY) + [1, 2, 3]&.count + ^^^^^ Use `size` instead of `count`. + RUBY + end + it 'registers an offense when calling count on to_a' do expect_offense(<<~RUBY) (1..3).to_a.count @@ -24,6 +31,13 @@ RUBY end + it 'registers an offense when safe navigation calling count on to_a' do + expect_offense(<<~RUBY) + (1..3)&.to_a&.count + ^^^^^ Use `size` instead of `count`. + RUBY + end + it 'registers an offense when calling count on Array[]' do expect_offense(<<~RUBY) Array[*1..5].count @@ -98,6 +112,13 @@ RUBY end + it 'registers an offense when safe navigation calling count on to_h' do + expect_offense(<<~RUBY) + [[:foo, :bar], [1, 2]]&.to_h&.count + ^^^^^ Use `size` instead of `count`. + RUBY + end + it 'registers an offense when calling count on Hash[]' do expect_offense(<<~RUBY) Hash[*('a'..'z')].count diff --git a/spec/rubocop/cop/performance/sort_reverse_spec.rb b/spec/rubocop/cop/performance/sort_reverse_spec.rb index 6ae2441bea..55d5333058 100644 --- a/spec/rubocop/cop/performance/sort_reverse_spec.rb +++ b/spec/rubocop/cop/performance/sort_reverse_spec.rb @@ -17,6 +17,17 @@ RUBY end + it 'registers an offense and corrects when sorting in reverse order with safe navigation' do + expect_offense(<<~RUBY) + array&.sort { |a, b| b <=> a } + ^^^^^^^^^^^^^^^^^^^^^^^ Use `sort&.reverse` instead. + RUBY + + expect_correction(<<~RUBY) + array&.sort&.reverse + RUBY + end + context 'when using numbered parameter', :ruby27 do it 'registers an offense and corrects when sorting in reverse order' do expect_offense(<<~RUBY) diff --git a/spec/rubocop/cop/performance/times_map_spec.rb b/spec/rubocop/cop/performance/times_map_spec.rb index 0c5fdf35cb..71b7ef59f4 100644 --- a/spec/rubocop/cop/performance/times_map_spec.rb +++ b/spec/rubocop/cop/performance/times_map_spec.rb @@ -16,6 +16,19 @@ end end + context 'with a block with safe navigation call' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, method: method) + 4&.times&.#{method} { |i| i.to_s } + ^^^^^^^^^^^{method}^^^^^^^^^^^^^^^ Use `Array.new(4)` with a block instead of `.times.#{method}`. + RUBY + + expect_correction(<<~RUBY) + Array.new(4) { |i| i.to_s } + RUBY + end + end + context 'for non-literal receiver' do it 'registers an offense' do expect_offense(<<~RUBY, method: method) @@ -42,6 +55,19 @@ end end + context 'with an explicitly passed block with safe navigation call' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, method: method) + 4&.times&.#{method}(&method(:foo)) + ^^^^^^^^^^^{method}^^^^^^^^^^^^^^^ Use `Array.new(4)` with a block instead of `.times.#{method}`. + RUBY + + expect_correction(<<~RUBY) + Array.new(4, &method(:foo)) + RUBY + end + end + context 'without a block' do it "doesn't register an offense" do expect_no_offenses(<<~RUBY)