Based on @matz's comments (https://bugs.ruby-lang.org/issues/16466#note-3), I do not think this is a bug. Splatted arguments where the final argument is a flagged hash will have the hash converted to keywords if the method accepts keywords. If the method does not accept keywords, I don't think the behavior is specified.
I think this change is more consistent than the current code. I cannot think of a justification why single(*args) would return an unflagged copy, but splat(*args) would return the flagged hash. I think there are valid arguments for both returning flagged hash, with the argument that flagged hashes only affect methods that accept keywords, and other methods they are passed straight through. I also think this change returning an unflagged copy is justifiable, with the argument that foo(*args) where the final element of args is a flagged hash should always be treated as foo(*args[0...-1], **args[-1]), regardless of the definition of foo.
I recommend that we accept this change for 3.2. I'm not sure we should backport this to 3.0 or 3.1, since it could break existing code (unlikely, but possible).
In case this change is considered desirable (either as a bug fix or as a deliberate feature change), I have submitted a pull request for it: https://github.com/ruby/ruby/pull/5645 . The pull request does not require any additional allocations.
Thank you for the PR, I think we should merge it.
Fixing this is important for multiple reasons:
ruby2_keywords semantics confusing/inconsistent/more complexFirst of all, this can avoid bad surprises when switching from ruby2_keyword to (*args, **kwargs) or (...) (which makes sense e.g. when a gem no longer needs to support Ruby 2):
A simple example:
ruby2_keywords def foo(*args)
bar(*args)
end
def bar(*args)
baz(*args)
end
def baz(a:)
a
end
p foo(a: 1) # => 1
This works on current master, but it should not, there is a missing ruby2_keywords on baz. The fact it does not fail may also confuse Ruby users (they might think the flag is kept across calls).
And if I translate this example to use (*args, **kwargs) instead of ruby2_keywords, it will be broken (on all versions):
def foo(*args, **kwargs)
bar(*args, **kwargs)
end
def bar(*args)
baz(*args)
end
def baz(a:)
a
end
p foo(a: 1) # => in `baz': wrong number of arguments (given 1, expected 0; required keyword: a) (ArgumentError)
Second, as mentioned above this is an inconsistency and if any user observes this they will likely be very confused for good reasons.
The semantics of ruby2_keywords should stay as simple as possible, because it's already quite complex.
Notably:
foo(*args) call site, the callee receives a Hash without the flag set. This is necessary as keeping the flag would break code by continuing to treat the Hash as kwargs when it should not, and to provide an easy migration to other ways of delegation. This guarantee currently holds for all cases, except for the case reported in this issue.And third since this behavior is observable to users, it might force other Ruby implementations to replicate this bug, which would be very unfortunate.
This behavior does not make sense semantically and can be tricky to implement.
The property that a call site does not need to know about a callee is a valuable one (both conceptually and in the implementation as it has performance implications), and this bug breaks that (the arguments are treated differently based on a specific callee).
From the dev meeting log:
Conclusion:
matz: I think it is good to fix
@mame (Yusuke Endoh): It may break (a lot of?) existing programs, but hopefully easy to fix. I’ll discuss
As far as I understand, this change will break the following code:
def target(a:)
p a
end
# after the patch, ruby2_keywords is requried here
def bar(*args)
target(*args)
end
ruby2_keywords def foo(*args)
bar(*args)
end
p foo(a: 42) #=> 42 in Ruby 3.1
#=> ArgumentError after the patch
After the patch, we need to add ruby2_keywords to the definition of bar.
According to our previous experience, rails is using this kind of multi-stage delegation pattern so much. Maybe we need to ask the core developers of rails to check and support the change. cc/ @kamipo
Yes, that's correct, and that code would also break when migrating to other form of delegations (if not changing bar too).
I don't understand why we had these semantics in the first place, I think there are hard to understand for anyone (it seems it was unintentional).
They might lead people to think the ruby2_keywords flag is kept forever, but that is not the case.
The rule is the ruby2_keywords flag is "reset" (i.e., the Hash in the callee is not flagged) once used, e.g., on foo(*flagged_args).
This always holds, except for the case where the callee has a rest arg (this issue).
@mame (Yusuke Endoh) noticed as well and I said it back just after the 2.7 release that we should have fixed this (it would probably have been easier), but it seems I was not heard: #16466.
I believe this is worth fixing, for simpler semantics and for being able to migrate to other forms of delegations without major troubles.
It is easier to add some ruby2_keywords now than while migrating to (*args, **kwargs)/(...) figure out several ruby2_keywords were missing and have to review all places which do delegation.
It is likely some gems which do lots of delegation might miss a few ruby2_keywords, like rspec-mocks does.
Adding missing ruby2_keywords is not particularly hard, adding puts caller at the end is enough to find all of them along a delegation chain.
FWIW, I found that Rails is currently missing some ruby2_keywords due to this bug.
For example when running rspec-rails specs (cd rspec-rails && bundle && bundle exec rspec spec/rspec/rails/configuration_spec.rb), I counted about 7 missing ruby2_keywords for places which delegate with *args:
./spec/rspec/rails/configuration_spec.rb:273:in `welcome'
gems/actionpack-6.0.4.7/lib/abstract_controller/base.rb:195:in `process_action'
gems/actionpack-6.0.4.7/lib/abstract_controller/callbacks.rb:42:in `block in process_action'
gems/activesupport-6.0.4.7/lib/active_support/callbacks.rb:101:in `run_callbacks'
gems/actionpack-6.0.4.7/lib/abstract_controller/callbacks.rb:41:in `process_action'
gems/actionpack-6.0.4.7/lib/abstract_controller/base.rb:136:in `process'
gems/actionmailer-6.0.4.7/lib/action_mailer/rescuable.rb:25:in `block in process'
gems/actionmailer-6.0.4.7/lib/action_mailer/rescuable.rb:17:in `handle_exceptions'
gems/actionmailer-6.0.4.7/lib/action_mailer/rescuable.rb:24:in `process'
gems/actionview-6.0.4.7/lib/action_view/rendering.rb:39:in `process'
gems/actionmailer-6.0.4.7/lib/action_mailer/base.rb:637:in `block in process'
gems/activesupport-6.0.4.7/lib/active_support/notifications.rb:180:in `block in instrument'
gems/activesupport-6.0.4.7/lib/active_support/notifications/instrumenter.rb:24:in `instrument'
gems/activesupport-6.0.4.7/lib/active_support/notifications.rb:180:in `instrument'
gems/actionmailer-6.0.4.7/lib/action_mailer/base.rb:636:in `process'
gems/actionmailer-6.0.4.7/lib/action_mailer/message_delivery.rb:124:in `block in processed_mailer'
I think this means other Ruby implementations will be forced to replicate this bug (and its performance and significant complexity implications) as long as they target Ruby 3.0/3.1 compatibility, as multiple gems will rely on it without knowing it, which is very unfortunate, and it will take some time until such gems have the fix merged and have a release with it.
I still do believe it is worth fixing this for 3.2 and fix the gems, that will make it easier for gems when they can assume Ruby 3+ and migrate to (*args, **kwargs)/(...).
I also cannot hide the fact that I am very disappointed that #16466 was not fixed back then, before the Ruby 3 release. It seemed a straightforward bug with bad consequences if unfixed (and indeed it turns out to be the case), and it was reported days after the 2.7.0 release, so plenty of time before 3.0 or even 2.7.1. I'm afraid matz did not understand the situation in https://bugs.ruby-lang.org/issues/16466#note-3, in fact the fix does help to find the correct places for ruby2_keywords. But we cannot return in the past to change things, so let's focus on now and the future.
I also noticed another inconsistency on 3.0.3 and current master: -> *args { args.last }.call(*args) does not copy a marked Hash, but send(:foo, *args) (foo taking *args) does copy it (even though both are implemented in C and takea rest parameter).
@jeremyevans0's PR fixes that too, by always copying and unmarking, as expected.
I merged https://github.com/ruby/ruby/pull/5684, which is Jeremy's PR + a needed update of rspec-mocks + a NEWS entry.
This is the only way to really assess the impact on gems/apps depending on this bug, and to let gems/apps to find and add the missing ruby2_keywords (asking gems or even rails to use a branch of ruby is a lot of overhead).
Because this was a bug, and there is no other way to fix it, and this fix will help migration to other forms of delegation, I think it is very unlikely we would revert this change, but it remains a possibility until the 3.2 release.
A good technique to find the potentially-missing ruby2_keywords is to run the test suite, for where it fails find the last method which must receive keyword arguments,
use puts nil, caller, nil there, and check each method on the call chain which must delegate keywords is correctly marked as ruby2_keywords.
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.3