Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Style/SwapValues cop #8934

Merged
merged 1 commit into from Oct 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -5,6 +5,7 @@
### New features

* [#8895](https://github.com/rubocop-hq/rubocop/pull/8895): Add new `Lint/EmptyBlock` cop. ([@fatkodima][])
* [#8934](https://github.com/rubocop-hq/rubocop/pull/8934): Add new `Style/SwapValues` cop. ([@fatkodima][])
* [#7549](https://github.com/rubocop-hq/rubocop/issues/7549): Add new `Style/ArgumentsForwarding` cop. ([@koic][])
* [#8859](https://github.com/rubocop-hq/rubocop/issues/8859): Add new `Lint/UnmodifiedReduceAccumulator` cop. ([@dvandersluis][])

Expand Down
7 changes: 7 additions & 0 deletions config/default.yml
Expand Up @@ -4244,6 +4244,13 @@ Style/StructInheritance:
VersionAdded: '0.29'
VersionChanged: '0.86'

Style/SwapValues:
Description: 'This cop enforces the use of shorthand-style swapping of 2 variables.'
StyleGuide: '#values-swapping'
Enabled: pending
VersionAdded: '1.1'
SafeAutoCorrect: false

Style/SymbolArray:
Description: 'Use %i or %I for arrays of symbols.'
StyleGuide: '#percent-i'
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -509,6 +509,7 @@ In the following section you find all available cops:
* xref:cops_style.adoc#stylestringmethods[Style/StringMethods]
* xref:cops_style.adoc#stylestrip[Style/Strip]
* xref:cops_style.adoc#stylestructinheritance[Style/StructInheritance]
* xref:cops_style.adoc#styleswapvalues[Style/SwapValues]
* xref:cops_style.adoc#stylesymbolarray[Style/SymbolArray]
* xref:cops_style.adoc#stylesymbolliteral[Style/SymbolLiteral]
* xref:cops_style.adoc#stylesymbolproc[Style/SymbolProc]
Expand Down
33 changes: 33 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Expand Up @@ -9897,6 +9897,39 @@ end

* https://rubystyle.guide#no-extend-struct-new

== Style/SwapValues

|===
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Pending
| Yes
| Yes (Unsafe)
| 1.1
| -
|===

This cop enforces the use of shorthand-style swapping of 2 variables.
Its autocorrection is marked as unsafe, because it can erroneously remove
the temporary variable which is used later.

=== Examples

[source,ruby]
----
# bad
tmp = x
x = y
y = tmp

# good
x, y = y, x
----

=== References

* https://rubystyle.guide#values-swapping

== Style/SymbolArray

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -557,6 +557,7 @@
require_relative 'rubocop/cop/style/string_methods'
require_relative 'rubocop/cop/style/strip'
require_relative 'rubocop/cop/style/struct_inheritance'
require_relative 'rubocop/cop/style/swap_values'
require_relative 'rubocop/cop/style/symbol_array'
require_relative 'rubocop/cop/style/symbol_literal'
require_relative 'rubocop/cop/style/symbol_proc'
Expand Down
108 changes: 108 additions & 0 deletions lib/rubocop/cop/style/swap_values.rb
@@ -0,0 +1,108 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# This cop enforces the use of shorthand-style swapping of 2 variables.
# Its autocorrection is marked as unsafe, because it can erroneously remove
# the temporary variable which is used later.
#
# @example
# # bad
# tmp = x
# x = y
# y = tmp
#
# # good
# x, y = y, x
#
class SwapValues < Base
include RangeHelp
extend AutoCorrector

MSG = 'Replace this and assignments at lines %<x_line>d '\
'and %<y_line>d with `%<replacement>s`.'

SIMPLE_ASSIGNMENT_TYPES = %i[lvasgn ivasgn cvasgn gvasgn casgn].to_set.freeze

def on_asgn(node)
return if allowed_assignment?(node)

tmp_assign = node
x_assign, y_assign = *node.right_siblings.take(2)
return unless x_assign && y_assign && swapping_values?(tmp_assign, x_assign, y_assign)

add_offense(node, message: message(x_assign, y_assign)) do |corrector|
range = correction_range(tmp_assign, y_assign)
corrector.replace(range, replacement(x_assign))
end
end

SIMPLE_ASSIGNMENT_TYPES.each { |asgn_type| alias_method :"on_#{asgn_type}", :on_asgn }

private

def allowed_assignment?(node)
node.parent&.mlhs_type? || node.parent&.shorthand_asgn?
end

def swapping_values?(tmp_assign, x_assign, y_assign)
simple_assignment?(tmp_assign) &&
simple_assignment?(x_assign) &&
simple_assignment?(y_assign) &&
lhs(x_assign) == rhs(tmp_assign) &&
lhs(y_assign) == rhs(x_assign) &&
rhs(y_assign) == lhs(tmp_assign)
end

def simple_assignment?(node)
SIMPLE_ASSIGNMENT_TYPES.include?(node.type)
end

def message(x_assign, y_assign)
format(
MSG,
x_line: x_assign.first_line,
y_line: y_assign.first_line,
replacement: replacement(x_assign)
)
end

def replacement(x_assign)
x = lhs(x_assign)
y = rhs(x_assign)
"#{x}, #{y} = #{y}, #{x}"
end

def lhs(node)
case node.type
when :casgn
namespace, name, = *node
if namespace
"#{namespace.const_name}::#{name}"
else
name.to_s
end
else
node.children[0].to_s
end
end

def rhs(node)
case node.type
when :casgn
node.children[2].source
else
node.children[1].source
end
end

def correction_range(tmp_assign, y_assign)
range_by_whole_lines(
range_between(tmp_assign.source_range.begin_pos, y_assign.source_range.end_pos)
)
end
end
end
end
end
57 changes: 57 additions & 0 deletions spec/rubocop/cop/style/swap_values_spec.rb
@@ -0,0 +1,57 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Style::SwapValues do
subject(:cop) { described_class.new }

shared_examples 'verbosely swapping' do |type, x, y, correction|
it "registers an offense and corrects when verbosely swapping #{type} variables" do
expect_offense(<<~RUBY, x: x)
tmp = %{x}
^^^^^^^{x} Replace this and assignments at lines 2 and 3 with `#{correction}`.
#{x} = #{y}
#{y} = tmp
RUBY

expect_correction(<<~RUBY)
#{correction}
RUBY
end
end

it_behaves_like('verbosely swapping', 'local', 'x', 'y', 'x, y = y, x')
it_behaves_like('verbosely swapping', 'global', '$x', '$y', '$x, $y = $y, $x')
it_behaves_like('verbosely swapping', 'instance', '@x', '@y', '@x, @y = @y, @x')
it_behaves_like('verbosely swapping', 'class', '@@x', '@@y', '@@x, @@y = @@y, @@x')
it_behaves_like('verbosely swapping', 'constant', 'X', 'Y', 'X, Y = Y, X')
it_behaves_like('verbosely swapping', 'constant with namespaces',
'::X', 'Foo::Y', '::X, Foo::Y = Foo::Y, ::X')
it_behaves_like('verbosely swapping', 'mixed', '@x', '$y', '@x, $y = $y, @x')

it 'handles comments when correcting' do
expect_offense(<<~RUBY)
tmp = x # comment 1
^^^^^^^ Replace this and assignments at lines 3 and 4 with `x, y = y, x`.
# comment 2
x = y
y = tmp # comment 3
RUBY

expect_correction(<<~RUBY)
x, y = y, x
RUBY
end

it 'does not register an offense when idiomatically swapping variables' do
expect_no_offenses(<<~RUBY)
x, y = y, x
RUBY
end

it 'does not register an offense when almost swapping variables' do
expect_no_offenses(<<~RUBY)
tmp = x
x = y
y = not_a_tmp
RUBY
end
end