Bug #18011
closed`Method#parameters` is incorrect for forwarded arguments
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) over 3 years 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) over 3 years ago
jeremyevans0 (Jeremy Evans) wrote in #note-1:
Forwarded arguments is implemented as
(*args, &block)
withruby2_keywords
, not as(*args, **kwargs, &block)
. If you adddef wrapper4(*r, &b) = wrapped(*r, &b),
tomethods
and thenruby2_keywords :wrapper4
aftermethods
, you can see it gives the same output aswrapper1
andwrapper3
.If we want to indicate which methods have
ruby2_keywords
, one way would be to includeruby2_keywords
intoMethod#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) over 3 years 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) over 3 years 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) over 3 years 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) over 3 years ago
jeremyevans0 (Jeremy Evans) wrote in #note-4:
I think methods that use
ruby2_keywords
, either explicitly or implicitly through argument forwarding, should handleparameters
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).
Updated by josh.cheek (Josh Cheek) over 3 years 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) over 3 years ago
This seems related to #16456
Updated by matz (Yukihiro Matsumoto) over 3 years ago
Sounds reasonable. Accepted.
Matz.
Updated by nobu (Nobuyoshi Nakada) over 3 years 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) over 3 years 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+.
Updated by nobu (Nobuyoshi Nakada) over 3 years ago
- Status changed from Open to Closed
Applied in changeset git|4c3140d60f6f94504842a4d0c0d79752a87aec8d.
Add keyrest to ruby2_keywords parameters [Bug #18011]
Updated by Eregon (Benoit Daloze) almost 3 years ago
- Related to Feature #16456: Ruby 2.7 argument delegation (...) should be its own kind of parameter in Method#parameters added