From 2f5955040dcb65e9f69a762d1f4aa24be7aaa8cc Mon Sep 17 00:00:00 2001 From: Leo Arnold Date: Tue, 16 Nov 2021 02:08:22 +0100 Subject: [PATCH] [Fix #204] Add `Performance/Sum` option to ignore potential false positives Some codebases may contain non-numeric classes which implement a `:+` method. If `a`, `b` and `c` are objects of such a class, then certain corrections do not apply: ```ruby [a, b].reduce(c, :+) # is equivalent to [a, b].sum(c) [a, b].map(&:to_i).sum # is equivalent to [a, b].sum(&:to_i) [a, b].reduce(:+) # works [a, b].sum # raises TypeError ``` For users who wish to only register offenses where auto-correction is unproblematic, we add the option `OnlySumOrWithInitialValue` to the `Performance/Sum` cop. The option is disabled by default, so standard behavior is preserved, and can be activated by setting `OnlySumOrWithInitialValue: true`. --- CHANGELOG.md | 1 + changelog/.gitkeep | 0 ...new_add_performancesum_option_to_ignore.md | 1 + config/default.yml | 1 + docs/modules/ROOT/pages/cops_performance.adoc | 27 +++ lib/rubocop/cop/performance/sum.rb | 15 +- spec/rubocop/cop/performance/sum_spec.rb | 193 ++++++++++++++++++ 7 files changed, 237 insertions(+), 1 deletion(-) create mode 100644 changelog/.gitkeep create mode 100644 changelog/new_add_performancesum_option_to_ignore.md diff --git a/CHANGELOG.md b/CHANGELOG.md index cc78dc733a..18410688ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -312,3 +312,4 @@ [@ghiculescu]: https://github.com/ghiculescu [@mfbmina]: https://github.com/mfbmina [@mvz]: https://github.com/mvz +[@leoarnold]: https://github.com/leoarnold diff --git a/changelog/.gitkeep b/changelog/.gitkeep new file mode 100644 index 0000000000..e69de29bb2 diff --git a/changelog/new_add_performancesum_option_to_ignore.md b/changelog/new_add_performancesum_option_to_ignore.md new file mode 100644 index 0000000000..4a538a7738 --- /dev/null +++ b/changelog/new_add_performancesum_option_to_ignore.md @@ -0,0 +1 @@ +* [#204](https://github.com/rubocop/rubocop-performance/issues/204): Add `Performance/Sum` option to ignore potential false positives. ([@leoarnold][]) diff --git a/config/default.yml b/config/default.yml index 446b24a760..af19a0cf67 100644 --- a/config/default.yml +++ b/config/default.yml @@ -333,6 +333,7 @@ Performance/Sum: Reference: 'https://blog.bigbinary.com/2016/11/02/ruby-2-4-introduces-enumerable-sum.html' Enabled: 'pending' VersionAdded: '1.8' + OnlySumOrWithInitialValue: false Performance/TimesMap: Description: 'Checks for .times.map calls.' diff --git a/docs/modules/ROOT/pages/cops_performance.adoc b/docs/modules/ROOT/pages/cops_performance.adoc index 234d8fbe0a..83f3b1e54a 100644 --- a/docs/modules/ROOT/pages/cops_performance.adoc +++ b/docs/modules/ROOT/pages/cops_performance.adoc @@ -2047,6 +2047,8 @@ Please note that the auto-correction command line option will be changed from === Examples +==== OnlySumOrWithInitialValue: false (default) + [source,ruby] ---- # bad @@ -2064,6 +2066,31 @@ Please note that the auto-correction command line option will be changed from [1, 2, 3].sum(10, &:count) ---- +==== OnlySumOrWithInitialValue: true + +[source,ruby] +---- +# bad +[1, 2, 3].reduce(10, :+) +[1, 2, 3].map { |elem| elem ** 2 }.sum +[1, 2, 3].collect(&:count).sum(10) + +# good +[1, 2, 3].sum(10) +[1, 2, 3].sum { |elem| elem ** 2 } +[1, 2, 3].sum(10, &:count) +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| OnlySumOrWithInitialValue +| `false` +| Boolean +|=== + === References * https://blog.bigbinary.com/2016/11/02/ruby-2-4-introduces-enumerable-sum.html diff --git a/lib/rubocop/cop/performance/sum.rb b/lib/rubocop/cop/performance/sum.rb index 30067a4c09..cc346355ef 100644 --- a/lib/rubocop/cop/performance/sum.rb +++ b/lib/rubocop/cop/performance/sum.rb @@ -30,7 +30,7 @@ module Performance # Please note that the auto-correction command line option will be changed from # `rubocop -a` to `rubocop -A`, which includes unsafe auto-correction. # - # @example + # @example OnlySumOrWithInitialValue: false (default) # # bad # [1, 2, 3].inject(:+) # These bad cases with no initial value are unsafe and # [1, 2, 3].inject(&:+) # will not be auto-correced by default. If you want to @@ -45,6 +45,17 @@ module Performance # [1, 2, 3].sum { |elem| elem ** 2 } # [1, 2, 3].sum(10, &:count) # + # @example OnlySumOrWithInitialValue: true + # # bad + # [1, 2, 3].reduce(10, :+) + # [1, 2, 3].map { |elem| elem ** 2 }.sum + # [1, 2, 3].collect(&:count).sum(10) + # + # # good + # [1, 2, 3].sum(10) + # [1, 2, 3].sum { |elem| elem ** 2 } + # [1, 2, 3].sum(10, &:count) + # class Sum < Base include RangeHelp extend AutoCorrector @@ -103,6 +114,8 @@ def on_block(node) def handle_sum_candidate(node) sum_candidate?(node) do |method, init, operation| + next if cop_config['OnlySumOrWithInitialValue'] && init.empty? + range = sum_method_range(node) message = build_method_message(node, method, init, operation) diff --git a/spec/rubocop/cop/performance/sum_spec.rb b/spec/rubocop/cop/performance/sum_spec.rb index a3267c543e..a4deb3f9b4 100644 --- a/spec/rubocop/cop/performance/sum_spec.rb +++ b/spec/rubocop/cop/performance/sum_spec.rb @@ -79,6 +79,128 @@ RUBY end + context 'when `OnlySumOrWithInitialValue: true`' do + let(:cop_config) { { 'OnlySumOrWithInitialValue' => true } } + + it "registers an offense and corrects when using `array.#{method}(10, :+)`" do + expect_offense(<<~RUBY, method: method) + array.#{method}(10, :+) + ^{method}^^^^^^^^ Use `sum(10)` instead of `#{method}(10, :+)`. + RUBY + + expect_correction(<<~RUBY) + array.sum(10) + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(10) { |acc, elem| acc + elem }`" do + expect_offense(<<~RUBY, method: method) + array.#{method}(10) { |acc, elem| acc + elem } + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `sum(10)` instead of `#{method}(10) { |acc, elem| acc + elem }`. + RUBY + + expect_correction(<<~RUBY) + array.sum(10) + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(10) { |acc, elem| elem + acc }`" do + expect_offense(<<~RUBY, method: method) + array.#{method}(10) { |acc, elem| elem + acc } + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `sum(10)` instead of `#{method}(10) { |acc, elem| elem + acc }`. + RUBY + + expect_correction(<<~RUBY) + array.sum(10) + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(0, :+)`" do + expect_offense(<<~RUBY, method: method) + array.#{method}(0, :+) + ^{method}^^^^^^^ Use `sum` instead of `#{method}(0, :+)`. + RUBY + + expect_correction(<<~RUBY) + array.sum + RUBY + end + + it "registers an offense and corrects when using `array.#{method} 0, :+`" do + expect_offense(<<~RUBY, method: method) + array.#{method} 0, :+ + ^{method}^^^^^^ Use `sum` instead of `#{method}(0, :+)`. + RUBY + + expect_correction(<<~RUBY) + array.sum + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(0.0, :+)`" do + expect_offense(<<~RUBY, method: method) + array.#{method}(0.0, :+) + ^{method}^^^^^^^^^ Use `sum(0.0)` instead of `#{method}(0.0, :+)`. + RUBY + + expect_correction(<<~RUBY) + array.sum(0.0) + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(init, :+)`" do + expect_offense(<<~RUBY, method: method) + array.#{method}(init, :+) + ^{method}^^^^^^^^^^ Use `sum(init)` instead of `#{method}(init, :+)`. + RUBY + + expect_correction(<<~RUBY) + array.sum(init) + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(0, &:+)`" do + expect_offense(<<~RUBY, method: method) + array.#{method}(0, &:+) + ^{method}^^^^^^^^ Use `sum` instead of `#{method}(0, &:+)`. + RUBY + + expect_correction(<<~RUBY) + array.sum + RUBY + end + + it 'does not register an offense on `&:+` when initial value is not provided' do + expect_no_offenses(<<~RUBY) + array.#{method}(&:+) + RUBY + end + + it 'does not register an offense on array literals' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].#{method}(:+) + RUBY + end + + it 'does not register an offense when the array is empty' do + expect_no_offenses(<<~RUBY) + [].#{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 } + RUBY + end + + it 'does not register an offense when using `sum`' do + expect_no_offenses(<<~RUBY) + array.sum + RUBY + end + end + context 'when `SafeAutoCorrect: true' do let(:cop_config) { { 'SafeAutoCorrect' => true } } @@ -263,5 +385,76 @@ array.#{method}(&:count).sum(&:count) RUBY end + + context 'when `SafeAutoCorrect: true' do + let(:cop_config) { { 'SafeAutoCorrect' => true } } + + it "registers an offense and corrects when using `array.#{method} { |elem| elem ** 2 }.sum`" do + expect_offense(<<~RUBY, method: method) + array.%{method} { |elem| elem ** 2 }.sum + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^ Use `sum { ... }` instead of `%{method} { ... }.sum`. + RUBY + + expect_correction(<<~RUBY) + array.sum { |elem| elem ** 2 } + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(&:count).sum`" do + expect_offense(<<~RUBY, method: method) + array.%{method}(&:count).sum + ^{method}^^^^^^^^^^^^^ Use `sum { ... }` instead of `%{method} { ... }.sum`. + RUBY + + expect_correction(<<~RUBY) + array.sum(&:count) + RUBY + end + + it "registers an offense and corrects when using `#{method}(&:count).sum`" do + expect_offense(<<~RUBY, method: method) + %{method}(&:count).sum + ^{method}^^^^^^^^^^^^^ Use `sum { ... }` instead of `%{method} { ... }.sum`. + RUBY + + expect_correction(<<~RUBY) + sum(&:count) + RUBY + end + + it "registers an offense and corrects when using `array.#{method}(&:count).sum(10)`" do + expect_offense(<<~RUBY, method: method) + array.%{method}(&:count).sum(10) + ^{method}^^^^^^^^^^^^^^^^^ Use `sum(10) { ... }` instead of `%{method} { ... }.sum(10)`. + RUBY + + expect_correction(<<~RUBY) + array.sum(10, &:count) + RUBY + end + + it "registers an offense and corrects when using `array.#{method} { elem ** 2 }.sum(10)`" do + expect_offense(<<~RUBY, method: method) + array.%{method} { |elem| elem ** 2 }.sum(10) + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `sum(10) { ... }` instead of `%{method} { ... }.sum(10)`. + RUBY + + expect_correction(<<~RUBY) + array.sum(10) { |elem| elem ** 2 } + RUBY + end + + it "does not register an offense when using `array.#{method}(&:count).sum { |elem| elem ** 2 }`" do + expect_no_offenses(<<~RUBY) + array.#{method}(&:count).sum { |elem| elem ** 2 } + RUBY + end + + it "does not register an offense when using `array.#{method}(&:count).sum(&:count)`" do + expect_no_offenses(<<~RUBY) + array.#{method}(&:count).sum(&:count) + RUBY + end + end end end