diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a2f67f9cfa..09aaad31555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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][]) diff --git a/config/default.yml b/config/default.yml index f77265b10ae..a4aa74316cc 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 8661dffb295..a5dc4552690 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -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] diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index f28dc3807c9..769a4fee49b 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -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 |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 481d48c164a..a2c86564e8f 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -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' diff --git a/lib/rubocop/cop/style/swap_values.rb b/lib/rubocop/cop/style/swap_values.rb new file mode 100644 index 00000000000..730ac212718 --- /dev/null +++ b/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 %d '\ + 'and %d with `%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 diff --git a/spec/rubocop/cop/style/swap_values_spec.rb b/spec/rubocop/cop/style/swap_values_spec.rb new file mode 100644 index 00000000000..b19a750b91e --- /dev/null +++ b/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