From 0ac774be4ee3025cb8fd5a893ae229ac7b5c4d6b Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Thu, 10 Sep 2020 16:54:22 -0400 Subject: [PATCH] [Fix #163] Detect array indexes for `Performance/Detect` For example, `[1, 2, 3].detect{ ... }[0]` will now be caught and corrected by the cop. --- CHANGELOG.md | 5 ++ docs/modules/ROOT/pages/cops_performance.adoc | 6 +- lib/rubocop/cop/performance/detect.rb | 62 ++++++++++++++----- spec/rubocop/cop/performance/detect_spec.rb | 44 +++++++++++++ 4 files changed, 99 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10af6840ca..f65c4effd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ * [#164](https://github.com/rubocop-hq/rubocop-performance/pull/164): Fix an error for `Performance/CollectionLiteralInLoop` when a method from `Enumerable` is called with no receiver. ([@eugeneius][]) * [#165](https://github.com/rubocop-hq/rubocop-performance/issues/165): Fix a false positive for `Performance/Sum` when using initial value argument is a variable. ([@koic][]) +### Changes + +* [#163](https://github.com/rubocop-hq/rubocop-performance/pull/163): Change `Performance/Detect` to also detect offenses when index 0 or -1 is used instead (ie. `detect{ ... }[0]`). ([@dvandersluis][]) + ## 1.8.0 (2020-09-04) ### New features @@ -162,3 +166,4 @@ [@dischorde]: https://github.com/dischorde [@siegfault]: https://github.com/siegfault [@fatkodima]: https://github.com/fatkodima +[@dvandersluis]: https://github.com/dvandersluis diff --git a/docs/modules/ROOT/pages/cops_performance.adoc b/docs/modules/ROOT/pages/cops_performance.adoc index 72b58ff384..65c49f504f 100644 --- a/docs/modules/ROOT/pages/cops_performance.adoc +++ b/docs/modules/ROOT/pages/cops_performance.adoc @@ -577,8 +577,8 @@ str.sub!(/suffix$/, '') | 1.8 |=== -This cop is used to identify usages of -`select.first`, `select.last`, `find_all.first`, `find_all.last`, `filter.first`, and `filter.last` +This cop is used to identify usages of `first`, `last`, `[0]` or `[-1]` +chained to `select`, `find_all`, or `find_all` and change them to use `detect` instead. `ActiveRecord` compatibility: @@ -597,6 +597,8 @@ considered unsafe. [].find_all { |item| true }.last [].filter { |item| true }.first [].filter { |item| true }.last +[].filter { |item| true }[0] +[].filter { |item| true }[-1] # good [].detect { |item| true } diff --git a/lib/rubocop/cop/performance/detect.rb b/lib/rubocop/cop/performance/detect.rb index dd3995ebb5..a673bd7d2e 100644 --- a/lib/rubocop/cop/performance/detect.rb +++ b/lib/rubocop/cop/performance/detect.rb @@ -3,8 +3,8 @@ module RuboCop module Cop module Performance - # This cop is used to identify usages of - # `select.first`, `select.last`, `find_all.first`, `find_all.last`, `filter.first`, and `filter.last` + # This cop is used to identify usages of `first`, `last`, `[0]` or `[-1]` + # chained to `select`, `find_all`, or `find_all` # and change them to use `detect` instead. # # @example @@ -15,6 +15,8 @@ module Performance # [].find_all { |item| true }.last # [].filter { |item| true }.first # [].filter { |item| true }.last + # [].filter { |item| true }[0] + # [].filter { |item| true }[-1] # # # good # [].detect { |item| true } @@ -27,27 +29,41 @@ module Performance class Detect < Base extend AutoCorrector + CANDIDATE_METHODS = Set[:select, :find_all, :filter].freeze + MSG = 'Use `%s` instead of ' \ '`%s.%s`.' REVERSE_MSG = 'Use `reverse.%s` instead of ' \ '`%s.%s`.' + INDEX_MSG = 'Use `%s` instead of ' \ + '`%s[%i]`.' + INDEX_REVERSE_MSG = 'Use `reverse.%s` instead of ' \ + '`%s[%i]`.' def_node_matcher :detect_candidate?, <<~PATTERN { - (send $(block (send _ {:select :find_all :filter}) ...) ${:first :last} $...) - (send $(send _ {:select :find_all :filter} ...) ${:first :last} $...) + (send $(block (send _ %CANDIDATE_METHODS) ...) ${:first :last} $...) + (send $(block (send _ %CANDIDATE_METHODS) ...) $:[] (int ${0 -1})) + (send $(send _ %CANDIDATE_METHODS ...) ${:first :last} $...) + (send $(send _ %CANDIDATE_METHODS ...) $:[] (int ${0 -1})) } PATTERN def on_send(node) detect_candidate?(node) do |receiver, second_method, args| + # If + if second_method == :[] + index = args + args = {} + end + return unless args.empty? return unless receiver receiver, _args, body = *receiver if receiver.block_type? return if accept_first_call?(receiver, body) - register_offense(node, receiver, second_method) + register_offense(node, receiver, second_method, index) end end @@ -62,28 +78,31 @@ def accept_first_call?(receiver, body) lazy?(caller) end - def register_offense(node, receiver, second_method) + def register_offense(node, receiver, second_method, index) _caller, first_method, _args = *receiver range = receiver.loc.selector.join(node.loc.selector) - message = second_method == :last ? REVERSE_MSG : MSG + message = message_for_method(second_method, index) formatted_message = format(message, prefer: preferred_method, first_method: first_method, - second_method: second_method) + second_method: second_method, + index: index) add_offense(range, message: formatted_message) do |corrector| - autocorrect(corrector, node) + autocorrect(corrector, node, replacement(second_method, index)) end end - def autocorrect(corrector, node) - receiver, first_method = *node + def replacement(method, index) + if method == :last || method == :[] && index == -1 + "reverse.#{preferred_method}" + else + preferred_method + end + end - replacement = if first_method == :last - "reverse.#{preferred_method}" - else - preferred_method - end + def autocorrect(corrector, node, replacement) + receiver, _first_method = *node first_range = receiver.source_range.end.join(node.loc.selector) @@ -93,6 +112,17 @@ def autocorrect(corrector, node) corrector.replace(receiver.loc.selector, replacement) end + def message_for_method(method, index) + case method + when :[] + index == -1 ? INDEX_REVERSE_MSG : INDEX_MSG + when :last + REVERSE_MSG + else + MSG + end + end + def preferred_method config.for_cop('Style/CollectionMethods')['PreferredMethods']['detect'] || 'detect' end diff --git a/spec/rubocop/cop/performance/detect_spec.rb b/spec/rubocop/cop/performance/detect_spec.rb index 7e08245003..2d7e0a330b 100644 --- a/spec/rubocop/cop/performance/detect_spec.rb +++ b/spec/rubocop/cop/performance/detect_spec.rb @@ -101,6 +101,28 @@ RUBY end + it "registers an offense with #{method} short syntax and [0]" do + expect_offense(<<~RUBY, method: method) + [1, 2, 3].#{method}(&:even?)[0] + ^{method}^^^^^^^^^^^^ Use `detect` instead of `#{method}[0]`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].detect(&:even?) + RUBY + end + + it "registers an offense with #{method} short syntax and [0]" do + expect_offense(<<~RUBY, method: method) + [1, 2, 3].#{method}(&:even?)[-1] + ^{method}^^^^^^^^^^^^^ Use `reverse.detect` instead of `#{method}[-1]`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].reverse.detect(&:even?) + RUBY + end + it "registers an offense when #{method} is called on `lazy` without receiver" do expect_offense(<<~RUBY, method: method) lazy.#{method}(&:even?).first @@ -112,6 +134,28 @@ RUBY end + it "registers an offense and corrects when [0] is called on #{method}" do + expect_offense(<<~RUBY, method: method) + [1, 2, 3].#{method} { |i| i % 2 == 0 }[0] + ^{method}^^^^^^^^^^^^^^^^^^^^^^ Use `detect` instead of `#{method}[0]`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].detect { |i| i % 2 == 0 } + RUBY + end + + it "registers an offense and corrects when [-1] is called on #{method}" do + expect_offense(<<~RUBY, method: method) + [1, 2, 3].#{method} { |i| i % 2 == 0 }[-1] + ^{method}^^^^^^^^^^^^^^^^^^^^^^^ Use `reverse.detect` instead of `#{method}[-1]`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].reverse.detect { |i| i % 2 == 0 } + RUBY + end + it "does not register an offense when #{method} is used without first or last" do expect_no_offenses("[1, 2, 3].#{method} { |i| i % 2 == 0 }") end