Project

General

Profile

Bug #10413

Unexpected block invocation with unknown keywords

Added by ko1 (Koichi Sasada) over 5 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Target version:
ruby -v:
2.2dev
[ruby-core:65837]

Description

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.


Related issues

Related to Ruby master - Bug #10623: rb_hash_delete() can return Qundef in 2.2-rc1Closed12/20/2014Actions

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

  • 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]

Updated by ko1 (Koichi Sasada) over 5 years ago

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

Updated by ko1 (Koichi Sasada) over 5 years ago

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.

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

  • 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]

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

Hello.

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

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

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/

Updated by nagachika (Tomoyuki Chikanaga) over 5 years ago

  • 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.

Updated by usa (Usaku NAKAMURA) over 5 years ago

  • 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.

#9

Updated by usa (Usaku NAKAMURA) over 5 years ago

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

Also available in: Atom PDF