[GR-15171] Kwargs fix.

PullRequest: truffleruby/796

Author: aardvark179

CHANGELOG.md

@@ -4,6 +4,7 @@ Bug fixes:
 
 * Fixed `BigDecimal#dup` so it now just returns the receiver, per Ruby 2.5+ semantics (#1680).
 * Implemented `rb_eval_string_protect`.
+* Fixed `rb_get_kwargs` to correctly handle optional and rest arguments.
 
 # 1.0 RC 17
 

spec/ruby/optional/capi/ext/hash_spec.c

@@ -96,6 +96,11 @@ VALUE hash_spec_rb_hash_lookup2(VALUE self, VALUE hash, VALUE key, VALUE def) {
   return rb_hash_lookup2(hash, key, def);
 }
 
+VALUE hash_spec_rb_hash_lookup2_default_undef(VALUE self, VALUE hash, VALUE key) {
+  VALUE ret = rb_hash_lookup2(hash, key, Qundef);
+  return ret == Qundef ? Qtrue : Qfalse;
+}
+
 VALUE hash_spec_rb_hash_new(VALUE self) {
   return rb_hash_new();
 }
@@ -127,6 +132,7 @@ void Init_hash_spec(void) {
   rb_define_method(cls, "rb_hash_lookup_nil", hash_spec_rb_hash_lookup_nil, 2);
   rb_define_method(cls, "rb_hash_lookup", hash_spec_rb_hash_lookup, 2);
   rb_define_method(cls, "rb_hash_lookup2", hash_spec_rb_hash_lookup2, 3);
+  rb_define_method(cls, "rb_hash_lookup2_default_undef", hash_spec_rb_hash_lookup2_default_undef, 2);
   rb_define_method(cls, "rb_hash_new", hash_spec_rb_hash_new, 0);
   rb_define_method(cls, "rb_hash_size", hash_spec_rb_hash_size, 1);
   rb_define_method(cls, "rb_hash_set_ifnone", hash_spec_rb_hash_set_ifnone, 2);

spec/ruby/optional/capi/ext/util_spec.c

@@ -44,6 +44,30 @@ VALUE util_spec_rb_scan_args(VALUE self, VALUE argv, VALUE fmt, VALUE expected,
   return INT2NUM(result);
 }
 
+static VALUE util_spec_rb_get_kwargs(VALUE self, VALUE keyword_hash, VALUE keys, VALUE required, VALUE optional) {
+  int req = FIX2INT(required);
+  int opt = FIX2INT(optional);
+  int len = RARRAY_LEN(keys);
+
+  int values_len = req + (opt < 0 ? -1 - opt : opt);
+  int i = 0;
+
+  ID *ids = malloc(sizeof(VALUE) * len);
+  VALUE *results = malloc(sizeof(VALUE) * values_len);
+  int extracted = 0;
+  VALUE ary = Qundef;
+
+  for (i = 0; i < len; i++) {
+    ids[i] = SYM2ID(rb_ary_entry(keys, i));
+  }
+
+  extracted = rb_get_kwargs(keyword_hash, ids, req, opt, results);
+  ary = rb_ary_new_from_values(extracted, results);
+  free(results);
+  free(ids);
+  return ary;
+}
+
 static VALUE util_spec_rb_long2int(VALUE self, VALUE n) {
   return INT2NUM(rb_long2int(NUM2LONG(n)));
 }
@@ -64,6 +88,7 @@ static VALUE util_spec_rb_sourceline(VALUE self) {
 void Init_util_spec(void) {
   VALUE cls = rb_define_class("CApiUtilSpecs", rb_cObject);
   rb_define_method(cls, "rb_scan_args", util_spec_rb_scan_args, 4);
+  rb_define_method(cls, "rb_get_kwargs", util_spec_rb_get_kwargs, 4);
   rb_define_method(cls, "rb_long2int", util_spec_rb_long2int, 1);
   rb_define_method(cls, "rb_iter_break", util_spec_rb_iter_break, 0);
   rb_define_method(cls, "rb_sourcefile", util_spec_rb_sourcefile, 0);

spec/ruby/optional/capi/hash_spec.rb

@@ -211,6 +211,11 @@
 
         @s.rb_hash_lookup2(hash, :chunky, 10).should == 10
       end
+
+      it "returns undefined if that is the default value specified" do
+        hsh = Hash.new(0)
+        @s.rb_hash_lookup2_default_undef(hsh, :chunky).should be_true
+      end
     end
   end
 

spec/ruby/optional/capi/util_spec.rb

@@ -154,6 +154,50 @@
     end
   end
 
+  describe "rb_get_kwargs" do
+    it "extracts required arguments in the order requested" do
+      h = { :a => 7, :b => 5 }
+      @o.rb_get_kwargs(h, [:b, :a], 2, 0).should == [5, 7]
+      h.should == {}
+    end
+
+    it "extracts required and optional arguments in the order requested" do
+      h = { :a => 7, :c => 12, :b => 5 }
+      @o.rb_get_kwargs(h, [:b, :a, :c], 2, 1).should == [5, 7, 12]
+      h.should == {}
+    end
+
+    it "accepts nil instead of a hash when only optional arguments are requested" do
+      h = nil
+      @o.rb_get_kwargs(h, [:b, :a, :c], 0, 3).should == []
+      h.should == nil
+    end
+
+    it "raises an error if a required argument is not in the hash" do
+      h = { :a => 7, :c => 12, :b => 5 }
+      lambda { @o.rb_get_kwargs(h, [:b, :d], 2, 0) }.should raise_error(ArgumentError, /missing keyword: d/)
+      h.should == {:a => 7, :c => 12}
+    end
+
+    it "does not raise an error for an optional argument not in the hash" do
+      h = { :a => 7, :b => 5 }
+      @o.rb_get_kwargs(h, [:b, :a, :c], 2, 1).should == [5, 7]
+      h.should == {}
+    end
+
+    it "raises an error if there are additional arguments  and optional is positive" do
+      h = { :a => 7, :c => 12, :b => 5 }
+      lambda { @o.rb_get_kwargs(h, [:b, :a], 2, 0) }.should raise_error(ArgumentError, /unknown keyword: c/)
+      h.should == {:c => 12}
+    end
+
+    it "leaves additional arguments in the hash if optional is negative" do
+      h = { :a => 7, :c => 12, :b => 5 }
+      @o.rb_get_kwargs(h, [:b, :a], 2, -1).should == [5, 7]
+      h.should == {:c => 12}
+    end
+  end
+
   platform_is wordsize: 64 do
     describe "rb_long2int" do
       it "raises a RangeError if the value is outside the range of a C int" do

src/main/c/cext/ruby.c

@@ -1655,13 +1655,21 @@ VALUE rb_hash_lookup(VALUE hash, VALUE key) {
 }
 
 VALUE rb_hash_lookup2(VALUE hash, VALUE key, VALUE default_value) {
-  return RUBY_INVOKE(hash, "fetch", key, default_value);
+  VALUE result = RUBY_INVOKE(hash, "_get_or_undefined", key);
+  if (result == Qundef) {
+    result = default_value;
+  }
+  return result;
 }
 
 VALUE rb_hash_set_ifnone(VALUE hash, VALUE if_none) {
   return RUBY_CEXT_INVOKE("rb_hash_set_ifnone", hash, if_none);
 }
 
+VALUE rb_hash_keys(VALUE hash) {
+  return RUBY_INVOKE(hash, "keys");
+}
+
 VALUE rb_hash_key_str(VALUE hash) {
   rb_tr_error("rb_hash_key_str not yet implemented");
 }
@@ -4678,24 +4686,93 @@ VALUE rb_current_receiver(void) {
   rb_tr_error("rb_current_receiver not implemented");
 }
 
-int rb_get_kwargs(VALUE keyword_hash, const ID *table, int required, int optional, VALUE *values) {
-  if (optional == -1) {
-    rb_tr_error("rb_get_kwargs with rest not implemented");
+static VALUE rb_keyword_error_new(const char *error, VALUE keys) {
+  long i = 0, len = RARRAY_LEN(keys);
+  VALUE error_message = rb_sprintf("%s keyword%.*s", error, len > 1, "s");
+
+  if (len > 0) {
+    rb_str_append(error_message, rb_str_new_cstr(": "));
+    while (1) {
+      const VALUE k = RARRAY_AREF(keys, i);
+      Check_Type(k, T_SYMBOL); /* wrong hash is given to rb_get_kwargs */
+      rb_str_append(error_message, rb_sym2str(k));
+      if (++i >= len) break;
+      rb_str_append(error_message, rb_str_new_cstr(", "));
+    }
   }
 
-  if (optional > 0) {
-    rb_tr_error("rb_get_kwargs with optional not implemented");
+  return rb_exc_new_str(rb_eArgError, error_message);
+}
+
+NORETURN(static void rb_keyword_error(const char *error, VALUE keys)) {
+  rb_exc_raise(rb_keyword_error_new(error, keys));
+}
+
+NORETURN(static void unknown_keyword_error(VALUE hash, const ID *table, int keywords)) {
+  int i;
+  for (i = 0; i < keywords; i++) {
+    VALUE key = table[i];
+    rb_hash_delete(hash, key);
   }
+  rb_keyword_error("unknown", rb_hash_keys(hash));
+}
+
+ static VALUE rb_tr_extract_keyword(VALUE keyword_hash, ID key, VALUE *values) {
+   VALUE val = rb_hash_lookup2(keyword_hash, key, Qundef);
+   if (values) {
+     rb_hash_delete(keyword_hash, key);
+   }
+   return val;
+}
+
+int rb_get_kwargs(VALUE keyword_hash, const ID *table, int required, int optional, VALUE *values) {
+  int rest = 0;
+  int extracted = 0;
+  VALUE missing = Qnil;
 
   if (optional < 0) {
-    rb_tr_error("rb_get_kwargs with rest and optional not implemented");
+	rest = 1;
+	optional = -1-optional;
   }
 
   for (int n = 0; n < required; n++) {
-    values[n] = rb_hash_fetch(keyword_hash, table[n]);
+    VALUE val = rb_tr_extract_keyword(keyword_hash, table[n], values);
+    if (values) {
+      values[n] = val;
+    }
+    if (val == Qundef) {
+      if (NIL_P(missing)) {
+        missing = rb_ary_new();
+      }
+      rb_ary_push(missing, table[n]);
+      rb_keyword_error("missing", missing);
+    }
+    extracted++;
+  }
+
+  if (optional && !NIL_P(keyword_hash)) {
+    for (int m = required; m < required + optional; m++) {
+      VALUE val = rb_tr_extract_keyword(keyword_hash, table[m], values);
+      if (values) {
+        values[m] = val;
+      }
+      if (val != Qundef) {
+        extracted++;
+      }
+    }
+  }
+
+  if (!rest && !NIL_P(keyword_hash)) {
+	if (RHASH_SIZE(keyword_hash) > (unsigned int)(values ? 0 : extracted)) {
+	    unknown_keyword_error(keyword_hash, table, required + optional);
+	}
+  }
+
+  for (int i = extracted; i < required + optional; i++) {
+    values[i] = Qundef;
   }
 
-  return required;
+  return extracted;
 }
 
 VALUE rb_extract_keywords(VALUE *orighash) {