From 07c8ef8dd1c0c2f0b368c8010c17989b3f47883b Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sun, 14 Feb 2021 08:19:02 +0900 Subject: [PATCH] Add new `Style/RedundantSelfAssignmentBranch` cop This PR adds new `Style/RedundantSelfAssignmentBranch` cop. It checks where conditional branch makes redundant self-assignment. ```ruby # bad foo = condition ? bar : foo # good foo = bar if condition ``` Redundancy for self-assignment to `elsif` may also be detected in future, but this PR implements ternary operator-centric detection. --- ...le_redundant_self_assignment_branch_cop.md | 1 + config/default.yml | 5 + lib/rubocop.rb | 1 + lib/rubocop/cop/lint/shadowed_argument.rb | 2 +- .../style/redundant_self_assignment_branch.rb | 88 +++++++ .../redundant_self_assignment_branch_spec.rb | 236 ++++++++++++++++++ 6 files changed, 332 insertions(+), 1 deletion(-) create mode 100644 changelog/new_add_new_style_redundant_self_assignment_branch_cop.md create mode 100644 lib/rubocop/cop/style/redundant_self_assignment_branch.rb create mode 100644 spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb diff --git a/changelog/new_add_new_style_redundant_self_assignment_branch_cop.md b/changelog/new_add_new_style_redundant_self_assignment_branch_cop.md new file mode 100644 index 00000000000..510429c9d7a --- /dev/null +++ b/changelog/new_add_new_style_redundant_self_assignment_branch_cop.md @@ -0,0 +1 @@ +* [#9999](https://github.com/rubocop/rubocop/pull/9999): Add new `Style/RedundantSelfAssignmentBranch` cop. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 3a7ea6ea29e..d322abccf58 100644 --- a/config/default.yml +++ b/config/default.yml @@ -4408,6 +4408,11 @@ Style/RedundantSelfAssignment: Safe: false VersionAdded: '0.90' +Style/RedundantSelfAssignmentBranch: + Description: 'Checks for places where conditional branch makes redundant self-assignment.' + Enabled: pending + VersionAdded: '<>' + Style/RedundantSort: Description: >- Use `min` instead of `sort.first`, diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 4b475009723..63cad4fb9f3 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -512,6 +512,7 @@ require_relative 'rubocop/cop/style/redundant_fetch_block' require_relative 'rubocop/cop/style/redundant_file_extension_in_require' require_relative 'rubocop/cop/style/redundant_self_assignment' +require_relative 'rubocop/cop/style/redundant_self_assignment_branch' require_relative 'rubocop/cop/style/sole_nested_conditional' require_relative 'rubocop/cop/style/static_class' require_relative 'rubocop/cop/style/method_called_on_do_end_block' diff --git a/lib/rubocop/cop/lint/shadowed_argument.rb b/lib/rubocop/cop/lint/shadowed_argument.rb index 1324d664ad0..32c44112262 100644 --- a/lib/rubocop/cop/lint/shadowed_argument.rb +++ b/lib/rubocop/cop/lint/shadowed_argument.rb @@ -141,7 +141,7 @@ def assignment_without_argument_usage(argument) end def reference_pos(node) - node = node.parent.masgn_type? ? node.parent : node + node = node.parent if node.parent.masgn_type? node.source_range.begin_pos end diff --git a/lib/rubocop/cop/style/redundant_self_assignment_branch.rb b/lib/rubocop/cop/style/redundant_self_assignment_branch.rb new file mode 100644 index 00000000000..c9083cd3178 --- /dev/null +++ b/lib/rubocop/cop/style/redundant_self_assignment_branch.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # This cop checks for places where conditional branch makes redundant self-assignment. + # + # @example + # + # # bad + # foo = condition ? bar : foo + # + # # good + # foo = bar if condition + # + # # bad + # foo = condition ? foo : bar + # + # # good + # foo = bar unless condition + # + class RedundantSelfAssignmentBranch < Base + include RangeHelp + extend AutoCorrector + + MSG = 'Remove the self-assignment branch.' + + # @!method bad_method?(node) + def_node_matcher :bad_method?, <<~PATTERN + (send nil? :bad_method ...) + PATTERN + + def on_lvasgn(node) + variable, expression = *node + return unless expression&.if_type? + return unless expression.ternary? || expression.else? + + if_branch = expression.if_branch + else_branch = expression.else_branch + + if self_assign?(variable, if_branch) + register_offense(expression, if_branch, else_branch, 'unless') + elsif self_assign?(variable, else_branch) + register_offense(expression, else_branch, if_branch, 'if') + end + end + + alias on_ivasgn on_lvasgn + alias on_cvasgn on_lvasgn + alias on_gvasgn on_lvasgn + + private + + def self_assign?(variable, branch) + variable.to_s == branch&.source + end + + def register_offense(if_node, offense_branch, opposite_branch, keyword) + add_offense(offense_branch) do |corrector| + if if_node.ternary? + replacement = "#{opposite_branch.source} #{keyword} #{if_node.condition.source}" + corrector.replace(if_node, replacement) + else + if_node_loc = if_node.loc + + range = range_by_whole_lines(offense_branch.source_range, include_final_newline: true) + corrector.remove(range) + range = range_by_whole_lines(if_node_loc.else, include_final_newline: true) + corrector.remove(range) + + autocorrect_if_condition(corrector, if_node, if_node_loc, keyword) + end + end + end + + def autocorrect_if_condition(corrector, if_node, if_node_loc, keyword) + else_branch = if_node.else_branch + + if else_branch.respond_to?(:elsif?) && else_branch.elsif? + corrector.replace(if_node.condition, else_branch.condition.source) + else + corrector.replace(if_node_loc.keyword, keyword) + end + end + end + end + end +end diff --git a/spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb b/spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb new file mode 100644 index 00000000000..09e8955bc9f --- /dev/null +++ b/spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb @@ -0,0 +1,236 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::RedundantSelfAssignmentBranch, :config do + it 'registers and corrects an offense when self-assigning redundant else ternary branch' do + expect_offense(<<~RUBY) + foo = condition ? bar : foo + ^^^ Remove the self-assignment branch. + RUBY + + expect_correction(<<~RUBY) + foo = bar if condition + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant if ternary branch' do + expect_offense(<<~RUBY) + foo = condition ? foo : bar + ^^^ Remove the self-assignment branch. + RUBY + + expect_correction(<<~RUBY) + foo = bar unless condition + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else branch' do + expect_offense(<<~RUBY) + foo = if condition + bar + else + foo + ^^^ Remove the self-assignment branch. + end + RUBY + + expect_correction(<<~RUBY) + foo = if condition + bar + end + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant if branch' do + expect_offense(<<~RUBY) + foo = if condition + foo + ^^^ Remove the self-assignment branch. + else + bar + end + RUBY + + expect_correction(<<~RUBY) + foo = unless condition + bar + end + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else branch and multiline if branch' do + expect_offense(<<~RUBY) + foo = if condition + bar + baz + else + foo + ^^^ Remove the self-assignment branch. + end + RUBY + + expect_correction(<<~RUBY) + foo = if condition + bar + baz + end + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else branch and multiline else branch' do + expect_offense(<<~RUBY) + foo = if condition + foo + ^^^ Remove the self-assignment branch. + else + bar + baz + end + RUBY + + expect_correction(<<~RUBY) + foo = unless condition + bar + baz + end + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else branch and empty if branch' do + expect_offense(<<~RUBY) + foo = if condition + else + foo + ^^^ Remove the self-assignment branch. + end + RUBY + + expect_correction(<<~RUBY) + foo = if condition + end + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else branch and empty else branch' do + expect_offense(<<~RUBY) + foo = if condition + foo + ^^^ Remove the self-assignment branch. + else + end + RUBY + + expect_correction(<<~RUBY) + foo = unless condition + end + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else ternary branch for ivar' do + expect_offense(<<~RUBY) + @foo = condition ? @bar : @foo + ^^^^ Remove the self-assignment branch. + RUBY + + expect_correction(<<~RUBY) + @foo = @bar if condition + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else ternary branch for cvar' do + expect_offense(<<~RUBY) + @@foo = condition ? @@bar : @@foo + ^^^^^ Remove the self-assignment branch. + RUBY + + expect_correction(<<~RUBY) + @@foo = @@bar if condition + RUBY + end + + it 'registers and corrects an offense when self-assigning redundant else ternary branch for gvar' do + expect_offense(<<~RUBY) + $foo = condition ? $bar : $foo + ^^^^ Remove the self-assignment branch. + RUBY + + expect_correction(<<~RUBY) + $foo = $bar if condition + RUBY + end + + it 'does not register an offense when not self-assigning redundant branches' do + expect_no_offenses(<<~RUBY) + foo = condition ? bar : baz + RUBY + end + + it 'does not register an offense when using only if branch' do + expect_no_offenses(<<~RUBY) + foo = if condition + bar + end + RUBY + end + + it 'does not register an offense when multi assignment' do + expect_no_offenses(<<~RUBY) + foo, bar = baz + RUBY + end + + # Ignore method calls as they can have side effects. In other words, it may be unsafe detection. + it 'does not register an offense when lhs is not variable' do + expect_no_offenses(<<~RUBY) + foo.do_something = condition ? foo.do_something : bar.do_something + RUBY + end + + it 'registers and corrects an offense when using `elsif` and self-assigning the value of `then` branch' do + expect_offense(<<~RUBY) + foo = if condition + foo + ^^^ Remove the self-assignment branch. + elsif another_condtion + bar + else + baz + end + RUBY + + expect_correction(<<~RUBY) + foo = if another_condtion + bar + else + baz + end + RUBY + end + + # It may be possible to extend it to register an offense in future. + # auto-correction test patterns should be considered and implemented. + it 'registers and corrects an offense when using `elsif` and self-assigning the value of `elsif` branch' do + expect_no_offenses(<<~RUBY) + foo = if condition + bar + elsif another_condtion + foo + else + baz + end + RUBY + end + + # It may be possible to extend it to register an offense in future. + # auto-correction test patterns should be considered and implemented. + it 'registers and corrects an offense when using `elsif` and self-assigning the value of `else` branch' do + expect_no_offenses(<<~RUBY) + foo = if condition + bar + elsif another_condtion + baz + else + foo + end + RUBY + end +end