False positive from Lint/UselessAssignment with if/unless modifier

[savef]

Variable assignments in if and unless modifiers can be regarded as useless if they are only used in the conditional expression. See the examples below.

---

Expected behavior

The following examples should not be treated as useless assignments.

Actual behavior

The following examples are in fact treated as useless assignments.

Steps to reproduce the problem

Put this in a file and run RuboCop on it:

# frozen_string_literal: true

a.to_f if (a = 123)
b.to_f if /(?<b>\d+)/ =~ '123'

You'll see this:

$ bin/rubocop -D x.rb 
Inspecting 1 file
W

Offenses:

x.rb:3:12: W: Lint/UselessAssignment: Useless assignment to variable - a.
a.to_f if (a = 123)
           ^
x.rb:4:11: W: Lint/UselessAssignment: Useless assignment to variable - b.
b.to_f if /(?<b>\d+)/ =~ '123'
          ^^^^^^^^^^^

1 file inspected, 2 offenses detected

RuboCop version

$ rubocop -V
0.43.0 (using Parser 2.3.1.4, running on ruby 2.3.0 x86_64-linux)

[mikegee]

I confirmed the reported behavior and the following equivalent code does not have UselessAssignment offenses:

if (a = 123)
  a.to_f
end

if /(?<b>\d+)/ =~ '123'
  b.to_f
end

[jonas054]

If you run the program

puts a if (a = 123)

you'll get

/tmp/3591.rb:1: warning: found = in conditional, should be ==
/tmp/3591.rb:1:in `<main>': undefined local variable or method `a' for main:Object (NameError)

and pretty much the same for

puts b if /(?<b>\d+)/ =~ '123'

so it's not entirely wrong to say that it's a useless assignment.

That doesn't mean that there are no bugs in Lint::UselessAssignment, though. Running rubocop on

a = b = nil
puts a if (a = 123)
puts b if /(?<b>\d+)/ =~ '456'

we get

/tmp/3591.rb:1:1: W: Useless assignment to variable - a.
a = b = nil
^
/tmp/3591.rb:1:5: W: Useless assignment to variable - b.
a = b = nil
    ^

and in that case the assignments are necessary to create the scope for a and b.

[dominicsayers]

Still an issue with Rubocop 0.47.1 (using Parser 2.4.0.0, running on ruby 2.4.0 x86_64-linux)

[triplepointfive]

Looks like the same error with loop modifiers, e.g.

d = nil
puts(d) while (d = a.pop)

This gives false complain

3.rb:1:1: W: Lint/UselessAssignment: Useless assignment to variable - d.
d = nil
^

Reproducing in 0.63.1 (using Parser 2.6.0.0, running on ruby 2.6.0 x86_64-darwin18)

[TheDeepestSpace]

Same problem with assignments in map or each:

a = [1, 2, 3, 4]
a.map! { |e| e = 42 }

This code results with:

x.rb:2:11: W: Lint/UnusedBlockArgument: Unused block argument - e. You can omit the argument if you don't care about it.
a.map! { |e| e = 4 }
          ^
x.rb:2:14: W: Lint/UselessAssignment: Useless assignment to variable - e.
a.map! { |e| e = 4 }
             ^

(the first offense is dropped if using something like e = e == 42 ? 0 : e)

0.67.2 (using Parser 2.6.2.0, running on ruby 2.5.1 x86_64-linux)

[mikegee]

@TheDeepestSpace I don't think Rubocop is wrong to complain about your example. This code behaves exactly the same as your example:

a = [1, 2, 3, 4]
a.map! { 42 }

In both examples, a becomes [42, 42, 42, 42] and e is undefined outside the block.

[TheDeepestSpace]

Good catch, @mikegee. Thanks for the clarification.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[stale]

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

[jonas054]

I think I have a solution. PR coming up.

[Drenmi]

Nice one @jonas054! I can review your PR once I am back from my vacation. 🙂