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

Bug with Style/RedundantSelfAssignmentBranch #10024

Closed
jimgraham opened this issue Aug 18, 2021 · 0 comments · Fixed by #10027
Closed

Bug with Style/RedundantSelfAssignmentBranch #10024

jimgraham opened this issue Aug 18, 2021 · 0 comments · Fixed by #10027
Labels

Comments

@jimgraham
Copy link

Multiline conditional assignments are firing the Style/RedundantSelfAssignmentBranch rule. If this rule is applied, it modifies the conditional. In the falsey case, those conditionals evaluate to nil, and then nil is assigned (instead of no assignment).


Expected behavior

The Style/RedundantSelfAssignmentBranch does not fire for multiline if/else conditional assignments. Basically the cases covered by

07c8ef8#diff-9aa30a608c9b3f7d4c969285ada36ce789273d243e71262eeac97ae5a93ae1adR26-R34

Actual behavior

The rules do fire, leading to incorrect code

Steps to reproduce the problem

Using the configuration

Style/RedundantSelfAssignmentBranch:
  Enabled: true

Style/IfUnlessModifier:
  Enabled: false

to isolate the behavior to only this cop,

  1. Create test file:
# frozen_string_literal: true
# join_if.rb

require 'test/unit/assertions'
include Test::Unit::Assertions

url = 'foo'
path = nil

url = if path
        [url, path].join('/')
      else
        url
      end

assert_equal('foo', url)
  1. run the file ruby join_if.rb. Test passes

  2. Run bundle exec rubocop -a with Style/RedundantSelfAssignmentBranch enabled. Cop reports

join_if.rb:12:9: C: [Corrected] Style/RedundantSelfAssignmentBranch: Remove the self-assignment branch.
        url
        ^^^

2 files inspected, 2 offenses detected, 1 offense corrected
  1. Code is modified to
# frozen_string_literal: true
# join_if.rb

require 'test/unit/assertions'
include Test::Unit::Assertions

url = 'foo'
path = nil

url = if path
        [url, path].join('/')
      end

assert_equal('foo', url)
  1. Run test again
  2. Test fails

The modification fails because the multiline conditional expression returns nil and that's what is assigned to the variable.

If the change recommended were instead (which perhaps is what Style/IfUnlessModifier does)

url = [url, path].join('/') if path

it would be correct, but as recommended / auto-corrected, it is not.

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler.
If you see extension cop versions (e.g. rubocop-performance, rubocop-rspec, and others)
output by rubocop -V, include them as well. Here's an example:

$ bundle exec rubocop -V
1.19.0 (using Parser 3.0.2.0, rubocop-ast 1.10.0, running on ruby 2.7.1 x86_64-darwin20)
@koic koic added the bug label Aug 19, 2021
koic added a commit to koic/rubocop that referenced this issue Aug 19, 2021
…tSelfAssignmentBranch`

Fixes rubocop#10024.

This PR fixes an incorrect auto-correct for `Style/RedundantSelfAssignmentBranch`
when using multiline `if` / `else` conditional assignment.
bbatsov pushed a commit that referenced this issue Aug 19, 2021
…signmentBranch`

Fixes #10024.

This PR fixes an incorrect auto-correct for `Style/RedundantSelfAssignmentBranch`
when using multiline `if` / `else` conditional assignment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants