From: "Eregon (Benoit Daloze)" <noreply@...>
Date: 2022-03-14T17:23:01+00:00
Subject: [ruby-core:107898] [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).


Thank you for the PR, I think we should merge it.

Fixing this is important for multiple reasons:
* Can cause issues when migrating to other forms of delegation
* Makes the `ruby2_keywords` semantics confusing/inconsistent/more complex
* May force other Ruby implementations to replicate this bug if not fixed

First 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:
```ruby
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):
```ruby
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:
* The ruby2_keywords flag for a Hash instance never changes, the only way to "change" it is to create a new Hash instance with another value for the flag (already the case, great).
* When the ruby2_keywords flag of a Hash is used at a `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).

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

* 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:ruby-core-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>