Bug #10314
closedDefault argument lookup fails in Ruby 2.2 for circular shadowed variable names
Description
The following code prints nil
in Ruby 2.2.0-preview1 but worked in all previous version of Ruby back to 1.8.7:
class Foo
def foo; "abc" end
# this default param should resolve at runtime to the #foo method call
def run(foo = foo)
p foo # print shadowed local var defaulting to attr value
end
end
puts "Testing #{RUBY_VERSION}:"
Foo.new.run
# Ruby 2.2.0-preview1
# => nil
# Ruby 1.x 2.x etc
# => "abc"
My guess is this is happening because "foo" in "foo = foo" is resolving to the argument variable "foo", which currently has the value of nil. It would be equivalent to setting "qux = qux" in a method body, which has been the expected behavior for a long time.
I understand that shadowing variables is something you should probably never do, but unfortunately this code was already written and working for quite a while, so I figured it would be wise to file a bug report for the following reasons:
- This seems like a breaking change in Ruby 2.2.0-preview1 that was not announced in the changelog. My guess is this change may have been unintentional, but if it was we need a changelog entry at the very least.
- If this is newly expected behavior, I wanted to chime in that I don't think it makes much sense. I can't think of any time when a user would expect the default value of a "foo = foo" argument to be the same foo argument itself. That would be tautologically nil. Arguably, it doesn't make much sense inside of a method body either when there is a shadowed method that could be called instead.
Files
Updated by ko1 (Koichi Sasada) almost 10 years ago
Loren Segal wrote:
- This seems like a breaking change in Ruby 2.2.0-preview1 that was not announced in the changelog. My guess is this change may have been unintentional, but if it was we need a changelog entry at the very least.
I agree. How about to show a warning?
- If this is newly expected behavior, I wanted to chime in that I don't think it makes much sense. I can't think of any time when a user would expect the default value of a "foo = foo" argument to be the same foo argument itself. That would be tautologically nil. Arguably, it doesn't make much sense inside of a method body either when there is a shadowed method that could be called instead.
I think this change is to make same as method body ("foo = foo" in method body).
I'm not sure your proposal is natural or not (matz issue?).
I like current behavior because it is simple (define a local variable foo' after the sentence
foo=').
Updated by hsbt (Hiroshi SHIBATA) almost 10 years ago
- Related to Bug #9593: Keyword arguments default argument assignment behaviour not consistent with optional argument added
Updated by hsbt (Hiroshi SHIBATA) almost 10 years ago
- Related to Bug #10280: Regression while evaluating default argments of a method added
Updated by matz (Yukihiro Matsumoto) over 9 years ago
- Status changed from Open to Rejected
In other part of Ruby, foo = foo
makes foo to nil.
To get old behavior, try foo = foo()
.
Matz.
Updated by ko1 (Koichi Sasada) over 9 years ago
- Status changed from Rejected to Assigned
- Assignee set to nobu (Nobuyoshi Nakada)
Maybe we need warning() strongly (without verbose).
Updated by nobu (Nobuyoshi Nakada) over 9 years ago
- Status changed from Assigned to Closed
- % Done changed from 0 to 100
Applied in changeset r48188.
parse.y: warn circular argument reference
- parse.y (gettable_gen): warn circular argument reference, for
transition from 2.1 and earlier. [ruby-core:65990] [Bug #10314]
Updated by tmm1 (Aman Karmani) over 9 years ago
This change in behavior breaks rails 3.2, for example here: https://github.com/github/rails/blob/3-2-github/activerecord/lib/active_record/associations/has_many_association.rb#L53-L55
In this case, I think the intended behavior is def has_cached_counter?(reflection = self.reflection)
to access the reflection attributed defined in the superclass: https://github.com/github/rails/blob/3-2-github/activerecord/lib/active_record/associations/association.rb#L21
Is adding an explicit self.
prefix necessary to preserve compatibility in 2.2 and forward?
Updated by tmm1 (Aman Karmani) over 9 years ago
I see, @matz (Yukihiro Matsumoto) recommended foo = foo()
. I guess this is a breaking change in ruby 2.2.
Updated by hsbt (Hiroshi SHIBATA) over 9 years ago
tmm1
Please request to backport has_many_association patch into Rails 3.2.
Updated by tmm1 (Aman Karmani) over 9 years ago
I opened a pull request to add ruby 2.2 compatibility to rails 3-2-stable: https://github.com/rails/rails/pull/18160.
In our large rails app, I only had to fix 5 function definitions, by adding parenthesis to 4 of them. The warnings made it very easy to find and fix the code.
Updated by usa (Usaku NAKAMURA) over 9 years ago
- Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN to 2.0.0: DONTNEED, 2.1: DONTNEED
Updated by funny_falcon (Yura Sokolov) over 9 years ago
It looks like it is better to make it an error than a warning.
Updated by ko1 (Koichi Sasada) over 9 years ago
- Status changed from Closed to Open
Nobu, what do you think about funny falcon's idea?
Updated by nobu (Nobuyoshi Nakada) over 9 years ago
I'm in favor of that error.
Updated by olivierlacan (Olivier Lacan) about 7 years ago
nobu (Nobuyoshi Nakada) wrote:
I'm in favor of that error.
Do you still want to implement this error?
Also, wasn't def run(foo = self.foo)
a viable alternative to def run(foo = foo())
to avoid circular reference warnings?
Empty parens tend to trigger errors from tools like Rubocop.
Updated by nobu (Nobuyoshi Nakada) about 7 years ago
olivierlacan (Olivier Lacan) wrote:
Also, wasn't
def run(foo = self.foo)
a viable alternative todef run(foo = foo())
to avoid circular reference warnings?
Unless #foo
is private.
Empty parens tend to trigger errors from tools like Rubocop.
Then you should report it to Rubocop, not here.
Updated by jeremyevans0 (Jeremy Evans) almost 5 years ago
Attached is a patch to turn circular argument reference from a warning to a SyntaxError. Do we still want to do that?
Updated by matz (Yukihiro Matsumoto) almost 5 years ago
I think it's OK now to make this error.
Matz.
Updated by jeremyevans (Jeremy Evans) over 4 years ago
- Status changed from Open to Closed
Applied in changeset git|0162e7e6471b639dfeeded29943e9e27c9519826.
Make circular argument reference a SyntaxError instead of a warning
Fixes [Bug #10314]