Make Data#initialize reject Integer keys

Author: nobu

array.c

@@ -6524,7 +6524,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;

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);

spec/ruby/core/data/deconstruct_keys_spec.rb

@@ -34,29 +34,6 @@
     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,50 +49,53 @@
     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)
 
     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_error(TypeError, /can't convert MockObject to 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

struct.c

@@ -716,18 +716,18 @@ num_members(VALUE klass)
 struct struct_hash_set_arg {
     VALUE self;
     VALUE unknown_keywords;
-    VALUE found_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 rb_struct_pos(VALUE s, VALUE *name, 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;
-    VALUE klass = rb_obj_class(args->self);
-    VALUE members = struct_ivar_get(klass, id_members);
-    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();
@@ -737,13 +737,15 @@ struct_hash_set_i(VALUE key, VALUE val, VALUE arg)
     else {
         rb_struct_modify(args->self);
         RSTRUCT_SET_RAW(args->self, i, val);
-
-        if (NIL_P(args->found_keywords)) {
-            args->found_keywords = rb_hash_new();
-        }
-        VALUE member = rb_ary_entry(members, i);
-        rb_hash_aset(args->found_keywords, member, Qtrue);
     }
+    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;
 }
 
@@ -776,13 +778,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;
-        arg.found_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(", "))));
         }
@@ -1124,6 +1126,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;
@@ -1143,7 +1151,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;
         }
@@ -1171,15 +1179,15 @@ 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;
 
     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);
@@ -1251,7 +1259,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_RAW(s, i);
 }
@@ -1289,26 +1297,26 @@ 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_RAW(s, i, val);
     return 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_RAW(s, i);
 }
@@ -1744,6 +1752,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
@@ -1827,24 +1850,31 @@ rb_data_initialize_m(int argc, const VALUE *argv, VALUE self)
         rb_error_arity(argc, 0, 0);
     }
 
-    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;
-    arg.found_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.found_keywords != Qnil) {
-        VALUE missing = rb_ary_diff(members, rb_hash_keys(arg.found_keywords));
-        if (RARRAY_LEN(missing) > 0) {
-          rb_exc_raise(rb_keyword_error_new("missing", missing));
-        }
+    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));
     }
-    if (arg.unknown_keywords != Qnil) {
-        rb_exc_raise(rb_keyword_error_new("unknown", arg.unknown_keywords));
+    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;
 }
 
@@ -2099,7 +2129,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