Bug #10413: Unexpected block invocation with unknown keywords

• Status: Closed
• Assignee: ko1 (Koichi Sasada)
• Target version: 2.2.0
• ruby -v: 2.2dev
• Backport: 2.0.0: DONE, 2.1: DONE

[ko1 (Koichi Sasada)]

When unknown keywords are passed, unexpected block is invoked.

def bar(k2: 'v2')
end

def foo
  bar(k1: 1)
end

foo(){
  puts caller_locations
  raise "unreachable"
}

current behavior:

ruby 2.2.0dev (2014-10-22 trunk 48083) [i386-mswin32_110]
test.rb:10:in `block in <main>': unreachable (RuntimeError)
	from test.rb:5:in `foo'
	from test.rb:8:in `<main>'
test.rb:5:in `foo'
test.rb:8:in `<main>'

expected:

../../gitruby/test.rb:5:in `foo': unknown keyword: k1 (ArgumentError)
        from ../../gitruby/test.rb:8:in `<main>'

This strange behavior is because "unknown_keyword_error" in class.c calls rb_hash_delete() (Hash#delete) with :k1 and invoke passed block.

I'm now re-writing all of this part and my branch does not have this problem.
However, Ruby 2.0 and 2.1 have same problem.

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

Applied in changeset r48102.

---

class.c: delete expected keywords directly

• class.c (unknown_keyword_error): delete expected keywords
  directly from raw table, so that the given block is not called.
  [ruby-core:65837] [Bug #10413]

[ko1 (Koichi Sasada)]

• Status changed from Closed to Open

How about to change rb_hash_delete()?
Nobody expect rb_hash_delete() invoke a passed block.

Index: hash.c
===================================================================
--- hash.c	(revision 48083)
+++ hash.c	(working copy)
@@ -972,8 +972,8 @@ rb_hash_index(VALUE hash, VALUE value)
     return rb_hash_key(hash, value);
 }
 
-static VALUE
-rb_hash_delete_key(VALUE hash, VALUE key)
+VALUE
+rb_hash_delete(VALUE hash, VALUE key)
 {
     st_data_t ktmp = (st_data_t)key, val;
 
@@ -1008,13 +1008,13 @@ rb_hash_delete_key(VALUE hash, VALUE key
  *
  */
 
-VALUE
-rb_hash_delete(VALUE hash, VALUE key)
+static VALUE
+rb_hash_delete_m(VALUE hash, VALUE key)
 {
     VALUE val;
 
     rb_hash_modify_check(hash);
-    val = rb_hash_delete_key(hash, key);
+    val = rb_hash_delete(hash, key);
     if (val != Qundef) return val;
     if (rb_block_given_p()) {
 	return rb_yield(key);
@@ -1066,7 +1066,7 @@ rb_hash_shift(VALUE hash)
 	else {
 	    rb_hash_foreach(hash, shift_i_safe, (VALUE)&var);
 	    if (var.key != Qundef) {
-		rb_hash_delete_key(hash, var.key);
+		rb_hash_delete(hash, var.key);
 		return rb_assoc_new(var.key, var.val);
 	    }
 	}
@@ -3881,7 +3881,7 @@ Init_Hash(void)
     rb_define_method(rb_cHash,"values_at", rb_hash_values_at, -1);
 
     rb_define_method(rb_cHash,"shift", rb_hash_shift, 0);
-    rb_define_method(rb_cHash,"delete", rb_hash_delete, 1);
+    rb_define_method(rb_cHash,"delete", rb_hash_delete_m, 1);
     rb_define_method(rb_cHash,"delete_if", rb_hash_delete_if, 0);
     rb_define_method(rb_cHash,"keep_if", rb_hash_keep_if, 0);
     rb_define_method(rb_cHash,"select", rb_hash_select, 0);

[ko1 (Koichi Sasada)]

Nobu also proposed that

(1) export rb_hash_delete_key()
(2) and obsolete rb_hash_delete() (show warning)

This approach is very compatible way.

My idea (change the specification rb_hash_delete()) has advantage that this helps rb_hash_delete() users who don't expect invoking block. However, it has disadvantage who expect invoking block.

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset r48114.

---

hash.c: rb_hash_delete does not call the block

• hash.c (rb_hash_delete): now does not call the block given to
  the current method. [ruby-core:65861] [Bug #10413]

[nagachika (Tomoyuki Chikanaga)]

Hello.

r48114 introduce an incompatibility. I'll backport only r48102 into ruby_2_1.
Is there any problem about the approach of r48102?

[nobu (Nobuyoshi Nakada)]

I changed the behavior of rb_hash_delete() itself, and see if somebody rants.

rb_hash_delete() is still called at several places, and possibly other extension libraries,
and they don't seem to intend to call the block given to the calling method.
e.g. http://wannabe53.wordpress.com/2012/04/01/%E6%8B%A1%E5%BC%B5%E3%83%A9%E3%82%A4%E3%83%96%E3%83%A9%E3%83%AA%E3%81%AE%E3%83%A1%E3%82%BD%E3%83%83%E3%83%89%E5%AE%9A%E7%BE%A9%E3%81%A7%E3%83%8F%E3%83%9E%E3%81%A3%E3%81%9F/

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED to 2.0.0: REQUIRED, 2.1: DONE

I backported only r48114 into ruby_2_1 at r48137.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.0.0: REQUIRED, 2.1: DONE to 2.0.0: DONE, 2.1: DONE

make and apply a patch like r48102 to ruby_2_0_0 at r48284.

[usa (Usaku NAKAMURA)]

• Related to Bug #10623: rb_hash_delete() can return Qundef in 2.2-rc1 added