Bug #2545

Array#delete_if is borked if user calls 'break'

Added by Brian Candler over 5 years ago. Updated almost 4 years ago.

[ruby-core:27366]
Status:Closed
Priority:Normal
Assignee:Yukihiro Matsumoto
ruby -v:ruby 1.8.7 (2009-06-12 patchlevel 174) [x86_64-linux] Backport:

Description

=begin
Array is corrupted if you break out of a delete_if { ... } loop. I would expect that the elements already marked as deleted would be deleted, and the remainder of the array would be unchanged.

a = [5,6,7,8,9,10]
=> [5, 6, 7, 8, 9, 10]
a.delete_if { |x| break if x > 8; x < 7 }
=> nil
a
=> [7, 8, 7, 8, 9, 10]

RUBY_VERSION
=> "1.8.7"
RUBY_PATCHLEVEL
=> 174
=end


Related issues

Related to Ruby trunk - Bug #10722: Array#keep_if is borked if user calls 'break' Closed 01/09/2015
Related to Ruby trunk - Feature #10714: Array#reject! nonlinear performance problem Closed 01/08/2015

Associated revisions

Revision 32360
Added by Nobuyoshi Nakada almost 4 years ago

  • array.c (rb_ary_reject_bang, rb_ary_delete_if): rejected elements should be removed. fixed [Bug #2545]

Revision 32360
Added by Nobuyoshi Nakada almost 4 years ago

  • array.c (rb_ary_reject_bang, rb_ary_delete_if): rejected elements should be removed. fixed [Bug #2545]

Revision 32373
Added by Nobuyoshi Nakada almost 4 years ago

  • array.c (ary_reject_bang): should not remove elements which are not yielded. [Bug #2545]

Revision 32373
Added by Nobuyoshi Nakada almost 4 years ago

  • array.c (ary_reject_bang): should not remove elements which are not yielded. [Bug #2545]

History

#1 Updated by Nobuyoshi Nakada over 5 years ago

Hi,

At Sat, 2 Jan 2010 05:55:00 +0900,
Brian Candler wrote in :

Array is corrupted if you break out of a delete_if { ... }
loop. I would expect that the elements already marked as
deleted would be deleted, and the remainder of the array
would be unchanged.

The behavior would be an implementation detail, and should be
undefined (or implementation defined), I guess.

Index: array.c
===================================================================
--- array.c (revision 26229)
+++ array.c (working copy)
@@ -2307,7 +2307,18 @@ rb_ary_reject_bang(VALUE ary)
     for (i1 = i2 = 0; i1 < RARRAY_LEN(ary); i1++) {
    VALUE v = RARRAY_PTR(ary)[i1];
-   if (RTEST(rb_yield(v))) continue;
    if (i1 != i2) {
+       int state = 0;
+       if (RTEST(rb_protect(rb_yield, v, &state))) continue;
        rb_ary_store(ary, i2, v);
+       if (state) {
+       VALUE *ptr = RARRAY_PTR(ary);
+       long len = RARRAY_LEN(ary);
+       MEMCPY(ptr + i2 + 1, ptr + i1 + 1, VALUE, len - i1 - 1);
+       ARY_SET_LEN(ary, len - i1 + i2);
+       rb_jump_tag(state);
+       }
+   }
+   else {
+       if (RTEST(rb_yield(v))) continue;
    }
    i2++;

Nobu Nakada

#2 Updated by Shyouhei Urabe over 5 years ago

  • Status changed from Open to Assigned
  • Assignee set to Yukihiro Matsumoto

I don't think so. I'd also expect as the reporter did. Isn't it a bug?

Assigning to matz because this can be a design issue.

#3 Updated by Dave B almost 4 years ago

Just met this problem:
ruby 1.8.7 (2011-02-18 patchlevel 334) [i386-mingw32]

I'd also consider it a bug and that the ruby implementation should be hidden from the user. Once an element has been selected for deletion, at the end of this iteration, it should be expected to be gone. When using very large arrays, where the programmer knows of a shortcut (e.g. the rest of the array need not be considered), s/he should be encouraged to handle it with 'break'. In testing, I was left wondering whether 'delete_if' was non-destructive, because nothing had changed, and started looking for a bang! method.
To achieve the current behaviour, I only need an Array#dup above the loop.

Thanks to Nobu for working a patch.

daz

#4 Updated by Nobuyoshi Nakada almost 4 years ago

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

This issue was solved with changeset r32360.
Brian, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • array.c (rb_ary_reject_bang, rb_ary_delete_if): rejected elements should be removed. fixed [Bug #2545]

#5 Updated by Nobuyoshi Nakada almost 4 years ago

  • Category changed from core to core
  • Project changed from Ruby 1.8 to Ruby trunk
  • Target version deleted (Ruby 1.8.7)

#6 Updated by Tomoyuki Chikanaga almost 4 years ago

Hi,

According to test added by r32360,

a = [ 5, 6, 7, 8, 9, 10 ]
a.delete_if {|i| break i if i > 8; i < 7}

it results
a # => [7, 8]

But I feel it could be [7, 8, 9, 10] because block didn't return true for 9, 10.
Matz, How do you think about it?

#7 Updated by _ wanabe 5 months ago

  • Related to Bug #10722: Array#keep_if is borked if user calls 'break' added

#8 Updated by Nobuyoshi Nakada 5 months ago

  • Related to Feature #10714: Array#reject! nonlinear performance problem added

Also available in: Atom PDF