From 2a294d499bf03211d02695f613f784a05943ea35 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Tue, 3 Nov 2020 14:01:38 -0800 Subject: [PATCH] Make Array methods return Array instances instead of subclass instances This changes the following methods to return Array instances instead of subclass instances: * Array#drop * Array#drop_while * Array#flatten * Array#slice! * Array#slice/#[] * Array#take * Array#take_while * Array#uniq * Array#* Fixes [Bug #6087] --- array.c | 12 +-- spec/ruby/core/array/flatten_spec.rb | 24 +++-- spec/ruby/core/array/multiply_spec.rb | 18 +++- spec/ruby/core/array/shared/slice.rb | 60 ++++++++--- spec/ruby/core/array/uniq_spec.rb | 12 ++- test/ruby/test_array.rb | 140 +++++++++++++++----------- 6 files changed, 171 insertions(+), 95 deletions(-) diff --git a/array.c b/array.c index 6cd64129af..8cb9f51a86 100644 --- a/array.c +++ b/array.c @@ -1189,7 +1189,7 @@ ary_make_partial_step(VALUE ary, VALUE klass, long offset, long len, long step) static VALUE ary_make_shared_copy(VALUE ary) { - return ary_make_partial(ary, rb_obj_class(ary), 0, RARRAY_LEN(ary)); + return ary_make_partial(ary, rb_cArray, 0, RARRAY_LEN(ary)); } enum ary_take_pos_flags @@ -1628,7 +1628,7 @@ rb_ary_subseq_step(VALUE ary, long beg, long len, long step) if (alen < len || alen < beg + len) { len = alen - beg; } - klass = rb_obj_class(ary); + klass = rb_cArray; if (len == 0) return ary_new(klass, 0); if (step == 0) rb_raise(rb_eArgError, "slice step cannot be zero"); @@ -4010,7 +4010,6 @@ ary_slice_bang_by_rb_ary_splice(VALUE ary, long pos, long len) } else { VALUE arg2 = rb_ary_new4(len, RARRAY_CONST_PTR_TRANSIENT(ary)+pos); - RBASIC_SET_CLASS(arg2, rb_obj_class(ary)); rb_ary_splice(ary, pos, len, 0, 0); return arg2; } @@ -4820,7 +4819,7 @@ rb_ary_times(VALUE ary, VALUE times) len = NUM2LONG(times); if (len == 0) { - ary2 = ary_new(rb_obj_class(ary), 0); + ary2 = ary_new(rb_cArray, 0); goto out; } if (len < 0) { @@ -4831,7 +4830,7 @@ rb_ary_times(VALUE ary, VALUE times) } len *= RARRAY_LEN(ary); - ary2 = ary_new(rb_obj_class(ary), len); + ary2 = ary_new(rb_cArray, len); ARY_SET_LEN(ary2, len); ptr = RARRAY_CONST_PTR_TRANSIENT(ary); @@ -5947,7 +5946,6 @@ rb_ary_uniq(VALUE ary) hash = ary_make_hash(ary); uniq = rb_hash_values(hash); } - RBASIC_SET_CLASS(uniq, rb_obj_class(ary)); if (hash) { ary_recycle_hash(hash); } @@ -6146,7 +6144,7 @@ flatten(VALUE ary, int level) st_clear(memo); } - RBASIC_SET_CLASS(result, rb_obj_class(ary)); + RBASIC_SET_CLASS(result, rb_cArray); return result; } diff --git a/spec/ruby/core/array/flatten_spec.rb b/spec/ruby/core/array/flatten_spec.rb index e7cd114b9b..b2aa015764 100644 --- a/spec/ruby/core/array/flatten_spec.rb +++ b/spec/ruby/core/array/flatten_spec.rb @@ -75,12 +75,24 @@ describe "Array#flatten" do [[obj]].flatten(1) end - it "returns subclass instance for Array subclasses" do - ArraySpecs::MyArray[].flatten.should be_an_instance_of(ArraySpecs::MyArray) - ArraySpecs::MyArray[1, 2, 3].flatten.should be_an_instance_of(ArraySpecs::MyArray) - ArraySpecs::MyArray[1, [2], 3].flatten.should be_an_instance_of(ArraySpecs::MyArray) - ArraySpecs::MyArray[1, [2, 3], 4].flatten.should == ArraySpecs::MyArray[1, 2, 3, 4] - [ArraySpecs::MyArray[1, 2, 3]].flatten.should be_an_instance_of(Array) + ruby_version_is ''...'3.0' do + it "returns subclass instance for Array subclasses" do + ArraySpecs::MyArray[].flatten.should be_an_instance_of(ArraySpecs::MyArray) + ArraySpecs::MyArray[1, 2, 3].flatten.should be_an_instance_of(ArraySpecs::MyArray) + ArraySpecs::MyArray[1, [2], 3].flatten.should be_an_instance_of(ArraySpecs::MyArray) + ArraySpecs::MyArray[1, [2, 3], 4].flatten.should == ArraySpecs::MyArray[1, 2, 3, 4] + [ArraySpecs::MyArray[1, 2, 3]].flatten.should be_an_instance_of(Array) + end + end + + ruby_version_is '3.0' do + it "returns Array instance for Array subclasses" do + ArraySpecs::MyArray[].flatten.should be_an_instance_of(Array) + ArraySpecs::MyArray[1, 2, 3].flatten.should be_an_instance_of(Array) + ArraySpecs::MyArray[1, [2], 3].flatten.should be_an_instance_of(Array) + ArraySpecs::MyArray[1, [2, 3], 4].flatten.should == Array[1, 2, 3, 4] + [ArraySpecs::MyArray[1, 2, 3]].flatten.should be_an_instance_of(Array) + end end it "is not destructive" do diff --git a/spec/ruby/core/array/multiply_spec.rb b/spec/ruby/core/array/multiply_spec.rb index 8ccec13d42..16e407348b 100644 --- a/spec/ruby/core/array/multiply_spec.rb +++ b/spec/ruby/core/array/multiply_spec.rb @@ -76,10 +76,20 @@ describe "Array#* with an integer" do @array = ArraySpecs::MyArray[1, 2, 3, 4, 5] end - it "returns a subclass instance" do - (@array * 0).should be_an_instance_of(ArraySpecs::MyArray) - (@array * 1).should be_an_instance_of(ArraySpecs::MyArray) - (@array * 2).should be_an_instance_of(ArraySpecs::MyArray) + ruby_version_is ''...'3.0' do + it "returns a subclass instance" do + (@array * 0).should be_an_instance_of(ArraySpecs::MyArray) + (@array * 1).should be_an_instance_of(ArraySpecs::MyArray) + (@array * 2).should be_an_instance_of(ArraySpecs::MyArray) + end + end + + ruby_version_is '3.0' do + it "returns an Array instance" do + (@array * 0).should be_an_instance_of(Array) + (@array * 1).should be_an_instance_of(Array) + (@array * 2).should be_an_instance_of(Array) + end end it "does not call #initialize on the subclass instance" do diff --git a/spec/ruby/core/array/shared/slice.rb b/spec/ruby/core/array/shared/slice.rb index f36890fa4e..845be768c6 100644 --- a/spec/ruby/core/array/shared/slice.rb +++ b/spec/ruby/core/array/shared/slice.rb @@ -397,28 +397,56 @@ describe :array_slice, shared: true do @array = ArraySpecs::MyArray[1, 2, 3, 4, 5] end - it "returns a subclass instance with [n, m]" do - @array.send(@method, 0, 2).should be_an_instance_of(ArraySpecs::MyArray) + ruby_version_is ''...'3.0' do + it "returns a subclass instance with [n, m]" do + @array.send(@method, 0, 2).should be_an_instance_of(ArraySpecs::MyArray) + end + + it "returns a subclass instance with [-n, m]" do + @array.send(@method, -3, 2).should be_an_instance_of(ArraySpecs::MyArray) + end + + it "returns a subclass instance with [n..m]" do + @array.send(@method, 1..3).should be_an_instance_of(ArraySpecs::MyArray) + end + + it "returns a subclass instance with [n...m]" do + @array.send(@method, 1...3).should be_an_instance_of(ArraySpecs::MyArray) + end + + it "returns a subclass instance with [-n..-m]" do + @array.send(@method, -3..-1).should be_an_instance_of(ArraySpecs::MyArray) + end + + it "returns a subclass instance with [-n...-m]" do + @array.send(@method, -3...-1).should be_an_instance_of(ArraySpecs::MyArray) + end end - it "returns a subclass instance with [-n, m]" do - @array.send(@method, -3, 2).should be_an_instance_of(ArraySpecs::MyArray) - end + ruby_version_is '3.0' do + it "returns a Array instance with [n, m]" do + @array.send(@method, 0, 2).should be_an_instance_of(Array) + end - it "returns a subclass instance with [n..m]" do - @array.send(@method, 1..3).should be_an_instance_of(ArraySpecs::MyArray) - end + it "returns a Array instance with [-n, m]" do + @array.send(@method, -3, 2).should be_an_instance_of(Array) + end - it "returns a subclass instance with [n...m]" do - @array.send(@method, 1...3).should be_an_instance_of(ArraySpecs::MyArray) - end + it "returns a Array instance with [n..m]" do + @array.send(@method, 1..3).should be_an_instance_of(Array) + end - it "returns a subclass instance with [-n..-m]" do - @array.send(@method, -3..-1).should be_an_instance_of(ArraySpecs::MyArray) - end + it "returns a Array instance with [n...m]" do + @array.send(@method, 1...3).should be_an_instance_of(Array) + end - it "returns a subclass instance with [-n...-m]" do - @array.send(@method, -3...-1).should be_an_instance_of(ArraySpecs::MyArray) + it "returns a Array instance with [-n..-m]" do + @array.send(@method, -3..-1).should be_an_instance_of(Array) + end + + it "returns a Array instance with [-n...-m]" do + @array.send(@method, -3...-1).should be_an_instance_of(Array) + end end it "returns an empty array when m == n with [m...n]" do diff --git a/spec/ruby/core/array/uniq_spec.rb b/spec/ruby/core/array/uniq_spec.rb index fd604987c1..5911c23e6a 100644 --- a/spec/ruby/core/array/uniq_spec.rb +++ b/spec/ruby/core/array/uniq_spec.rb @@ -128,8 +128,16 @@ describe "Array#uniq" do [false, nil, 42].uniq { :bar }.should == [false] end - it "returns subclass instance on Array subclasses" do - ArraySpecs::MyArray[1, 2, 3].uniq.should be_an_instance_of(ArraySpecs::MyArray) + ruby_version_is ''...'3.0' do + it "returns subclass instance on Array subclasses" do + ArraySpecs::MyArray[1, 2, 3].uniq.should be_an_instance_of(ArraySpecs::MyArray) + end + end + + ruby_version_is '3.0' do + it "returns Array instance on Array subclasses" do + ArraySpecs::MyArray[1, 2, 3].uniq.should be_an_instance_of(Array) + end end it "properly handles an identical item even when its #eql? isn't reflexive" do diff --git a/test/ruby/test_array.rb b/test/ruby/test_array.rb index d6c15b8673..0ed97de1ae 100644 --- a/test/ruby/test_array.rb +++ b/test/ruby/test_array.rb @@ -15,6 +15,11 @@ class TestArray < Test::Unit::TestCase $VERBOSE = @verbose end + def assert_equal_instance(x, y, *msg) + assert_equal(x, y, *msg) + assert_instance_of(x.class, y) + end + def test_percent_i assert_equal([:foo, :bar], %i[foo bar]) assert_equal([:"\"foo"], %i["foo]) @@ -114,6 +119,9 @@ class TestArray < Test::Unit::TestCase assert_equal('1', (x * 1).join(":")) assert_equal('', (x * 0).join(":")) + assert_instance_of(Array, (@cls[] * 5)) + assert_instance_of(Array, (@cls[1] * 5)) + *x = *(1..7).to_a assert_equal(7, x.size) assert_equal([1, 2, 3, 4, 5, 6, 7], x) @@ -842,14 +850,14 @@ class TestArray < Test::Unit::TestCase a2 = @cls[ 5, 6 ] a3 = @cls[ 4, a2 ] a4 = @cls[ a1, a3 ] - assert_equal(@cls[1, 2, 3, 4, 5, 6], a4.flatten) - assert_equal(@cls[ a1, a3], a4) + assert_equal_instance([1, 2, 3, 4, 5, 6], a4.flatten) + assert_equal_instance(@cls[ a1, a3], a4) a5 = @cls[ a1, @cls[], a3 ] - assert_equal(@cls[1, 2, 3, 4, 5, 6], a5.flatten) - assert_equal(@cls[1, 2, 3, 4, [5, 6]], a5.flatten(1)) - assert_equal(@cls[], @cls[].flatten) - assert_equal(@cls[], + assert_equal_instance([1, 2, 3, 4, 5, 6], a5.flatten) + assert_equal_instance([1, 2, 3, 4, [5, 6]], a5.flatten(1)) + assert_equal_instance([], @cls[].flatten) + assert_equal_instance([], @cls[@cls[@cls[@cls[],@cls[]],@cls[@cls[]],@cls[]],@cls[@cls[@cls[]]]].flatten) end @@ -1472,59 +1480,59 @@ class TestArray < Test::Unit::TestCase assert_equal(1, a.slice(-100)) assert_nil(a.slice(-101)) - assert_equal(@cls[1], a.slice(0,1)) - assert_equal(@cls[100], a.slice(99,1)) - assert_equal(@cls[], a.slice(100,1)) - assert_equal(@cls[100], a.slice(99,100)) - assert_equal(@cls[100], a.slice(-1,1)) - assert_equal(@cls[99], a.slice(-2,1)) + assert_equal_instance([1], a.slice(0,1)) + assert_equal_instance([100], a.slice(99,1)) + assert_equal_instance([], a.slice(100,1)) + assert_equal_instance([100], a.slice(99,100)) + assert_equal_instance([100], a.slice(-1,1)) + assert_equal_instance([99], a.slice(-2,1)) - assert_equal(@cls[10, 11, 12], a.slice(9, 3)) - assert_equal(@cls[10, 11, 12], a.slice(-91, 3)) + assert_equal_instance([10, 11, 12], a.slice(9, 3)) + assert_equal_instance([10, 11, 12], a.slice(-91, 3)) assert_nil(a.slice(-101, 2)) - assert_equal(@cls[1], a.slice(0..0)) - assert_equal(@cls[100], a.slice(99..99)) - assert_equal(@cls[], a.slice(100..100)) - assert_equal(@cls[100], a.slice(99..200)) - assert_equal(@cls[100], a.slice(-1..-1)) - assert_equal(@cls[99], a.slice(-2..-2)) + assert_equal_instance([1], a.slice(0..0)) + assert_equal_instance([100], a.slice(99..99)) + assert_equal_instance([], a.slice(100..100)) + assert_equal_instance([100], a.slice(99..200)) + assert_equal_instance([100], a.slice(-1..-1)) + assert_equal_instance([99], a.slice(-2..-2)) - assert_equal(@cls[10, 11, 12], a.slice(9..11)) - assert_equal(@cls[98, 99, 100], a.slice(97..)) - assert_equal(@cls[10, 11, 12], a.slice(-91..-89)) - assert_equal(@cls[10, 11, 12], a.slice(-91..-89)) + assert_equal_instance([10, 11, 12], a.slice(9..11)) + assert_equal_instance([98, 99, 100], a.slice(97..)) + assert_equal_instance([10, 11, 12], a.slice(-91..-89)) + assert_equal_instance([10, 11, 12], a.slice(-91..-89)) - assert_equal(@cls[5, 8, 11], a.slice((4..12)%3)) - assert_equal(@cls[95, 97, 99], a.slice((94..)%2)) + assert_equal_instance([5, 8, 11], a.slice((4..12)%3)) + assert_equal_instance([95, 97, 99], a.slice((94..)%2)) # [0] [1] [2] [3] [4] [5] [6] [7] # ary = [ 1 2 3 4 5 6 7 8 ... ] # (0) (1) (2) <- (..7) % 3 # (2) (1) (0) <- (7..) % -3 - assert_equal(@cls[1, 4, 7], a.slice((..7)%3)) - assert_equal(@cls[8, 5, 2], a.slice((7..)% -3)) + assert_equal_instance([1, 4, 7], a.slice((..7)%3)) + assert_equal_instance([8, 5, 2], a.slice((7..)% -3)) # [-98] [-97] [-96] [-95] [-94] [-93] [-92] [-91] [-90] # ary = [ ... 3 4 5 6 7 8 9 10 11 ... ] # (0) (1) (2) <- (-98..-90) % 3 # (2) (1) (0) <- (-90..-98) % -3 - assert_equal(@cls[3, 6, 9], a.slice((-98..-90)%3)) - assert_equal(@cls[11, 8, 5], a.slice((-90..-98)% -3)) + assert_equal_instance([3, 6, 9], a.slice((-98..-90)%3)) + assert_equal_instance([11, 8, 5], a.slice((-90..-98)% -3)) # [ 48] [ 49] [ 50] [ 51] [ 52] [ 53] # [-52] [-51] [-50] [-49] [-48] [-47] # ary = [ ... 49 50 51 52 53 54 ... ] # (0) (1) (2) <- (48..-47) % 2 # (2) (1) (0) <- (-47..48) % -2 - assert_equal(@cls[49, 51, 53], a.slice((48..-47)%2)) - assert_equal(@cls[54, 52, 50], a.slice((-47..48)% -2)) + assert_equal_instance([49, 51, 53], a.slice((48..-47)%2)) + assert_equal_instance([54, 52, 50], a.slice((-47..48)% -2)) idx = ((3..90) % 2).to_a - assert_equal(@cls[*a.values_at(*idx)], a.slice((3..90)%2)) + assert_equal_instance(a.values_at(*idx), a.slice((3..90)%2)) idx = 90.step(3, -2).to_a - assert_equal(@cls[*a.values_at(*idx)], a.slice((90 .. 3)% -2)) + assert_equal_instance(a.values_at(*idx), a.slice((90 .. 3)% -2)) end def test_slice_out_of_range @@ -1550,15 +1558,18 @@ class TestArray < Test::Unit::TestCase assert_equal(@cls[1, 2, 3, 5], a) a = @cls[1, 2, 3, 4, 5] - assert_equal(@cls[3,4], a.slice!(2,2)) + s = a.slice!(2,2) + assert_equal_instance([3,4], s) assert_equal(@cls[1, 2, 5], a) a = @cls[1, 2, 3, 4, 5] - assert_equal(@cls[4,5], a.slice!(-2,2)) + s = a.slice!(-2,2) + assert_equal_instance([4,5], s) assert_equal(@cls[1, 2, 3], a) a = @cls[1, 2, 3, 4, 5] - assert_equal(@cls[3,4], a.slice!(2..3)) + s = a.slice!(2..3) + assert_equal_instance([3,4], s) assert_equal(@cls[1, 2, 5], a) a = @cls[1, 2, 3, 4, 5] @@ -1921,26 +1932,22 @@ class TestArray < Test::Unit::TestCase sc = Class.new(@cls) a = sc[] b = a.dup - assert_instance_of(sc, a.uniq) - assert_equal(sc[], a.uniq) + assert_equal_instance([], a.uniq) assert_equal(b, a) a = sc[1] b = a.dup - assert_instance_of(sc, a.uniq) - assert_equal(sc[1], a.uniq) + assert_equal_instance([1], a.uniq) assert_equal(b, a) a = sc[1, 1] b = a.dup - assert_instance_of(sc, a.uniq) - assert_equal(sc[1], a.uniq) + assert_equal_instance([1], a.uniq) assert_equal(b, a) a = sc[1, 1] b = a.dup - assert_instance_of(sc, a.uniq{|x| x}) - assert_equal(sc[1], a.uniq{|x| x}) + assert_equal_instance([1], a.uniq{|x| x}) assert_equal(b, a) end @@ -2355,23 +2362,23 @@ class TestArray < Test::Unit::TestCase end def test_take - assert_equal([1,2,3], [1,2,3,4,5,0].take(3)) + assert_equal_instance([1,2,3], @cls[1,2,3,4,5,0].take(3)) assert_raise(ArgumentError, '[ruby-dev:34123]') { [1,2].take(-1) } - assert_equal([1,2], [1,2].take(1000000000), '[ruby-dev:34123]') + assert_equal_instance([1,2], @cls[1,2].take(1000000000), '[ruby-dev:34123]') end def test_take_while - assert_equal([1,2], [1,2,3,4,5,0].take_while {|i| i < 3 }) + assert_equal_instance([1,2], @cls[1,2,3,4,5,0].take_while {|i| i < 3 }) end def test_drop - assert_equal([4,5,0], [1,2,3,4,5,0].drop(3)) + assert_equal_instance([4,5,0], @cls[1,2,3,4,5,0].drop(3)) assert_raise(ArgumentError, '[ruby-dev:34123]') { [1,2].drop(-1) } - assert_equal([], [1,2].drop(1000000000), '[ruby-dev:34123]') + assert_equal_instance([], @cls[1,2].drop(1000000000), '[ruby-dev:34123]') end def test_drop_while - assert_equal([3,4,5,0], [1,2,3,4,5,0].drop_while {|i| i < 3 }) + assert_equal_instance([3,4,5,0], @cls[1,2,3,4,5,0].drop_while {|i| i < 3 }) end LONGP = [127, 63, 31, 15, 7].map {|x| 2**x-1 }.find do |x| @@ -2451,6 +2458,7 @@ class TestArray < Test::Unit::TestCase def test_aref assert_raise(ArgumentError) { [][0, 0, 0] } + assert_raise(ArgumentError) { @cls[][0, 0, 0] } end def test_fetch @@ -2954,15 +2962,6 @@ class TestArray < Test::Unit::TestCase end end - class Array2 < Array - end - - def test_array_subclass - assert_equal(Array2, Array2[1,2,3].uniq.class, "[ruby-dev:34581]") - assert_equal(Array2, Array2[1,2][0,1].class) # embedded - assert_equal(Array2, Array2[*(1..100)][1..99].class) #not embedded - end - def test_initialize2 a = [1] * 1000 a.instance_eval { initialize } @@ -3302,3 +3301,24 @@ class TestArray < Test::Unit::TestCase end end end + +class TestArraySubclass < TestArray + def setup + @verbose = $VERBOSE + $VERBOSE = nil + @cls = Class.new(Array) + end + + def test_to_a + a = @cls[ 1, 2, 3 ] + a_id = a.__id__ + assert_equal_instance([1, 2, 3], a.to_a) + assert_not_equal(a_id, a.to_a.__id__) + end + + def test_array_subclass + assert_equal(Array, @cls[1,2,3].uniq.class, "[ruby-dev:34581]") + assert_equal(Array, @cls[1,2][0,1].class) # embedded + assert_equal(Array, @cls[*(1..100)][1..99].class) #not embedded + end +end