Skip to content

Commit

Permalink
[Fix #434]: prefer push over concat literal
Browse files Browse the repository at this point in the history
  • Loading branch information
amomchilov committed Jan 12, 2024
1 parent fca972f commit ae2d49e
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/new_ArrayConcatLiteral.md
Original file line number Diff line number Diff line change
@@ -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][])
7 changes: 7 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ 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'
Expand Down
78 changes: 78 additions & 0 deletions lib/rubocop/cop/performance/array_concat_literal.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions lib/rubocop/cop/performance_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
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'
Expand Down
41 changes: 41 additions & 0 deletions spec/rubocop/cop/performance/array_concat_literal_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit ae2d49e

Please sign in to comment.