[ruby-core:108120] [Ruby master Bug#18625] ruby2_keywords does not unmark the hash if the receiving method has a *rest parameter

Issue #18625 has been updated by Eregon (Benoit Daloze).


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.

----------------------------------------
Bug #18625: ruby2_keywords does not unmark the hash if the receiving method has a *rest parameter
https://bugs.ruby-lang.org/issues/18625#change-97092

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
The code below shows the inconsistency.
In all cases the `marked` Hash is copied at call sites using `some_call(*args)`, however for the case of `splat` it keeps the ruby2_keywords flag to true, and not false as expected.
This can be observed in user code and will hurt migration from `ruby2_keywords` to other ways of delegation (`(...)` and `(*args, **kwargs)`).
I believe this is another manifestation of #16466.

```ruby
ruby2_keywords def foo(*args)
  args
end

def single(arg)
  arg
end

def splat(*args)
  args.last
end

def kwargs(**kw)
  kw
end

h = { a: 1 }
args = foo(**h)
marked = args.last
Hash.ruby2_keywords_hash?(marked) # => true

after_usage = single(*args)
after_usage == h # => true
after_usage.equal?(marked) # => false
p Hash.ruby2_keywords_hash?(after_usage) # => false

after_usage = splat(*args)
after_usage == h # => true
after_usage.equal?(marked) # => false
p Hash.ruby2_keywords_hash?(after_usage) # => true, BUG, should be false

after_usage = kwargs(*args)
after_usage == h # => true
after_usage.equal?(marked) # => false
p Hash.ruby2_keywords_hash?(after_usage) # => false

Hash.ruby2_keywords_hash?(marked) # => true
```

I'm implementing Ruby 3 kwargs in TruffleRuby and this came up as an inconsistency in specs.
In TruffleRuby it's also basically not possible to implement this behavior, because at a splat call site where we check for a last Hash argument marked as ruby2_keywords, we have no idea of which method will be called yet, and so cannot differentiate behavior based on that.

cc @jeremyevans0 @mame



-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>