Project

General

Profile

Actions

Bug #18011

closed

`Method#parameters` is incorrect for forwarded arguments

Added by josh.cheek (Josh Cheek) 4 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [arm64-darwin20]
[ruby-core:104430]

Description

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}]

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

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.

Updated by josh.cheek (Josh Cheek) 4 months ago

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]]

Updated by Eregon (Benoit Daloze) 4 months ago

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.

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

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.

Updated by josh.cheek (Josh Cheek) 4 months ago

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"

Updated by Eregon (Benoit Daloze) 4 months ago

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 (Jeremy Evans)'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).

Updated by josh.cheek (Josh Cheek) 4 months ago

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

Updated by Dan0042 (Daniel DeLorme) 4 months ago

This seems related to #16456

Updated by matz (Yukihiro Matsumoto) 3 months ago

Sounds reasonable. Accepted.

Matz.

Updated by nobu (Nobuyoshi Nakada) 3 months ago

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.

Updated by Eregon (Benoit Daloze) 3 months ago

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

Actions #12

Updated by nobu (Nobuyoshi Nakada) 3 months ago

  • Status changed from Open to Closed

Applied in changeset git|4c3140d60f6f94504842a4d0c0d79752a87aec8d.


Add keyrest to ruby2_keywords parameters [Bug #18011]

Actions

Also available in: Atom PDF