Project

General

Profile

Bug #15479

Array#reject! modifies literal Array

Added by Eregon (Benoit Daloze) almost 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-linux]
[ruby-core:90781]

Description

This was found by running ruby/spec with -R2 (such as mspec -R2 -fs core/array/reject_spec.rb).
TravisCI log on https://travis-ci.org/ruby/spec/jobs/473175799#L539

Here is a simple reproducer.
MRI seems to modify the Array literal permanently:

3.times do
  a = [1, 2, 3, 4]
  puts "initial: #{a}"
  begin
    a.reject! do |x|
      case x
      when 2 then true
      when 3 then raise StandardError, 'Oops'
      else false
      end
    end
  rescue StandardError
  end

  puts "after:   #{a}"
end

prints

initial: [1, 2, 3, 4]
after:   [1, 3, 4]
initial: [1, 3, 4, 4]
after:   [1, 3, 4, 4]
initial: [1, 3, 4, 4]
after:   [1, 3, 4, 4]

2.5.3 behaves fine, but trunk is also affected.


Related issues

Related to Ruby master - Feature #15349: Use a shared array for the `duparray` instructionClosedActions

Updated by MSP-Greg (Greg L) almost 2 years ago

Is the distinction really whether 2.6.0 & trunk only modify the array if reject! successfully enumerates all array members?

But if so, why does it modify the first loop? Strange behavior...

Updated by k0kubun (Takashi Kokubun) almost 2 years ago

  • Assignee set to tenderlovemaking (Aaron Patterson)

tenderlovemaking (Aaron Patterson) According to git bisect, https://github.com/ruby/ruby/commit/d46cd60f3cfb88f1591d6ce0f23e750d3833434f (Feature #15289) seems to have triggered this. Could you check it?

Updated by mrkn (Kenta Murata) almost 2 years ago

This is a smaller reproducible code.

mrkn-mbp15-late2016:bigdecimal mrkn$ cat t.rb
def foo
  a = [1, 2, 3, 4]
  a.reject! {|x|
    next true if x == 2
    return a if x == 3
    false
  }
end

p foo
p foo
mrkn-mbp15-late2016:bigdecimal mrkn$ ruby -v t.rb
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin18]
[1, 3, 4]
[1, 3, 4, 4]

Updated by k0kubun (Takashi Kokubun) almost 2 years ago

Oops, I copy-pasted the ticket number in the commit message, but actually it should have been Feature #15349.

#5

Updated by k0kubun (Takashi Kokubun) almost 2 years ago

  • Related to Feature #15349: Use a shared array for the `duparray` instruction added
#6

Updated by mrkn (Kenta Murata) almost 2 years ago

  • Status changed from Open to Assigned
#7

Updated by k0kubun (Takashi Kokubun) almost 2 years ago

  • Status changed from Assigned to Closed

Applied in changeset trunk|r66632.


erb.rb: increase warn level only when non-zero safe_level

is given.

This is merging Eric's patch in [Bug #15479] to Ruby 2.6's behavior in r66631.

Updated by k0kubun (Takashi Kokubun) almost 2 years ago

  • Status changed from Closed to Assigned

oops, closed this ticket by a wrong ticket number in my commit... reopening.

Updated by tenderlovemaking (Aaron Patterson) almost 2 years ago

It seems like this is also a bug in Ruby 2.5 and Ruby 2.4. If you modify the test program a little bit like this:

def foo
  b = [1, 2, 3, 4]
  3.times do
    a = b.dup
    puts "initial: #{a}"
    begin
      a.reject! do |x|
        case x
        when 2 then true
        when 3 then raise StandardError, 'Oops'
        else false
        end
      end
    rescue StandardError
    end

    puts "after:   #{a}"
  end
end

foo

It will fail on 2.4.X, 2.5.X, and 2.6. It seems like "reject!" isn't unsharing the array. I will make a patch to fix this.

#10

Updated by tenderlovemaking (Aaron Patterson) almost 2 years ago

  • Status changed from Assigned to Closed

Applied in changeset trunk|r66756.


Mark array as "going to be modified" in Array#reject!

Before this patch, if reject! is called on a shared array it can
mutate the shared array rather than a copy. This patch marks the array
as "going to be modified" so that the shared source array isn't
mutated.

[Bug #15479] [ruby-core:90781]

Updated by tenderlovemaking (Aaron Patterson) almost 2 years ago

  • Backport changed from 2.4: DONTNEED, 2.5: DONTNEED, 2.6: REQUIRED to 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED

r66756 should be backported, so I updated the backport field.

Updated by nagachika (Tomoyuki Chikanaga) almost 2 years ago

  • Backport changed from 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED to 2.4: REQUIRED, 2.5: DONE, 2.6: REQUIRED

ruby_2_5 r66784 merged revision(s) 66756.

Updated by naruse (Yui NARUSE) almost 2 years ago

  • Backport changed from 2.4: REQUIRED, 2.5: DONE, 2.6: REQUIRED to 2.4: REQUIRED, 2.5: DONE, 2.6: DONE

ruby_2_6 r66847 merged revision(s) 66756.

Updated by usa (Usaku NAKAMURA) almost 2 years ago

  • Backport changed from 2.4: REQUIRED, 2.5: DONE, 2.6: DONE to 2.4: DONE, 2.5: DONE, 2.6: DONE

ruby_2_4 r66966 merged revision(s) 66756.

Also available in: Atom PDF