From 35d213801eb2faaacf62a9d2de948c35a8451632 Mon Sep 17 00:00:00 2001 From: SerhiiMisiura Date: Thu, 8 Oct 2020 10:49:00 -0500 Subject: [PATCH] [Fix #177] Performance/Sum should avoid empty arrays --- CHANGELOG.md | 2 + lib/rubocop/cop/performance/sum.rb | 56 +++++++++++++++++------- spec/rubocop/cop/performance/sum_spec.rb | 20 ++++++++- 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ecede9..1ae3a05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Changes * [#170](https://github.com/rubocop-hq/rubocop-performance/pull/170): Extend `Performance/Sum` to register an offense for `map { ... }.sum`. ([@eugeneius][]) +* [#179](https://github.com/rubocop-hq/rubocop-performance/pull/179): Change `Performance/Sum` to warn about empty arrays, and not register an offense on empty array literals. ([@ghiculescu][]) ## 1.8.1 (2020-09-19) @@ -174,3 +175,4 @@ [@siegfault]: https://github.com/siegfault [@fatkodima]: https://github.com/fatkodima [@dvandersluis]: https://github.com/dvandersluis +[@ghiculescu]: https://github.com/ghiculescu diff --git a/lib/rubocop/cop/performance/sum.rb b/lib/rubocop/cop/performance/sum.rb index cf58bd1..76f82ef 100644 --- a/lib/rubocop/cop/performance/sum.rb +++ b/lib/rubocop/cop/performance/sum.rb @@ -26,6 +26,8 @@ class Sum < Base extend AutoCorrector MSG = 'Use `%s` instead of `%s`.' + MSG_IF_NO_INIT_VALUE = + 'Use `%s` instead of `%s`, unless calling `%s` on an empty array.' def_node_matcher :sum_candidate?, <<~PATTERN (send _ ${:inject :reduce} $_init ? ${(sym :+) (block_pass (sym :+))}) @@ -53,15 +55,39 @@ class Sum < Base alias elem_plus_acc? acc_plus_elem? def on_send(node) + return if empty_array_literal?(node) + + handle_sum_candidate(node) + handle_sum_map_candidate(node) + end + + def on_block(node) + sum_with_block_candidate?(node) do |send, init, var_acc, var_elem, body| + if acc_plus_elem?(body, var_acc, var_elem) || elem_plus_acc?(body, var_elem, var_acc) + range = sum_block_range(send, node) + message = build_block_message(send, init, var_acc, var_elem, body) + + add_offense(range, message: message) do |corrector| + autocorrect(corrector, init, range) + end + end + end + end + + private + + def handle_sum_candidate(node) sum_candidate?(node) do |method, init, operation| range = sum_method_range(node) - message = build_method_message(method, init, operation) + message = build_method_message(node, method, init, operation) add_offense(range, message: message) do |corrector| autocorrect(corrector, init, range) end end + end + def handle_sum_map_candidate(node) sum_map_candidate?(node) do |map, init| next if node.block_literal? || node.block_argument? @@ -73,20 +99,15 @@ def on_send(node) end end - def on_block(node) - sum_with_block_candidate?(node) do |send, init, var_acc, var_elem, body| - if acc_plus_elem?(body, var_acc, var_elem) || elem_plus_acc?(body, var_elem, var_acc) - range = sum_block_range(send, node) - message = build_block_message(send, init, var_acc, var_elem, body) - - add_offense(range, message: message) do |corrector| - autocorrect(corrector, init, range) - end - end - end + def empty_array_literal?(node) + receiver = node.children.first + array_literal?(node) && receiver && receiver.children.empty? end - private + def array_literal?(node) + receiver = node.children.first + receiver&.literal? && receiver&.array_type? + end def autocorrect(corrector, init, range) return if init.empty? @@ -119,10 +140,15 @@ def sum_block_range(send, node) range_between(send.loc.selector.begin_pos, node.loc.end.end_pos) end - def build_method_message(method, init, operation) + def build_method_message(node, method, init, operation) good_method = build_good_method(init) bad_method = build_method_bad_method(init, method, operation) - format(MSG, good_method: good_method, bad_method: bad_method) + msg = if init.empty? && !array_literal?(node) + MSG_IF_NO_INIT_VALUE + else + MSG + end + format(msg, good_method: good_method, bad_method: bad_method) end def build_sum_map_message(method, init) diff --git a/spec/rubocop/cop/performance/sum_spec.rb b/spec/rubocop/cop/performance/sum_spec.rb index a81913f..cfbd2d1 100644 --- a/spec/rubocop/cop/performance/sum_spec.rb +++ b/spec/rubocop/cop/performance/sum_spec.rb @@ -73,7 +73,7 @@ it 'does not autocorrect `:+` when initial value is not provided' do expect_offense(<<~RUBY, method: method) array.#{method}(:+) - ^{method}^^^^ Use `sum` instead of `#{method}(:+)`. + ^{method}^^^^ Use `sum` instead of `#{method}(:+)`, unless calling `#{method}(:+)` on an empty array. RUBY expect_no_corrections @@ -93,12 +93,28 @@ it 'does not autocorrect `&:+` when initial value is not provided' do expect_offense(<<~RUBY, method: method) array.#{method}(&:+) - ^{method}^^^^^ Use `sum` instead of `#{method}(&:+)`. + ^{method}^^^^^ Use `sum` instead of `#{method}(&:+)`, unless calling `#{method}(&:+)` on an empty array. RUBY expect_no_corrections end + # ideally it would autocorrect to `[1, 2, 3].sum` + it 'registers an offense but does not autocorrect on array literals' do + expect_offense(<<~RUBY, method: method) + [1, 2, 3].#{method}(:+) + ^{method}^^^^ Use `sum` instead of `#{method}(:+)`. + RUBY + + expect_no_corrections + end + + it 'does not register an offense when the array is empty' do + expect_no_offenses(<<~RUBY, method: method) + [].#{method}(:+) + RUBY + end + it 'does not register an offense when block does not implement summation' do expect_no_offenses(<<~RUBY) array.#{method} { |acc, elem| elem * 2 }