Skip to content

Commit

Permalink
Fix keyword splat passing as regular argument
Browse files Browse the repository at this point in the history
Since Ruby 3.0, Ruby has passed a keyword splat as a regular
argument in the case of a call to a Ruby method where the
method does not accept keyword arguments, if the method
call does not contain an argument splat:

```ruby
def self.f(obj) obj end
def self.fs(*obj) obj[0] end
h = {a: 1}
f(**h).equal?(h)  # Before: true; After: false
fs(**h).equal?(h) # Before: true; After: false

a = []
f(*a, **h).equal?(h)  # Before and After: false
fs(*a, **h).equal?(h) # Before and After: false
```

The fact that the behavior differs when passing an empty
argument splat makes it obvious that something is not
working the way it is intended.  Ruby 2 always copied
the keyword splat hash, and that is the expected behavior
in Ruby 3.

This bug is because of a missed check in setup_parameters_complex.
If the keyword splat passed is not mutable, then it points to
an existing object and not a new object, and therefore it must
be copied.

Now, there are 3 specs for the broken behavior of directly
using the keyword splatted hash.  Fix two specs and add a
new version guard. Do not keep the specs for the broken
behavior for earlier Ruby versions, in case this fix is
backported. For the ruby2_keywords spec, just remove the
related line, since that line is unrelated to what the
spec is testing.

Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>
  • Loading branch information
jeremyevans and nobu committed Dec 7, 2023
1 parent e6a6ea9 commit ca204a2
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 21 deletions.
1 change: 0 additions & 1 deletion spec/ruby/core/module/ruby2_keywords_spec.rb
Expand Up @@ -22,7 +22,6 @@ def regular(*args)
end

h = {a: 1}
obj.regular(**h).should.equal?(h)

last = mark(**h).last
Hash.ruby2_keywords_hash?(last).should == true
Expand Down
18 changes: 10 additions & 8 deletions spec/ruby/language/hash_spec.rb
Expand Up @@ -220,15 +220,17 @@ def m(**h)
h.should == { one: 1, two: 2 }
end

it "does not copy when calling a method taking a positional Hash" do
def m(h)
h.delete(:one); h
end
ruby_bug "#20012", ""..."3.3" do
it "makes a copy when calling a method taking a positional Hash" do
def m(h)
h.delete(:one); h
end

h = { one: 1, two: 2 }
m(**h).should == { two: 2 }
m(**h).should.equal?(h)
h.should == { two: 2 }
h = { one: 1, two: 2 }
m(**h).should == { two: 2 }
m(**h).should_not.equal?(h)
h.should == { one: 1, two: 2 }
end
end

ruby_version_is "3.1" do
Expand Down
15 changes: 9 additions & 6 deletions spec/ruby/language/keyword_arguments_spec.rb
Expand Up @@ -87,13 +87,16 @@ def m(*a)
end

context "**" do
it "does not copy a non-empty Hash for a method taking (*args)" do
def m(*args)
args[0]
end
ruby_version_is "3.3" do
it "copies a non-empty Hash for a method taking (*args)" do
def m(*args)
args[0]
end

h = {a: 1}
m(**h).should.equal?(h)
h = {a: 1}
m(**h).should_not.equal?(h)
h.should == {a: 1}
end
end

it "copies the given Hash for a method taking (**kwargs)" do
Expand Down
26 changes: 20 additions & 6 deletions test/ruby/test_keyword.rb
Expand Up @@ -237,16 +237,16 @@ def kwargs(**kw)
assert_equal(true, Hash.ruby2_keywords_hash?(marked))
end

def assert_equal_not_same(kw, res)
assert_instance_of(Hash, res)
assert_equal(kw, res)
assert_not_same(kw, res)
end

def test_keyword_splat_new
kw = {}
h = {a: 1}

def self.assert_equal_not_same(kw, res)
assert_instance_of(Hash, res)
assert_equal(kw, res)
assert_not_same(kw, res)
end

def self.yo(**kw) kw end
m = method(:yo)
assert_equal(false, yo(**{}).frozen?)
Expand Down Expand Up @@ -459,6 +459,20 @@ def self.yo(opts) = opts
assert_equal_not_same h, yo(*a, **h)
end

def test_keyword_splat_to_non_keyword_method
h = {a: 1}.freeze

def self.yo(kw) kw end
assert_equal_not_same(h, yo(**h))
assert_equal_not_same(h, method(:yo).(**h))
assert_equal_not_same(h, :yo.to_proc.(self, **h))

def self.yoa(*kw) kw[0] end
assert_equal_not_same(h, yoa(**h))
assert_equal_not_same(h, method(:yoa).(**h))
assert_equal_not_same(h, :yoa.to_proc.(self, **h))
end

def test_regular_kwsplat
kw = {}
h = {:a=>1}
Expand Down
4 changes: 4 additions & 0 deletions vm_args.c
Expand Up @@ -609,6 +609,10 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
kw_flag &= ~(VM_CALL_KW_SPLAT | VM_CALL_KW_SPLAT_MUT);
}
else {
if (!(kw_flag & VM_CALL_KW_SPLAT_MUT)) {
converted_keyword_hash = rb_hash_dup(converted_keyword_hash);
}

if (last_arg != converted_keyword_hash) {
last_arg = converted_keyword_hash;
args->argv[args->argc-1] = last_arg;
Expand Down

0 comments on commit ca204a2

Please sign in to comment.