Bug #20640: Evaluation Order Issue in f(**h, &h.delete(key))

• Status: Closed
• Assignee: jeremyevans0 (Jeremy Evans)
• Target version: -
• Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN

[jeremyevans0 (Jeremy Evans)]

Since Ruby 3.0, there is an evaluation order issue when passing a single keyword splat and a block pass expression that modifies the keyword splat:

def f(*a, **kw) kw[:a].class end
h = {a: ->{}}
f(**h, &h.delete(:a))
# Ruby 2.0 - 2.7: Proc
# Ruby 3.0 - 3.4: NilClass

For single keyword splats followed by positional argument splats, this has been an issue since 3.3:

def f(*a, **kw) kw[:a].class end
h = {a: ->{}}
a = []
f(*a, **h, &h.delete(:a))
# Ruby 2.0 - 3.2: Proc
# Ruby 3.3 - 3.4: NilClass

Ruby handles these issues for positional splats, duplicating the splatted array before evaluating post, keyword, or block argument expressions:

f(*a, a.pop)    # post argument
f(*a, **a.pop)  # keyword splat argument
f(*a, a: a.pop) # keyword argument
f(*a, &a.pop)   # block argument

So it should handle the case for a keyword splat that could potentially be modified by block argument expression.

I'll submit a pull request shortly to fix this issue.

[jeremyevans0 (Jeremy Evans)]

Pull request: https://github.com/ruby/ruby/pull/11206

[matz (Yukihiro Matsumoto)]

• Assignee set to jeremyevans0 (Jeremy Evans)

The behavior of modifying the splatting object in to_proc has been flaky in the history of the language. I think the current behavior is a bit inconsistent and should be fixed (only if there's no performance penalty).

Matz.

[mame (Yusuke Endoh)]

Briefly discussed at the dev meeting, and we found more pedantic case. We would like to see how much of a performance penalty it would bring to see if it should be fixed.

def f(*a, **kw) kw[:a].class end

h = {a: 1}
foo = Object.new
foo.define_singleton_method(:to_proc) do
  h.clear
  proc {}
end
p f(**h, &foo) #=> expected: Integer, actual: NilClass

[Eregon (Benoit Daloze)]

IMO this kind of issue is not worth fixing, creating extra copies for this seems clearly not worth it (in perf hit vs very minimal gain).
It is a bug of the user code to mutate arguments while passing them, the user code should be fixed to not do that.
IOW the examples above make no sense and are unclear at best, relying on any specific semantics there is extremely brittle, and there is no value to do this in the first place anyway.

[jeremyevans0 (Jeremy Evans)]

mame (Yusuke Endoh) wrote in #note-3:

Briefly discussed at the dev meeting, and we found more pedantic case. We would like to see how much of a performance penalty it would bring to see if it should be fixed.

def f(*a, **kw) kw[:a].class end

h = {a: 1}
foo = Object.new
foo.define_singleton_method(:to_proc) do
  h.clear
  proc {}
end
p f(**h, &foo) #=> expected: Integer, actual: NilClass

There would likely be a significant performance penalty. Ever method call with splat and keyword splat, splat and block, or keyword splat and block, would need at least VM instructions added to check the types of the arguments, or all such calls would need to allocate when they currently do not.

My understanding was that we accept that implicit conversion methods that modify other objects can cause problems, we only try to protect against expressions in the method call causing problems.

• Status changed from Open to Closed

Applied in changeset git|07d3bf4832532ae7446c9a6924d79aed60a7a9a5.

---

Fix evaluation order issue in f(**h, &h.delete(key))

Previously, this would delete the key in h before keyword
splatting h. This goes against how ruby handles f(*a, &a.pop)
and similar expressions.

Fix this by having the compiler check whether the block pass
expression is safe. If it is not safe, then dup the keyword
splatted hash before evaluating the block pass expression.

For expression: h=nil; f(**h, &h.delete(:key))

VM instructions before:

0000 putnil                                                           (   1)[Li]
0001 setlocal_WC_0                          h@0
0003 putself
0004 getlocal_WC_0                          h@0
0006 getlocal_WC_0                          h@0
0008 putobject                              :key
0010 opt_send_without_block                 <calldata!mid:delete, argc:1, ARGS_SIMPLE>
0012 splatkw
0013 send                                   <calldata!mid:f, argc:1, ARGS_BLOCKARG|FCALL|KW_SPLAT>, nil
0016 leave

VM instructions after:

0000 putnil                                                           (   1)[Li]
0001 setlocal_WC_0                          h@0
0003 putself
0004 putspecialobject                       1
0006 newhash                                0
0008 getlocal_WC_0                          h@0
0010 opt_send_without_block                 <calldata!mid:core#hash_merge_kwd, argc:2, ARGS_SIMPLE>
0012 getlocal_WC_0                          h@0
0014 putobject                              :key
0016 opt_send_without_block                 <calldata!mid:delete, argc:1, ARGS_SIMPLE>
0018 send                                   <calldata!mid:f, argc:1, ARGS_BLOCKARG|FCALL|KW_SPLAT|KW_SPLAT_MUT>, nil
0021 leave

Fixes [Bug #20640]