Project

General

Profile

Bug #10623

rb_hash_delete() can return Qundef in 2.2-rc1

Added by tmm1 (Aman Gupta) almost 5 years ago. Updated almost 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
ruby -v:
ruby 2.2.0dev [x86_64-darwin14]
[ruby-core:66989]

Description

The behavior of rb_hash_delete() has changed from 2.1. Before, it would always return Qnil or VALUE. Now it can also return Qundef, which is breaking the posix-spawn gem's usage:

https://github.com/rtomayko/posix-spawn/blob/master/ext/posix-spawn.c#L242-L258

It also appears RTEST(Qundef) returns true, which causes a segfault in posix-spawn gem here:

https://github.com/rtomayko/posix-spawn/blob/master/ext/posix-spawn.c#L425

I think we should revert back to original behavior before 2.2.0 is released.


Related issues

Related to Ruby master - Bug #10413: Unexpected block invocation with unknown keywordsClosed10/22/2014Actions

Associated revisions

Revision 9c6eaad7
Added by ko1 (Koichi Sasada) almost 5 years ago

  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@48958 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 48958
Added by ko1 (Koichi Sasada) almost 5 years ago

  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

Revision 48958
Added by ko1 (Koichi Sasada) almost 5 years ago

  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

Revision 48958
Added by ko1 (Koichi Sasada) almost 5 years ago

  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

Revision 48958
Added by ko1 (Koichi Sasada) almost 5 years ago

  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

Revision 48958
Added by ko1 (Koichi Sasada) almost 5 years ago

  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

Revision 48958
Added by ko1 (Koichi Sasada) almost 5 years ago

  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

Revision 48436ac6
Added by nobu (Nobuyoshi Nakada) almost 5 years ago

