Project

General

Profile

Actions

Bug #10314

closed

Default argument lookup fails in Ruby 2.2 for circular shadowed variable names

Added by lsegal (Loren Segal) about 10 years ago. Updated about 5 years ago.

Status:
Closed
Target version:
-
ruby -v:
2.2.0-preview1
[ruby-core:65356]

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:

  1. 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.
  2. 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

circular-arg-ref-syntax-error-10314.patch (3.82 KB) circular-arg-ref-syntax-error-10314.patch jeremyevans0 (Jeremy Evans), 08/11/2019 11:26 PM

Related issues 2 (0 open2 closed)

Related to Ruby master - Bug #9593: Keyword arguments default argument assignment behaviour not consistent with optional argumentClosedmatz (Yukihiro Matsumoto)03/05/2014Actions
Related to Ruby master - Bug #10280: Regression while evaluating default argments of a methodRejected09/22/2014Actions

Updated by ko1 (Koichi Sasada) about 10 years ago

Loren Segal wrote:

  1. 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?

  1. 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.

Actions #11

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 to def 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.

Actions #17

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.

Actions #20

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]

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0