Bug #20008
closed
f(**kw, &block) calls block.to_proc before kw.to_hash
Description
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:
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
Updated by jeremyevans0 (Jeremy Evans) about 1 year ago
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.
Updated by jeremyevans0 (Jeremy Evans) about 1 year ago
- Related to Bug #20051: Op asgn calls handle keywords and keyword splats as positional arguments added
Updated by jeremyevans0 (Jeremy Evans) about 1 year ago
- Status changed from Open to Closed