[ruby-core:115796] [Ruby master Bug#20008] f(**kw, &block) calls block.to_proc before kw.to_hash

Issue #20008 has been updated by jeremyevans0 (Jeremy Evans).

Status changed from Open to Closed

Fixed by commit:a950f230788d51e13d16596e37cb77e4cc6e2311 and commit:f64357540eabad0f1bfaa6be60710d153325b064

----------------------------------------
Bug #20008: f(**kw, &block) calls block.to_proc before kw.to_hash 
https://bugs.ruby-lang.org/issues/20008#change-105740

* Author: jeremyevans0 (Jeremy Evans)
* Status: Closed
* Priority: Normal
* ruby -v: ruby 3.3.0dev (2023-11-12 master 60e19a0b5f) [x86_64-openbsd]
* Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN
----------------------------------------
The evaluation order expectation is that `kw.to_hash` is called first in this case.  Ruby previously called `kw.to_hash` before `block.to_proc` in Ruby 2, but stopped doing so in Ruby 3.0. In master, `f(*a, **kw, &block)` is also affected.  However, that is not true in Ruby 3.2 and earlier (though Ruby 2.0 and 2.1 called `kw.to_hash` before `a.to_a`).

The reason for the current behavior is that `vm_caller_setup_arg_block` calls `block.to_proc` before `vm_sendish` is called. `kw.to_hash` is not called until `CALLER_SETUP_ARG` or `setup_parameters_complex`.

I have a pull request that fixes this (https://github.com/ruby/ruby/pull/8877), by adding a `splatkw` VM instruction and calling it directly before any send instructions that have both `VM_CALL_ARGS_BLOCKARG` and `VM_CALL_KW_SPLAT` and not `VM_CALL_KW_SPLAT_MUT`. The `splatkw` instruction calls `kw.to_hash` if `kw` is not already a Hash.  The `splatkw` instruction is not needed for mutable keyword splats, as `kw.to_hash` would have already been called by earlier instructions in that case.  

It is possible to fix this without a new VM instruction, by adding the logic to `vm_caller_setup_arg_block` or `rb_vm_send`.  However, then an additional check is needed in every method call, instead of only before method calls that actually need it.  I would guess that approach would be slower, though as I have not benchmarked that approach, it is not an educated guess.

Currently, the `splatkw` instruction is inserted using the peephole optimizer.  I'm not sure if using the optimizer to fix bugs is acceptable.  If not, it could probably be moved to the compiler proper.

I do not believe we should backport this fix, as I consider a new VM instruction invasive, and the benefit of backporting seems small.

Example code:

```ruby
o = Object.new
o.define_singleton_method(:to_hash) {p :called_to_hash; {}}
o.define_singleton_method(:to_proc) {p :called_to_proc; lambda{}}

puts "p(**o, &o)"
p(**o, &o)
```

Actual Output:

```
p(**o, &o)
:called_to_proc
:called_to_hash
```

Expected Output:

```
p(**o, &o)
:called_to_hash
:called_to_proc
```



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- [e-mail]
 To unsubscribe send an email to [e-mail]
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/