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/RedundantSelfAssignmentBranch cop #9999

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
@@ -0,0 +1 @@
* [#9999](https://github.com/rubocop/rubocop/pull/9999): Add new `Style/RedundantSelfAssignmentBranch` cop. ([@koic][])
5 changes: 5 additions & 0 deletions config/default.yml
Expand Up @@ -4407,6 +4407,11 @@ Style/RedundantSelfAssignment:
Safe: false
VersionAdded: '0.90'

Style/RedundantSelfAssignmentBranch:
Description: 'Checks for places where conditional branch makes redundant self-assignment.'
Enabled: pending
VersionAdded: '<<next>>'

Style/RedundantSort:
Description: >-
Use `min` instead of `sort.first`,
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -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'
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/lint/shadowed_argument.rb
Expand Up @@ -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
Expand Down
88 changes: 88 additions & 0 deletions 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
236 changes: 236 additions & 0 deletions 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