Bug #18011: `Method#parameters` is incorrect for forwarded arguments

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [arm64-darwin20]
• Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN

[josh.cheek (Josh Cheek)]

When asking a method about its parameters, forwarded arguments say they are a rest and a block (wrapper1 in the example below).
However, when we use that signature, it raises an ArgumentError (wrapper2 in the example below).
When I add a keyrest to the signature, it works as expected (wrapper3 in the example below).
So I think forwarded arguments should include a keyrest in the output of Method#parameters

def wrapped(ord, kw:) = [ord, {kw: kw}]

methods = [
 def wrapper1(...)         = wrapped(...),
 def wrapper2(*r, &b)      = wrapped(*r, &b),
 def wrapper3(*r, **k, &b) = wrapped(*r, **k, &b),
]

methods.each do |name|
  puts File.read(__FILE__)[/#{name}\(.*?\)/]
  puts "  params: #{method(name).parameters.inspect}"
  puts "  result: #{(method(name).call 123, kw: 456 rescue $!).inspect}"
  puts
end

Output:

wrapper1(...)
  params: [[:rest, :*], [:block, :&]]
  result: [123, {:kw=>456}]

wrapper2(*r, &b)
  params: [[:rest, :r], [:block, :b]]
  result: #<ArgumentError: wrong number of arguments (given 2, expected 1; required keyword: kw)>

wrapper3(*r, **k, &b)
  params: [[:rest, :r], [:keyrest, :k], [:block, :b]]
  result: [123, {:kw=>456}]

[jeremyevans0 (Jeremy Evans)]

Forwarded arguments is implemented as (*args, &block) with ruby2_keywords, not as (*args, **kwargs, &block). If you add def wrapper4(*r, &b) = wrapped(*r, &b), to methods and then ruby2_keywords :wrapper4 after methods, you can see it gives the same output as wrapper1 and wrapper3.

If we want to indicate which methods have ruby2_keywords, one way would be to include ruby2_keywords into Method#parameters output if it is enabled for the method. Possibly this could be done by adding [:ruby2_keywords] as the last value in the returned array.

[josh.cheek (Josh Cheek)]

jeremyevans0 (Jeremy Evans) wrote in #note-1:

Forwarded arguments is implemented as (*args, &block) with ruby2_keywords, not as (*args, **kwargs, &block). If you add def wrapper4(*r, &b) = wrapped(*r, &b), to methods and then ruby2_keywords :wrapper4 after methods, you can see it gives the same output as wrapper1 and wrapper3.

If we want to indicate which methods have ruby2_keywords, one way would be to include ruby2_keywords into Method#parameters output if it is enabled for the method. Possibly this could be done by adding [:ruby2_keywords] as the last value in the returned array.

Thanks for the reply, I didn't know about the ruby2_keywords method. I guess if the people here disagree with me, then they can close this issue.

I guess I can detect this by the argument names of :* and :&, there doesn't seem to be any other way to get that.

---

For reference to any readers, he's saying to add another example like this:

def wrapped(ord, kw:) = [ord, {kw: kw}]

methods = [
  # ...
  def wrapper4(*r, &b)      = wrapped(*r, &b),
]

ruby2_keywords :wrapper4

methods.each do |name|
  # ...
end

Output:

# ...
wrapper4(*r, &b)
  params: [[:rest, :r], [:block, :b]]
  result: [123, {:kw=>456}]

---

Maybe worth noting that Ripper also parses it into the rest arg:

ruby -vr ripper -e '
  def args_for(sig) = %i(req opt rest req key keyrest block).zip(Ripper.sexp(sig)[1][0][2][1][1..])
  pp args_for "def m(...) end"
  pp args_for "def m(*, **) end"
'
ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [arm64-darwin20]
[[:req, nil],
 [:opt, nil],
 [:rest, [:args_forward]],
 [:req, nil],
 [:key, nil],
 [:keyrest, nil],
 [:block, nil]]
[[:req, nil],
 [:opt, nil],
 [:rest, [:rest_param, nil]],
 [:req, nil],
 [:key, nil],
 [:keyrest, [:kwrest_param, nil]],
 [:block, nil]]

[Eregon (Benoit Daloze)]

IMHO in Ruby 3+ it should return [[:rest, :*], [:keyrest, :**], [:block, :&]].

The fact that it uses ruby2_keywords internally is an implementation detail that I don't think needs to leak into Method#parameters.

[jeremyevans0 (Jeremy Evans)]

Eregon (Benoit Daloze) wrote in #note-3:

IMHO in Ruby 3+ it should return [[:rest, :*], [:keyrest, :**], [:block, :&]].

The fact that it uses ruby2_keywords internally is an implementation detail that I don't think needs to leak into Method#parameters.

I think methods that use ruby2_keywords, either explicitly or implicitly through argument forwarding, should handle parameters consistently. However, I'm fine with all ruby2_keywords methods adding [:keyrest, :**] as opposed to adding [:ruby2_keywords]. The :keyrest approach may be more backwards compatible.

Note that from a caller's perspective, [[:rest, :*]] and [[:rest, :*], [:keyrest, :**]] have identical behavior. Both accept an arbitrary number of positional argument and arbitrary keywords.

[josh.cheek (Josh Cheek)]

jeremyevans0 (Jeremy Evans) wrote in #note-4:

Note that from a caller's perspective, [[:rest, :*]] and [[:rest, :*], [:keyrest, :**]] have identical behavior. Both accept an arbitrary number of positional argument and arbitrary keywords.

Do you have thoughts on super? I can' t say you'll necessarily think differently in that case, but that was the context it initially arose in, and I switched it over to method wrapping because it made the example shorter. In the case of super, it is less clear to me that these can be considered equivalent, because I haven't touched the arguments, so I'm still thinking about them in a black-box sort of way, I'm definitely thinking about it from the caller's perspective (in my brain, personally, not as a general statement), so it's especially confusing when one works and one fails.

RUBY_VERSION # => "3.0.1"

# Identical parent methods
def m1(a:) = 'success'
def m2(a:) = 'success'

# Child methods with identical bodies
def self.m1(*r, &b) = super
def self.m2(...)    = super

# And allegedly identical parameters
method(:m1).parameters  # => [[:rest, :r], [:block, :b]]
method(:m2).parameters  # => [[:rest, :*], [:block, :&]]

# Yet one works and one fails
m1 a: 1 rescue $!  # => #<ArgumentError: wrong number of arguments (given 1, expected 0; required keyword: a)>
m2 a: 1 rescue $!  # => "success"

[Eregon (Benoit Daloze)]

jeremyevans0 (Jeremy Evans) wrote in #note-4:

I think methods that use ruby2_keywords, either explicitly or implicitly through argument forwarding, should handle parameters consistently.

For me ... and explicit ruby2_keywords should/can be two different things.
... need not be implemented by ruby2_keywords, and IMHO CRuby should not expose that implementation detail (which may change BTW, and is not necessarily accurate on other Ruby implementations).

However, I'm fine with all ruby2_keywords methods adding [:keyrest, :**] as opposed to adding [:ruby2_keywords]. The :keyrest approach may be more backwards compatible.

It is not fully ideal to pretend there is a keyrest paramater when there is not in the method definition, but I think this is a good compromise.
At least from the :** name we can find out it's added synthetically and is not from the source definition (method(def m(*,**); end).parameters => [[:rest], [:keyrest]]).
And technically ruby2_keywords accepts or forwards keyword arguments, so it makes sense.

Note that from a caller's perspective, [[:rest, :*]] and [[:rest, :*], [:keyrest, :**]] have identical behavior. Both accept an arbitrary number of positional argument and arbitrary keywords.

From [[:rest, :*]] one might expect any keyword args given by the caller are made positional in the callee, and that's not really the case with ruby2_keywords which keeps the information they were given as kwargs.

I think @jeremyevans0's solution of adding [:keyrest, :**] is the best here.
It also nicely matches an implementation which would convert (...) into (*, **, &) (the natural/intuitive form for that).

[josh.cheek (Josh Cheek)]

Y'all understand this way more than I do, can you comment on whether my analysis here seems correct?

[Dan0042 (Daniel DeLorme)]

This seems related to #16456

[matz (Yukihiro Matsumoto)]

Sounds reasonable. Accepted.

Matz.

[nobu (Nobuyoshi Nakada)]

Changing parameters broke rbs test.

https://github.com/nobu/ruby/runs/3076547487?check_suite_focus=true#step:15:350

Failure: test_argument_forwarding(RBS::RuntimePrototypeTest)
/home/runner/work/ruby/ruby/src/gems/src/rbs/test/test_helper.rb:194:in `assert_write'
/home/runner/work/ruby/ruby/src/gems/src/rbs/test/rbs/runtime_prototype_test.rb:231:in `block (2 levels) in test_argument_forwarding'
     228:         manager.build do |env|
     229:           p = Runtime.new(patterns: ["RBS::RuntimePrototypeTest::TestForArgumentForwarding"], env: env, merge: true)
     230: 
  => 231:           assert_write p.decls, <<-EOF
     232: class RBS::RuntimePrototypeTest::TestForArgumentForwarding
     233:   public
     234: 
/home/runner/work/ruby/ruby/src/gems/src/rbs/test/test_helper.rb:158:in `block in build'
/home/runner/work/ruby/ruby/src/lib/tmpdir.rb:96:in `mktmpdir'
/home/runner/work/ruby/ruby/src/gems/src/rbs/test/test_helper.rb:139:in `build'
/home/runner/work/ruby/ruby/src/gems/src/rbs/test/rbs/runtime_prototype_test.rb:228:in `block in test_argument_forwarding'
/home/runner/work/ruby/ruby/src/gems/src/rbs/test/test_helper.rb:73:in `new'
/home/runner/work/ruby/ruby/src/gems/src/rbs/test/rbs/runtime_prototype_test.rb:227:in `test_argument_forwarding'
<"class RBS::RuntimePrototypeTest::TestForArgumentForwarding\n" +
"  public\n" +
"\n" +
"  def foo: (*untyped) { (*untyped) -> untyped } -> untyped\n" +
"end\n"> expected but was
<"class RBS::RuntimePrototypeTest::TestForArgumentForwarding\n" +
"  public\n" +
"\n" +
"  def foo: (*untyped, **untyped) { (*untyped) -> untyped } -> untyped\n" +
"end\n">

diff:
  class RBS::RuntimePrototypeTest::TestForArgumentForwarding
    public
  
?   def foo:                         (*untyped) { (*untyped)  -> untyped } -> untyped
?            (*untyped, **untyped) {            ->          }                        
?           ++++++++++++++++++++++++            ????        ?           -------------
  end

Method#ruby2_keywords? may be needed, I guess.

[Eregon (Benoit Daloze)]

nobu (Nobuyoshi Nakada) wrote in #note-10:

Method#ruby2_keywords? may be needed, I guess.

It's possible to detect like method.parameters.include?([:keyrest, :**]).
I'm not sure how RBS represents delegation. The previous (*untyped) seems incorrect, while the actual result (*untyped, **untyped) seems better for Ruby 3+.

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset git|4c3140d60f6f94504842a4d0c0d79752a87aec8d.

---

Add keyrest to ruby2_keywords parameters [Bug #18011]

[Eregon (Benoit Daloze)]

• Related to Feature #16456: Ruby 2.7 argument delegation (...) should be its own kind of parameter in Method#parameters added