Add new ArrayConcatLiteral cop to replace a.concat([1, 2, 3]) with a.push(1, 2, 3)

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][])

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][])

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: '<<next>>'
+
+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: '<<next>>'
+
 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.

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
 )

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

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 `%<current>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

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

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'

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
 

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

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