From 7ba583d753411a39dec78ea6e7c8d2701b04c118 Mon Sep 17 00:00:00 2001 From: Takashi Kokubun Date: Fri, 3 Apr 2026 20:20:44 -0700 Subject: merge revision(s) c27ae8d91aadca0660070ee1eeae9598b1fe47ee, b6e6ccc6c22ebb464ce101e42a14e7daea2a80b6, cbca59377ebfa1bc1183322fa9d9d0067d445c24: [Backport #21844] [ruby/psych] Remove excessive check of message Make `Data#initialize` reject `Integer` keys [Bug #21844] Fix error message --- array.c | 2 +- internal/array.h | 1 + spec/ruby/core/data/deconstruct_keys_spec.rb | 86 +++++++++------------- struct.c | 103 +++++++++++++++++++-------- test/psych/test_data.rb | 5 +- test/ruby/test_data.rb | 26 +++++-- 6 files changed, 133 insertions(+), 90 deletions(-) diff --git a/array.c b/array.c index e13239ad3d..3fbf4991e7 100644 --- a/array.c +++ b/array.c @@ -6528,7 +6528,7 @@ rb_ary_uniq(VALUE ary) * see also {Methods for Deleting}[rdoc-ref:Array@Methods+for+Deleting]. */ -static VALUE +VALUE rb_ary_compact_bang(VALUE ary) { VALUE *p, *t, *end; diff --git a/internal/array.h b/internal/array.h index 3a689646fb..a2cd06f2e3 100644 --- a/internal/array.h +++ b/internal/array.h @@ -37,6 +37,7 @@ size_t rb_ary_size_as_embedded(VALUE ary); void rb_ary_make_embedded(VALUE ary); bool rb_ary_embeddable_p(VALUE ary); VALUE rb_ary_diff(VALUE ary1, VALUE ary2); +VALUE rb_ary_compact_bang(VALUE ary); RUBY_EXTERN VALUE rb_cArray_empty_frozen; static inline VALUE rb_ary_entry_internal(VALUE ary, long offset); diff --git a/spec/ruby/core/data/deconstruct_keys_spec.rb b/spec/ruby/core/data/deconstruct_keys_spec.rb index df378f8b98..5914fcf6b7 100644 --- a/spec/ruby/core/data/deconstruct_keys_spec.rb +++ b/spec/ruby/core/data/deconstruct_keys_spec.rb @@ -34,29 +34,6 @@ describe "Data#deconstruct_keys" do d.deconstruct_keys(['x', 'y']).should == {'x' => 1, 'y' => 2} end - it "accepts argument position number as well but returns them as keys" do - klass = Data.define(:x, :y) - d = klass.new(1, 2) - - d.deconstruct_keys([0, 1]).should == {0 => 1, 1 => 2} - d.deconstruct_keys([0] ).should == {0 => 1} - d.deconstruct_keys([-1] ).should == {-1 => 2} - end - - it "ignores incorrect position numbers" do - klass = Data.define(:x, :y) - d = klass.new(1, 2) - - d.deconstruct_keys([0, 3]).should == {0 => 1} - end - - it "support mixing attribute names and argument position numbers" do - klass = Data.define(:x, :y) - d = klass.new(1, 2) - - d.deconstruct_keys([0, :x]).should == {0 => 1, :x => 1} - end - it "returns an empty hash when there are more keys than attributes" do klass = Data.define(:x, :y) d = klass.new(1, 2) @@ -72,14 +49,6 @@ describe "Data#deconstruct_keys" do d.deconstruct_keys([:x, :a]).should == {x: 1} end - it "returns at first not existing argument position number" do - klass = Data.define(:x, :y) - d = klass.new(1, 2) - - d.deconstruct_keys([3, 0]).should == {} - d.deconstruct_keys([0, 3]).should == {0 => 1} - end - it "accepts nil argument and return all the attributes" do klass = Data.define(:x, :y) d = klass.new(1, 2) @@ -87,35 +56,46 @@ describe "Data#deconstruct_keys" do d.deconstruct_keys(nil).should == {x: 1, y: 2} end - it "tries to convert a key with #to_int if index is not a String nor a Symbol, but responds to #to_int" do - klass = Data.define(:x, :y) - d = klass.new(1, 2) + ruby_bug "Bug #21844", ""..."4.1" do + it "tries to convert a key with #to_str if index is not a String nor a Symbol, but responds to #to_str" do + klass = Data.define(:x, :y) + d = klass.new(1, 2) - key = mock("to_int") - key.should_receive(:to_int).and_return(1) + key = mock("to_str") + key.should_receive(:to_str).and_return("y") - d.deconstruct_keys([key]).should == { key => 2 } - end + d.deconstruct_keys([key]).should == { "y" => 2 } + end - it "raises a TypeError if the conversion with #to_int does not return an Integer" do - klass = Data.define(:x, :y) - d = klass.new(1, 2) + it "raise an error on argument position number" do + klass = Data.define(:x, :y) + d = klass.new(1, 2) - key = mock("to_int") - key.should_receive(:to_int).and_return("not an Integer") + -> { + d.deconstruct_keys([0, 1]) + }.should raise_error(TypeError, "0 is not a symbol nor a string") + end - -> { - d.deconstruct_keys([key]) - }.should raise_error(TypeError, /can't convert MockObject to Integer/) - end + it "raises a TypeError if the conversion with #to_str does not return a String" do + klass = Data.define(:x, :y) + d = klass.new(1, 2) - it "raises TypeError if index is not a String, a Symbol and not convertible to Integer " do - klass = Data.define(:x, :y) - d = klass.new(1, 2) + key = mock("to_str") + key.should_receive(:to_str).and_return(0) - -> { - d.deconstruct_keys([0, []]) - }.should raise_error(TypeError, "no implicit conversion of Array into Integer") + -> { + d.deconstruct_keys([key]) + }.should raise_consistent_error(TypeError, /can't convert MockObject into String/) + end + + it "raises TypeError if index is not a Symbol and not convertible to String " do + klass = Data.define(:x, :y) + d = klass.new(1, 2) + + -> { + d.deconstruct_keys([0, []]) + }.should raise_error(TypeError, "0 is not a symbol nor a string") + end end it "raise TypeError if passed anything except nil or array" do diff --git a/struct.c b/struct.c index 31df3798cb..a32861b523 100644 --- a/struct.c +++ b/struct.c @@ -716,15 +716,17 @@ num_members(VALUE klass) struct struct_hash_set_arg { VALUE self; VALUE unknown_keywords; + VALUE missing_keywords; + long missing_count; }; -static int rb_struct_pos(VALUE s, VALUE *name); +static int rb_struct_pos(VALUE s, VALUE *name, bool name_only); +static VALUE deconstruct_keys(VALUE s, VALUE keys, bool name_only); static int -struct_hash_set_i(VALUE key, VALUE val, VALUE arg) +struct_hash_aset(VALUE key, VALUE val, struct struct_hash_set_arg *args, bool name_only) { - struct struct_hash_set_arg *args = (struct struct_hash_set_arg *)arg; - int i = rb_struct_pos(args->self, &key); + int i = rb_struct_pos(args->self, &key, name_only); if (i < 0) { if (NIL_P(args->unknown_keywords)) { args->unknown_keywords = rb_ary_new(); @@ -735,6 +737,14 @@ struct_hash_set_i(VALUE key, VALUE val, VALUE arg) rb_struct_modify(args->self); RSTRUCT_SET(args->self, i, val); } + return i; +} + +static int +struct_hash_set_i(VALUE key, VALUE val, VALUE arg) +{ + struct struct_hash_set_arg *args = (struct struct_hash_set_arg *)arg; + struct_hash_aset(key, val, args, false); return ST_CONTINUE; } @@ -767,12 +777,13 @@ rb_struct_initialize_m(int argc, const VALUE *argv, VALUE self) break; } if (keyword_init) { - struct struct_hash_set_arg arg; + struct struct_hash_set_arg arg = { + .self = self, + .unknown_keywords = Qnil, + }; rb_mem_clear((VALUE *)RSTRUCT_CONST_PTR(self), n); - arg.self = self; - arg.unknown_keywords = Qnil; rb_hash_foreach(argv[0], struct_hash_set_i, (VALUE)&arg); - if (arg.unknown_keywords != Qnil) { + if (UNLIKELY(!NIL_P(arg.unknown_keywords))) { rb_raise(rb_eArgError, "unknown keywords: %s", RSTRING_PTR(rb_ary_join(arg.unknown_keywords, rb_str_new2(", ")))); } @@ -1113,6 +1124,12 @@ rb_struct_to_h(VALUE s) */ static VALUE rb_struct_deconstruct_keys(VALUE s, VALUE keys) +{ + return deconstruct_keys(s, keys, false); +} + +static VALUE +deconstruct_keys(VALUE s, VALUE keys, bool name_only) { VALUE h; long i; @@ -1132,7 +1149,7 @@ rb_struct_deconstruct_keys(VALUE s, VALUE keys) h = rb_hash_new_with_size(RARRAY_LEN(keys)); for (i=0; i= 0 && args->missing_count > 0) { + VALUE k = RARRAY_AREF(args->missing_keywords, i); + if (!NIL_P(k)) { + RARRAY_ASET(args->missing_keywords, i, Qnil); + args->missing_count--; + } + } + return ST_CONTINUE; +} + /* * call-seq: * DataClass::members -> array_of_symbols @@ -1816,22 +1848,31 @@ rb_data_initialize_m(int argc, const VALUE *argv, VALUE self) rb_error_arity(argc, 0, 0); } - if (RHASH_SIZE(argv[0]) < num_members) { - VALUE missing = rb_ary_diff(members, rb_hash_keys(argv[0])); - rb_exc_raise(rb_keyword_error_new("missing", missing)); - } - - struct struct_hash_set_arg arg; + VALUE missing = rb_ary_dup(members); + RBASIC_CLEAR_CLASS(missing); + struct struct_hash_set_arg arg = { + .self = self, + .unknown_keywords = Qnil, + .missing_keywords = missing, + .missing_count = (long)num_members, + }; rb_mem_clear((VALUE *)RSTRUCT_CONST_PTR(self), num_members); - arg.self = self; - arg.unknown_keywords = Qnil; - rb_hash_foreach(argv[0], struct_hash_set_i, (VALUE)&arg); + rb_hash_foreach(argv[0], data_hash_set_i, (VALUE)&arg); // Freeze early before potentially raising, so that we don't leave an // unfrozen copy on the heap, which could get exposed via ObjectSpace. OBJ_FREEZE(self); - if (arg.unknown_keywords != Qnil) { - rb_exc_raise(rb_keyword_error_new("unknown", arg.unknown_keywords)); + if (UNLIKELY(arg.missing_count > 0)) { + rb_ary_compact_bang(missing); + RUBY_ASSERT(RARRAY_LEN(missing) == arg.missing_count, "missing_count=%ld but %ld", arg.missing_count, RARRAY_LEN(missing)); + RBASIC_SET_CLASS_RAW(missing, rb_cArray); + rb_exc_raise(rb_keyword_error_new("missing", missing)); } + VALUE unknown_keywords = arg.unknown_keywords; + if (UNLIKELY(!NIL_P(unknown_keywords))) { + RBASIC_SET_CLASS_RAW(unknown_keywords, rb_cArray); + rb_exc_raise(rb_keyword_error_new("unknown", unknown_keywords)); + } + return Qnil; } @@ -2086,7 +2127,11 @@ rb_data_inspect(VALUE s) * end */ -#define rb_data_deconstruct_keys rb_struct_deconstruct_keys +static VALUE +rb_data_deconstruct_keys(VALUE s, VALUE keys) +{ + return deconstruct_keys(s, keys, true); +} /* * Document-class: Struct diff --git a/test/psych/test_data.rb b/test/psych/test_data.rb index 57c3478193..5e340c580a 100644 --- a/test/psych/test_data.rb +++ b/test/psych/test_data.rb @@ -83,12 +83,11 @@ module Psych # completely different members TestData.send :remove_const, :D - TestData.const_set :D, Data.define(:foo, :bar) + TestData.const_set :D, Data.define(:a, :c) e = assert_raise(ArgumentError) { Psych.unsafe_load d } - assert_equal 'unknown keywords: :a, :b', e.message + assert_include e.message, 'keyword:' ensure TestData.send :remove_const, :D end end end - diff --git a/test/ruby/test_data.rb b/test/ruby/test_data.rb index 5ac4c6b84b..174106139a 100644 --- a/test/ruby/test_data.rb +++ b/test/ruby/test_data.rb @@ -69,15 +69,33 @@ class TestData < Test::Unit::TestCase assert_equal(1, test_kw.foo) assert_equal(2, test_kw.bar) assert_equal(test_kw, klass.new(foo: 1, bar: 2)) + assert_equal(test_kw, klass.new('foo' => 1, 'bar' => 2)) assert_equal(test_kw, test) # Wrong protocol assert_raise(ArgumentError) { klass.new(1) } assert_raise(ArgumentError) { klass.new(1, 2, 3) } - assert_raise(ArgumentError) { klass.new(foo: 1) } - assert_raise(ArgumentError) { klass.new(foo: 1, bar: 2, baz: 3) } - # Could be converted to foo: 1, bar: 2, but too smart is confusing - assert_raise(ArgumentError) { klass.new(1, bar: 2) } + assert_raise(TypeError) do + klass.new(0 => 1, 1 => 2) + end + assert_raise(TypeError) do + klass.new(foo: 0, bar: 2, 0 => 1) + end + assert_raise_with_message(ArgumentError, "missing keyword: :bar") do + klass.new(foo: 1) + end + assert_raise_with_message(ArgumentError, "missing keyword: :bar") do + klass.new('foo' => 1) + end + assert_raise_with_message(ArgumentError, "missing keyword: :bar") do + klass.new(foo: 1, 'foo' => 1) + end + assert_raise_with_message(ArgumentError, "missing keywords: :foo, :bar") do + klass.new(x: 1, y: 2) + end + assert_raise_with_message(ArgumentError, "unknown keyword: :baz") do + klass.new(foo: 1, bar: 2, baz: 3) + end end def test_initialize_redefine -- cgit v1.2.3