Should Lint/ShadowingOuterLocalVariable ignore when assigning with #tap?

[Drenmi]

When assigning with #tap, it makes sense to use the same parameter name as the variable being assigned (because #tap opens a block parameterized with the receiver.) Thus this might be a special case. Having to come up with a new name for the same thing is confusing and leads to convoluted naming.

There is nothing to shadow, really, since the outer local variable is being assigned in this same statement.

---

Expected behavior

RuboCop does not register an offense.

Actual behavior

RuboCop registers an offense.

Steps to reproduce the problem

Run rubocop on:

foo = Hash.new.tap do |foo|
  foo[:bar] = :baz if qux?
end

RuboCop version

$ rubocop -V
rubocop (master) $ rubocop -V
0.55.0 (using Parser 2.5.1.0, running on ruby 2.5.1 x86_64-darwin16)

[bbatsov]

Or we can make this more generic so people can just exclude whatever block taking methods they want.

[mikegee]

I wouldn't want to just exclude tap blocks from this offense. I think it is important that the return value is immediately assigned to the local variable that was shadowed, like in the provided example. Only then is the shadowing offense meaningless.

[Drenmi]

@mikegee We would need to add the criteria that the variable is not previously assigned, as it will retain any previous value until the statement returns. 🤔

[drbrain]

If a variable appears in the local variable table and the block variable table because of the same expression there should be no shadowing error.

This warning was removed entirely in Ruby 2.6 because of methods like #find, #tap, etc. where re-use of the name is expected:

https://bugs.ruby-lang.org/issues/12490

Even in versions of ruby which had the shadowing warning the reasoning behind the warning doesn't hold because you won't end up with an unexpected value because the assignment overwrites the "shadowed" value:

$ ruby -wve 'a = [1, 2].find { |a| false }; p a'
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin18]
nil
$ ruby18 -wve 'a = [1, 2].find { |a| false }; p a'
ruby 1.8.7 (2013-06-27 patchlevel 374) [i686-darwin14.0.0]
nil

Unlike this dangerous use of shadowing which would cause unexpected behavior when moving to ruby 1.9+:

$ ruby -wve 'a = 0; [1, 2].find { |a| false }; p a'
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin18]
0
$ ruby18 -ve 'a = 0; [1, 2].find { |a| false }; p a'
ruby 1.8.7 (2013-06-27 patchlevel 374) [i686-darwin14.0.0]
2

[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!

[drbrain]

Are any of the contributors going to come back and look at this?

[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!

[drbrain]

Are any of the contributors going to come back and look at this?

[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!

[drbrain]

Are any of the contributors going to come back and look at this?

[mikegee]

Are any of the contributors going to come back and look at this?

It's not looking good so far. Do you have any ideas that could streamline the fix?

[drbrain]

I took the time to research this issue and noted above that the warning has been removed for ruby 2.6+ as it was emitted when shadowing wasn't present.

No collaborator has weighed in on it since to indicate acceptance of any proposed change, nor indicated a willingness to guide someone else in fixing it so I'm not sure what else can be done about it other than say "yea, it's still broken, when are you going to fix it?" every time the stale bot comes around.

[denys281]

So what should be changed? Should it be fixed case only with tap?

Btw in that ruby issue:

Lint tools including RuboCop can report the warning instead of ruby

[drbrain]

The check should mention "shadowing" anywhere a variable named n does not exist before an expression where the return value of some method is assigned to a variable named n and the variable named n is also used as a block variable used by method, for example:

# some other code in this scope that does not assign to n

n = [1, 2].find { |n| false }

In the above there is no shadowing as the assignment to n as a local variable occurs after some.method { |n| … } has completed. The two values of n cannot conflict or be shadowed as they do not exist simultaneously.

If people want some check for this type of expression it should not mention "shadowing" as nothing is shadowed. The order of execution is right-to-left (expression on right side of =, then assignment to n on left side of =), not left-to-right.

Here is a program that shadows the variable n:

n = 0

x = [1, 2].find { |n| false }

As noted above, n would be 2 for ruby 1.8.

In modern ruby the n in the outer scope does not get reassigned by the assignment to the block variable n:

$ ruby
n = 0

[1, 2].find { |n| p find_n: n; false }

p outer_n: n
⌃D
{:find_n=>1}
{:find_n=>2}
{:outer_n=>0}
$

But the value for n inside the block still shadows the value outside the block and may be unexpected.

[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!

[adfoster-r7]

Also running into the find scenario that a few have pointed out:

    target = targets.find { |target| .... }

[marcandre]

@drbrain

In the above there is no shadowing as the assignment to n as a local variable occurs after some.method { |n| … } has completed. The two values of n cannot conflict or be shadowed as they do not exist simultaneously.

That is incorrect. The "outside" variable can be referred to from inside the block, in particular:

recurse = ->(n) { puts(n); recurse.call(n-1) if n > 0 }
recurse.call(10) # => Prints 10 to 0

So there is shadowing.
That being said, it would make sense to relax the rule for that exact pattern, or a config option, probably turned on by default.

[Tao-Galasse]

any news on this?

It indeed seems kinda obvious to use the same variable name with #tap, it would be great to fix this! :) 🙏

[mikegee]

I don't think there's any debate on using the same variable name with #tap. If you want to work on a PR, I can try to help get it through.

Looking back over the discussion here, I see folks debating whether or not shadowing is really happening or not. We might want to refine our definition of "shadowing". One way to look at it is: using the same variable name in the block makes the outer local variable unavailable inside the block. That's a kind of shadowing too. I don't think the cop is wrong to point this out, but I do support allowing some shadowing when it is obviously ok like assigning to the local variable (frequently with #tap).