Issue #20008 has been updated by jeremyevans0 (Jeremy Evans). This was reviewed during yesterday's dev meeting and was found to be mostly OK, except for the use of the peephole optimizer to fix the issue. I moved the fix from the peephole optimizer to the compiler, which should make this acceptable. I found the same issue affected `super` calls with explicit arguments, so I fixed those as well. This does not affect `yield`, because you cannot pass a block to `yield`. It does not affect zsuper calls, as zsuper does not currently respect reassigning the block parameter. zsuper always uses the block passed to the original method, and does not call `to_proc` on it. This issue affects op asgn calls (e.g. `foo[**kw, &block] = bar`, but those are compiled incorrectly anyway, passing `**kw` as a `kw` positional argument (no `to_hash` conversion) to the `[]` and `[]=` methods, allowing code such as `[1][**0] += 2` to work. I'll file a separate bug report for that. ---------------------------------------- Bug #20008: f(**kw, &block) calls block.to_proc before kw.to_hash https://bugs.ruby-lang.org/issues/20008#change-105588 * Author: jeremyevans0 (Jeremy Evans) * Status: Open * 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/