diff --git a/changelog/new_ArrayConcatLiteral.md b/changelog/new_ArrayConcatLiteral.md new file mode 100644 index 0000000000..b04daa08a3 --- /dev/null +++ b/changelog/new_ArrayConcatLiteral.md @@ -0,0 +1 @@ +* [#434](https://github.com/rubocop/rubocop-performance/issues/434): Add `ArrayConcatLiteral` cop, which replaces `array.concat([1, 2, 3])` with `array.push(1, 2, 3)`. ([@amomchilov][]) diff --git a/changelog/new_ArrayPushSingle.md b/changelog/new_ArrayPushSingle.md new file mode 100644 index 0000000000..f13bf50e78 --- /dev/null +++ b/changelog/new_ArrayPushSingle.md @@ -0,0 +1 @@ +* [#431](https://github.com/rubocop/rubocop-performance/issues/431): Add `ArrayPushSingle` cop, which replaces `array.push(x)` with `array << x`. ([@amomchilov][]) diff --git a/config/default.yml b/config/default.yml index a508a39827..86b0d55ab2 100644 --- a/config/default.yml +++ b/config/default.yml @@ -11,6 +11,20 @@ Performance/AncestorsInclude: Safe: false VersionAdded: '1.7' +Performance/ArrayConcatLiteral: + Description: 'Use `array.push(1, 2, 3)` instead of `array.concat([1, 2, 3])`.' + Reference: 'https://github.com/rubocop/rubocop-performance/issues/434' + Enabled: true + Safe: false + VersionAdded: '<>' + +Performance/ArrayPushSingle: + Description: 'Use `array << x` instead of `array.push(x)`.' + Reference: 'https://github.com/rubocop/rubocop-performance/issues/431' + Enabled: true + Safe: false + VersionAdded: '<>' + Performance/ArraySemiInfiniteRangeSlice: Description: 'Identifies places where slicing arrays with semi-infinite ranges can be replaced by `Array#take` and `Array#drop`.' # This cop was created due to a mistake in microbenchmark. diff --git a/lib/rubocop-performance.rb b/lib/rubocop-performance.rb index 37a7ceb49c..ca6d5182c9 100644 --- a/lib/rubocop-performance.rb +++ b/lib/rubocop-performance.rb @@ -13,7 +13,7 @@ RuboCop::Cop::Lint::UnusedMethodArgument.singleton_class.prepend( Module.new do def autocorrect_incompatible_with - super.push(RuboCop::Cop::Performance::BlockGivenWithExplicitBlock) + super << RuboCop::Cop::Performance::BlockGivenWithExplicitBlock end end ) diff --git a/lib/rubocop/cop/performance/array_concat_literal.rb b/lib/rubocop/cop/performance/array_concat_literal.rb new file mode 100755 index 0000000000..2a10acccba --- /dev/null +++ b/lib/rubocop/cop/performance/array_concat_literal.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # Identifies places where concatinating a literal to an array + # can be replaced by pushing its elements directly. + # + # @safety + # This cop is unsafe because not all objects that respond to `#concat` also respond to `#push` + # + # @example + # # bad + # array.concat([1, 2, 3]) + # + # # good + # array.push(1, 2, 3) + # + # # good + # array.concat(not_a_literal) + # + class ArrayConcatLiteral < Base + extend AutoCorrector + + MSG = 'Use `push` instead of concatinating a literal.' + + RESTRICT_ON_SEND = %i[concat].freeze + + def_node_matcher :concat_call?, <<~PATTERN + (call $_receiver :concat $(array ...)) + PATTERN + + def on_send(node) + concat_call?(node) do |receiver, elements| + add_offense(node) do |corrector| + corrector.replace(node, "#{receiver.source}.push(#{remove_brackets(elements)})") + end + end + end + + def on_csend(node) + concat_call?(node) do |receiver, elements| + add_offense(node) do |corrector| + corrector.replace(node, "#{receiver.source}&.push(#{remove_brackets(elements)})") + end + end + end + + private + + # Copied from `Lint/RedundantSplatExpansion` + # TODO: Expose this as a helper API from the base RuboCop gem + PERCENT_W = '%w' + PERCENT_CAPITAL_W = '%W' + PERCENT_I = '%i' + PERCENT_CAPITAL_I = '%I' + + def remove_brackets(array) + array_start = array.loc.begin.source + elements = *array + elements = elements.map(&:source) + + if array_start.start_with?(PERCENT_W) + "'#{elements.join("', '")}'" + elsif array_start.start_with?(PERCENT_CAPITAL_W) + %("#{elements.join('", "')}") + elsif array_start.start_with?(PERCENT_I) + ":#{elements.join(', :')}" + elsif array_start.start_with?(PERCENT_CAPITAL_I) + %(:"#{elements.join('", :"')}") + else + elements.join(', ') + end + end + end + end + end +end diff --git a/lib/rubocop/cop/performance/array_push_single.rb b/lib/rubocop/cop/performance/array_push_single.rb new file mode 100755 index 0000000000..ef2aa18135 --- /dev/null +++ b/lib/rubocop/cop/performance/array_push_single.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # Identifies places where pushing a single element to an array + # can be replaced by `Array#<<`. + # + # @safety + # This cop is unsafe because not all objects that respond to `#push` also respond to `#<<` + # + # @example + # # bad + # array.push(1) + # + # # good + # array << 1 + # + # # good + # array.push(1, 2, 3) # `<<` only works for one element + # + class ArrayPushSingle < Base + extend AutoCorrector + + MSG = 'Use `<<` instead of `%s`.' + + PUSH_METHODS = Set[:push, :append].freeze + RESTRICT_ON_SEND = PUSH_METHODS + + def_node_matcher :push_call?, <<~PATTERN + (call $_receiver $%PUSH_METHODS $!(splat _)) + PATTERN + + def on_send(node) + push_call?(node) do |receiver, method_name, element| + message = format(MSG, current: method_name) + + add_offense(node, message: message) do |corrector| + corrector.replace(node, "#{receiver.source} << #{element.source}") + end + end + end + + def on_csend(node) + push_call?(node) do |receiver, method_name, element| + message = format(MSG, current: method_name) + + add_offense(node, message: message) do |corrector| + corrector.replace(node, "#{receiver.source}&.<< #{element.source}") + end + end + end + end + end + end +end diff --git a/lib/rubocop/cop/performance/redundant_split_regexp_argument.rb b/lib/rubocop/cop/performance/redundant_split_regexp_argument.rb index 659371d124..ec6914fedf 100644 --- a/lib/rubocop/cop/performance/redundant_split_regexp_argument.rb +++ b/lib/rubocop/cop/performance/redundant_split_regexp_argument.rb @@ -48,7 +48,7 @@ def replacement(regexp_node) stack = [] chars = regexp_content.chars.each_with_object([]) do |char, strings| if stack.empty? && char == '\\' - stack.push(char) + stack.push(char) # rubocop:disable Performance/ArrayPushSingle else strings << "#{stack.pop}#{char}" end diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 2a18f26120..5e3363cd16 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -4,6 +4,8 @@ require_relative 'mixin/sort_block' require_relative 'performance/ancestors_include' +require_relative 'performance/array_concat_literal' +require_relative 'performance/array_push_single' require_relative 'performance/array_semi_infinite_range_slice' require_relative 'performance/big_decimal_with_numeric_argument' require_relative 'performance/bind_call' diff --git a/spec/project_spec.rb b/spec/project_spec.rb index 342f226fd0..076705d013 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -71,12 +71,12 @@ supported_key = RuboCop::Cop::Util.to_supported_styles(style_name) valid = config[name][supported_key] unless valid - errors.push("#{supported_key} is missing for #{name}") + errors << "#{supported_key} is missing for #{name}" next end next if valid.include?(style) - errors.push("invalid #{style_name} '#{style}' for #{name} found") + errors << "invalid #{style_name} '#{style}' for #{name} found" end end diff --git a/spec/rubocop/cop/performance/array_concat_literal_spec.rb b/spec/rubocop/cop/performance/array_concat_literal_spec.rb new file mode 100644 index 0000000000..a1a7a51e2a --- /dev/null +++ b/spec/rubocop/cop/performance/array_concat_literal_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::ArrayConcatLiteral, :config do + it 'registers an offense and corrects when using `concat` with array literal' do + expect_offense(<<~RUBY) + array.concat([1, 2, 3]) + ^^^^^^^^^^^^^^^^^^^^^^^ Use `push` instead of concatinating a literal. + RUBY + + expect_correction(<<~RUBY) + array.push(1, 2, 3) + RUBY + end + + it 'registers an offense and corrects when using `concat` with array literal and safe navigation operator' do + expect_offense(<<~RUBY) + array&.concat([1, 2, 3]) + ^^^^^^^^^^^^^^^^^^^^^^^^ Use `push` instead of concatinating a literal. + RUBY + + expect_correction(<<~RUBY) + array&.push(1, 2, 3) + RUBY + end + + describe 'replacing `concat` with `push`' do + it 'returns the same result' do + expect([1, 2, 3].concat(4, 5, 6)).to eq([1, 2, 3].push(4, 5, 6)) + end + + it 'has the same side-effect' do + expected = [1, 2, 3] + expected.push(4, 5, 6) + + actual = [1, 2, 3] + actual.push(4, 5, 6) + + expect(actual).to eq(expected) + end + end +end diff --git a/spec/rubocop/cop/performance/array_push_single_spec.rb b/spec/rubocop/cop/performance/array_push_single_spec.rb new file mode 100755 index 0000000000..7beda8b537 --- /dev/null +++ b/spec/rubocop/cop/performance/array_push_single_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::ArrayPushSingle, :config do + it 'registers an offense and corrects when using `push` with a single element' do + expect_offense(<<~RUBY) + array.push(element) + ^^^^^^^^^^^^^^^^^^^ Use `<<` instead of `push`. + RUBY + + expect_correction(<<~RUBY) + array << element + RUBY + end + + it 'registers an offense and corrects when using `push` with a single element and safe navigation operator' do + expect_offense(<<~RUBY) + array&.push(element) + ^^^^^^^^^^^^^^^^^^^^ Use `<<` instead of `push`. + RUBY + + # gross. TODO: make a configuration option? + expect_correction(<<~RUBY) + array&.<< element + RUBY + end + + it 'does not register an offense when using `push` with multiple elements' do + expect_no_offenses(<<~RUBY) + array.push(1, 2, 3) + RUBY + end + + it 'does not register an offense when using `push` with splatted elements' do + expect_no_offenses(<<~RUBY) + array.push(*elements) + RUBY + end + + # rubocop:disable Performance/ArrayPushSingle + describe 'replacing `push` with `<<`' do + subject(:array) { [1, 2, 3] } + + it 'returns the same result' do + expect([1, 2, 3].push(4)).to eq([1, 2, 3] << 4) + end + + it 'has the same side-effect' do + a = [1, 2, 3] + a << 4 + + b = [1, 2, 3] + b << 4 + + expect(a).to eq(b) + end + end + # rubocop:enable Performance/ArrayPushSingle +end