hash/delete.c: add declaration

  • ext/-test-/hash/delete.c: add declaration of the function in internal.h. [Bug #10623]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@48967 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 48967
Added by nobu (Nobuyoshi Nakada) almost 5 years ago

hash/delete.c: add declaration

  • ext/-test-/hash/delete.c: add declaration of the function in internal.h. [Bug #10623]

Revision 48967
Added by nobu (Nobuyoshi Nakada) almost 5 years ago

hash/delete.c: add declaration

  • ext/-test-/hash/delete.c: add declaration of the function in internal.h. [Bug #10623]

Revision 48967
Added by nobu (Nobuyoshi Nakada) almost 5 years ago

hash/delete.c: add declaration

  • ext/-test-/hash/delete.c: add declaration of the function in internal.h. [Bug #10623]

Revision 48967
Added by nobu (Nobuyoshi Nakada) almost 5 years ago

hash/delete.c: add declaration

  • ext/-test-/hash/delete.c: add declaration of the function in internal.h. [Bug #10623]

Revision 48967
Added by nobu (Nobuyoshi Nakada) almost 5 years ago

hash/delete.c: add declaration

  • ext/-test-/hash/delete.c: add declaration of the function in internal.h. [Bug #10623]

Revision 48967
Added by nobu (Nobuyoshi Nakada) almost 5 years ago

hash/delete.c: add declaration

  • ext/-test-/hash/delete.c: add declaration of the function in internal.h. [Bug #10623]

History

Updated by tmm1 (Aman Gupta) almost 5 years ago

This regression was introduced in r48114 to fix #10413

Updated by nobu (Nobuyoshi Nakada) almost 5 years ago

I think a function can tell no keys from deleting nil value, like hash_delete in your patch, is necessary.

Updated by tmm1 (Aman Gupta) almost 5 years ago

nobu (Nobuyoshi Nakada) Do you think we need to change rb_hash_delete behavior? Or do you mean we should add another public c-api function for the undef behavior, like rb_hash_delete_xxx

Updated by ko1 (Koichi Sasada) almost 5 years ago

At Comment#3, Nobu said that we still need a function which returns no such entry.

We have three possibility:

(a) reutrn Qundef when no entry (current rb_hash_delete())
(b) return nil when no entry
(c) return passed block results or nil when no entry (previous rb_hash_delete())

So my recommendation is:

(1) Implement (a) with other name (ex: rb_hash_delete_key())
(2) Implement (b) named rb_hash_delete() using (1)
(3) Implement (c) for Hash#delete with (1).

tmm1 (Aman Gupta): is it okay?

Updated by ko1 (Koichi Sasada) almost 5 years ago

How about it?

rb_hash_delete_entry() for (1).

Index: ChangeLog
===================================================================
--- ChangeLog   (revision 48955)
+++ ChangeLog   (working copy)
@@ -1,3 +1,19 @@
+Wed Dec 24 11:24:03 2014  Koichi Sasada  <ko1@atdot.net>
+
+   * hash.c (rb_hash_delete): return Qnil if there are no corresponding
+     entry. [Bug #10623]
+
+   * hash.c (rb_hash_delete_entry): try delete and return Qundef if there
+     are no corresponding entry.
+
+   * internal.h: add rb_hash_delete_entry()'s declaration.
+
+   * symbol.c: use rb_hash_delete_entry().
+
+   * thread.c: use rb_hash_delete_entry().
+
+   * ext/-test-/hash/delete.c: use rb_hash_delete_entry().
+
 Wed Dec 24 09:35:11 2014  NAKAMURA Usaku  <usa@ruby-lang.org>

    * ext/fiddle/extconf.rb: remove ffitarget.h generated by configure on
Index: ext/-test-/hash/delete.c
===================================================================
--- ext/-test-/hash/delete.c    (revision 48955)
+++ ext/-test-/hash/delete.c    (working copy)
@@ -3,7 +3,7 @@
 static VALUE
 hash_delete(VALUE hash, VALUE key)
 {
-    VALUE ret = rb_hash_delete(hash, key);
+    VALUE ret = rb_hash_delete_entry(hash, key);
     return ret == Qundef ? Qnil : rb_ary_new_from_values(1, &ret);
 }

Index: hash.c
===================================================================
--- hash.c  (revision 48955)
+++ hash.c  (working copy)
@@ -969,23 +969,49 @@ rb_hash_index(VALUE hash, VALUE value)
     return rb_hash_key(hash, value);
 }

+/*
+ * delete a specified entry a given key.
+ * if there is the corresponding entry, return a value of the entry.
+ * if there is no corresponding entry, return Qundef.
+ */
 VALUE
-rb_hash_delete(VALUE hash, VALUE key)
+rb_hash_delete_entry(VALUE hash, VALUE key)
 {
     st_data_t ktmp = (st_data_t)key, val;

-    if (!RHASH(hash)->ntbl)
+    if (!RHASH(hash)->ntbl) {
         return Qundef;
-    if (RHASH_ITER_LEV(hash) > 0) {
-   if (st_delete_safe(RHASH(hash)->ntbl, &ktmp, &val, (st_data_t)Qundef)) {
+    }
+    else if (RHASH_ITER_LEV(hash) > 0 &&
+        (st_delete_safe(RHASH(hash)->ntbl, &ktmp, &val, (st_data_t)Qundef))) {
        FL_SET(hash, HASH_DELETED);
        return (VALUE)val;
    }
-    }
-    else if (st_delete(RHASH(hash)->ntbl, &ktmp, &val))
+    else if (st_delete(RHASH(hash)->ntbl, &ktmp, &val)) {
    return (VALUE)val;
+    }
+    else {
     return Qundef;
 }
+}
+
+/*
+ * delete a specified entry by a given key.
+ * if there is the corresponding entry, return a value of the entry.
+ * if there is no corresponding entry, return Qnil.
+ */
+VALUE
+rb_hash_delete(VALUE hash, VALUE key)
+{
+    VALUE deleted_value = rb_hash_delete_entry(hash, key);
+
+    if (deleted_value != Qundef) { /* likely pass */
+   return deleted_value;
+    }
+    else {
+   return Qnil;
+    }
+}

 /*
  *  call-seq:
@@ -1011,13 +1037,20 @@ rb_hash_delete_m(VALUE hash, VALUE key)
     VALUE val;

     rb_hash_modify_check(hash);
-    val = rb_hash_delete(hash, key);
-    if (val != Qundef) return val;
+    val = rb_hash_delete_entry(hash, key);
+
+    if (val != Qundef) {
+   return val;
+    }
+    else {
     if (rb_block_given_p()) {
    return rb_yield(key);
     }
+   else {
     return Qnil;
 }
+    }
+}

 struct shift_var {
     VALUE key;
@@ -1063,7 +1096,7 @@ rb_hash_shift(VALUE hash)
    else {
        rb_hash_foreach(hash, shift_i_safe, (VALUE)&var);
        if (var.key != Qundef) {
-       rb_hash_delete(hash, var.key);
+       rb_hash_delete_entry(hash, var.key);
        return rb_assoc_new(var.key, var.val);
        }
    }
Index: internal.h
===================================================================
--- internal.h  (revision 48955)
+++ internal.h  (working copy)
@@ -1119,6 +1119,9 @@ int rb_bug_reporter_add(void (*func)(FIL
 VALUE rb_str_normalize_ospath(const char *ptr, long len);
 #endif

+/* hash.c (export) */
+VALUE rb_hash_delete_entry(VALUE hash, VALUE key);
+
 /* io.c (export) */
 void rb_maygvl_fd_fix_cloexec(int fd);

Index: symbol.c
===================================================================
--- symbol.c    (revision 48955)
+++ symbol.c    (working copy)
@@ -740,7 +740,7 @@ rb_sym2id(VALUE sym)
        RSYMBOL(sym)->id = id |= num;
        /* make it permanent object */
        set_id_entry(num >>= ID_SCOPE_SHIFT, fstr, sym);
-       rb_hash_delete(global_symbols.dsymbol_fstr_hash, fstr);
+       rb_hash_delete_entry(global_symbols.dsymbol_fstr_hash, fstr);
    }
     }
     else {
Index: thread.c
===================================================================
--- thread.c    (revision 48955)
+++ thread.c    (working copy)
@@ -4806,13 +4806,13 @@ recursive_pop(VALUE list, VALUE obj, VAL
        return 0;
    }
    if (RB_TYPE_P(pair_list, T_HASH)) {
-       rb_hash_delete(pair_list, paired_obj);
+       rb_hash_delete_entry(pair_list, paired_obj);
        if (!RHASH_EMPTY_P(pair_list)) {
        return 1; /* keep hash until is empty */
        }
    }
     }
-    rb_hash_delete(list, obj);
+    rb_hash_delete_entry(list, obj);
     return 1;
 }

Updated by ko1 (Koichi Sasada) almost 5 years ago

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

Applied in changeset r48958.


  • hash.c (rb_hash_delete): return Qnil if there are no corresponding entry. [Bug #10623]
  • hash.c (rb_hash_delete_entry): try delete and return Qundef if there are no corresponding entry.
  • internal.h: add rb_hash_delete_entry()'s declaration.
  • symbol.c: use rb_hash_delete_entry().
  • thread.c: use rb_hash_delete_entry().
  • ext/-test-/hash/delete.c: use rb_hash_delete_entry().

Updated by nagachika (Tomoyuki Chikanaga) almost 5 years ago

  • Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN to 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: REQUIRED

Updated by nagachika (Tomoyuki Chikanaga) almost 5 years ago

  • Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: REQUIRED to 2.0.0: REQUIRED, 2.1: REQUIRED, 2.2: REQUIRED

I didn't confirm it by myyself, but r48114 was already backported into ruby_2_1/ruby_2_0_0. So this should be backported into them too.

Updated by naruse (Yui NARUSE) almost 5 years ago

  • Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED, 2.2: REQUIRED to 2.0.0: REQUIRED, 2.1: REQUIRED, 2.2: DONE
#11

Updated by usa (Usaku NAKAMURA) almost 5 years ago

  • Related to Bug #10413: Unexpected block invocation with unknown keywords added

Also available in: Atom PDF