Misc #15364: Suppress "shadowing outer local variable" warning for assignments e.g. with "tap"

• Status: Closed
• Assignee: -

[svoop (Sven Schwyn)]

The following pattern is quite common when initializing a complex object where some attributes (attr1 and 2) can be set by the initializer, whereas others (attr3 and 4) cannot:

# the local variable "thing" does not exist up to this point
thing = Thing.new(
  attr1: 'foo',
  attr2: 'bar',
).tap do |thing|
  attr3 = 'foz',
  attr4 = 'baz'
end

The syntax check with ruby -cw won't like this:

warning: shadowing outer local variable - thing

While a very useful warning in many situations, it is obsolete when the outer local variable doesn't exist up to this point and therefore no collision with the inner local variable of the same name can occur.

Of course, just change the inner variable name to silence the warning, but for the sake of readable code, it should be okay to "recycle" the same variable name both outer and inner (since they refer to the same thing).

(I don't know whether the syntax check is capable of telling whether "thing" is defined already or not though.)

[svoop (Sven Schwyn)]

• Tracker changed from Bug to Misc
• ruby -v deleted (any)
• Backport deleted (2.4: UNKNOWN, 2.5: UNKNOWN)

[shevegen (Robert A. Heiler)]

This is similar as to what was mentioned in one of the last developer meeting
(or rather the one in October 2018, I think).

Let me link to that:

https://docs.google.com/document/d/18f8peqDQ_nkLapK3BWFaAkEHaAIvD6hP0IhdkgDgTJI/pub

Relevant Quote:

[Feature #12490] Remove warning on shadowing block params

mame: It was introduced when backward incompatibility was involved, but it is no longer a problem, so why not remove it?
knu: It's painful when you have to change this just to silence the warning: user = users.find { |user| ... }
matz: I still think it’s not preferable to pick a conflicting name, but I understand your pain. Approved.

So what I gather from this is that matz prefers telling the ruby user/developer at hand
that there may be a conflicting name; but he also understands the situation of the same
(variable) names used. And I think the situation here, about .tap, is similar to this.

Personally I think it may thus be reasonable to assume that matz would apply the same
decision to the situation here, as was done in .find.

Of course, just change the inner variable name to silence the warning [...]

This is what I am doing. :)

I usually prefix that variable name via "inner_". I have no problem with that either,
by the way, but it also leads to slightly longer code, so I can understand the
comments by all who want to be able to use shorter names as well. (And the use
cases described, I think, are really existing; such as this one here too.)

I don't know how busy matz is, or anyone else from the ruby core team, but in the
event this issue may be open, I would suggest to you to add it to the next
upcoming developer meeting:

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

It seems a very small change.

As for the overall topic, that is how ruby warns, and how verbose the warnings
are; and if we also look at older comments such as the reverse stack trace (which
may confuse some people); but also looking at functionality such as the one added
by the did-you-mean gem, I think in the long run it would be beneficial for ruby
to allow for more flexibility at the least for warnings in general. I completely
understand matz' wanting to provide help but at the same time, I think it should
be possible for ruby users to tell ruby how verbose it ought to be, and what it
should warn about and what not. We currently can do so in some ways already,
e. g. fiddling around with $VERBOSE, but it is not very elegant and it may be
better to have something like a public API or similar to control how the warnings
are displayed, and if they are displayed. A bit like $SAFE, just for warnings,
but ideally without global variables. :)

But anyway, this is for another discussion I think - sorry for side tracking here,
my ideas are mostly related to "long term changes", perhaps even post ruby 3.x.

Edit: Actually, this may have been approved already by matz in that other issue,
so perhaps this issue here falls under issue 15342 as well.

[mame (Yusuke Endoh)]

• Status changed from Open to Closed

This warning has been already removed at r65369, and Ruby 2.6 won't print it. This check is possible in an external lint tool like Rubocop.