Skip to content

Commit

Permalink
Add new Style/RedundantSelfAssignmentBranch cop
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
koic committed Aug 11, 2021
1 parent bb5d832 commit 178e0a0
Show file tree
Hide file tree
Showing 6 changed files with 332 additions and 1 deletion.
@@ -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

0 comments on commit 178e0a0

Please sign in to comment.