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

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(", "))));
         }
@@ -1114,6 +1125,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<RARRAY_LEN(keys); i++) {
         VALUE key = RARRAY_AREF(keys, i);
-        int i = rb_struct_pos(s, &key);
+        int i = rb_struct_pos(s, &key, name_only);
         if (i < 0) {
             return h;
         }
@@ -1160,7 +1177,7 @@ rb_struct_init_copy(VALUE copy, VALUE s)
 }
 
 static int
-rb_struct_pos(VALUE s, VALUE *name)
+rb_struct_pos(VALUE s, VALUE *name, bool name_only)
 {
     long i;
     VALUE idx = *name;
@@ -1168,7 +1185,7 @@ rb_struct_pos(VALUE s, VALUE *name)
     if (SYMBOL_P(idx)) {
         return struct_member_pos(s, idx);
     }
-    else if (RB_TYPE_P(idx, T_STRING)) {
+    else if (name_only || RB_TYPE_P(idx, T_STRING)) {
         idx = rb_check_symbol(name);
         if (NIL_P(idx)) return -1;
         return struct_member_pos(s, idx);
@@ -1240,7 +1257,7 @@ invalid_struct_pos(VALUE s, VALUE idx)
 VALUE
 rb_struct_aref(VALUE s, VALUE idx)
 {
-    int i = rb_struct_pos(s, &idx);
+    int i = rb_struct_pos(s, &idx, false);
     if (i < 0) invalid_struct_pos(s, idx);
     return RSTRUCT_GET(s, i);
 }
@@ -1278,7 +1295,7 @@ rb_struct_aref(VALUE s, VALUE idx)
 VALUE
 rb_struct_aset(VALUE s, VALUE idx, VALUE val)
 {
-    int i = rb_struct_pos(s, &idx);
+    int i = rb_struct_pos(s, &idx, false);
     if (i < 0) invalid_struct_pos(s, idx);
     rb_struct_modify(s);
     RSTRUCT_SET(s, i, val);
@@ -1286,18 +1303,18 @@ rb_struct_aset(VALUE s, VALUE idx, VALUE val)
 }
 
 FUNC_MINIMIZED(VALUE rb_struct_lookup(VALUE s, VALUE idx));
-NOINLINE(static VALUE rb_struct_lookup_default(VALUE s, VALUE idx, VALUE notfound));
+NOINLINE(static VALUE rb_struct_lookup_default(VALUE s, VALUE idx, VALUE notfound, bool name_only));
 
 VALUE
 rb_struct_lookup(VALUE s, VALUE idx)
 {
-    return rb_struct_lookup_default(s, idx, Qnil);
+    return rb_struct_lookup_default(s, idx, Qnil, false);
 }
 
 static VALUE
-rb_struct_lookup_default(VALUE s, VALUE idx, VALUE notfound)
+rb_struct_lookup_default(VALUE s, VALUE idx, VALUE notfound, bool name_only)
 {
-    int i = rb_struct_pos(s, &idx);
+    int i = rb_struct_pos(s, &idx, name_only);
     if (i < 0) return notfound;
     return RSTRUCT_GET(s, i);
 }
@@ -1733,6 +1750,21 @@ rb_data_define(VALUE super, ...)
     return klass;
 }
 
+static int
+data_hash_set_i(VALUE key, VALUE val, VALUE arg)
+{
+    struct struct_hash_set_arg *args = (struct struct_hash_set_arg *)arg;
+    int i = struct_hash_aset(key, val, args, true);
+    if (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