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) about 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) about 10 years ago
- Related to Bug #9593: Keyword arguments default argument assignment behaviour not consistent with optional argument added
Updated by hsbt (Hiroshi SHIBATA) about 10 years ago
- Related to Bug #10280: Regression while evaluating default argments of a method added
Updated by matz (Yukihiro Matsumoto) about 10 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) about 10 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) about 10 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) almost 10 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) almost 10 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) almost 10 years ago
tmm1
Please request to backport has_many_association patch into Rails 3.2.
Updated by tmm1 (Aman Karmani) almost 10 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) almost 10 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) almost 10 years ago
It looks like it is better to make it an error than a warning.
Updated by ko1 (Koichi Sasada) almost 10 years ago
- Status changed from Closed to Open
Nobu, what do you think about funny falcon's idea?
Updated by nobu (Nobuyoshi Nakada) almost 10 years ago
I'm in favor of that error.
Updated by olivierlacan (Olivier Lacan) over 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) over 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 naruse (Yui NARUSE) almost 7 years ago
- Target version deleted (
2.2.0)
Updated by jeremyevans0 (Jeremy Evans) about 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) about 5 years ago
I think it's OK now to make this error.
Matz.
Updated by jeremyevans (Jeremy Evans) about 5 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]