Feature #1125

[*x] (array consisting only of a splat) does not necessarily return a new array

Added by Daniel Luz over 6 years ago. Updated over 3 years ago.

[ruby-core:21901]
Status:Closed
Priority:Normal
Assignee:Nobuyoshi Nakada

Description

=begin
For [*x], these are basically the possible outcomes:

  1. if x is an Array, returns it unmodified.
  2. elsif x responds to to_ary (to_a on 1.9.1), invokes that method and returns its result unmodified.
  3. else, returns a new array with x as its only element.

Cases #1 and #2 IMO violate the POLS, as I expected that an array literal would always return a new array. The practical consequence here is that I expected I'd be free to modify it without side effects. (For comparison, "#{x}" always returns a new string)

Simple test case:
x = [1, 2, 3]
[*x] << 4
p x # => [1, 2, 3, 4]

Thus, I propose ensuring these two cases always return new arrays.
One possible solution would be dup'ing the resulting array (I guess that'd have a rather low cost; the third case would result in an unnecessary dup, but at least it's just a single-item array). Another one would be to dumb down the interpreter, making it create a zero-length array and then concat the result of the splat to it.
=end


Related issues

Duplicated by Backport193 - Backport #5124: foo = [*bar] implies foo.equal?(bar) Closed 07/31/2011

Associated revisions

Revision 34633
Added by Nobuyoshi Nakada over 3 years ago

  • insns.def (splatarray): make new array if flag is set.
  • compile.c (iseq_compile_each): make new array with splat. [Feature #1125]

Revision 34633
Added by Nobuyoshi Nakada over 3 years ago

  • insns.def (splatarray): make new array if flag is set.
  • compile.c (iseq_compile_each): make new array with splat. [Feature #1125]

History

#1 Updated by Marc-Andre Lafortune over 5 years ago

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

=begin

=end

#2 Updated by Yusuke Endoh almost 5 years ago

=begin
Hi,

I expected that an array literal would always return a new array.

Agreed, but "bug" is too strong a word for this.
I move this ticket to 1.9.x feature.

--
Yusuke Endoh mame@tsg.ne.jp
=end

#3 Updated by Yusuke Endoh almost 5 years ago

  • Target version set to 2.0.0

=begin

=end

#4 Updated by Yusuke Endoh over 3 years ago

  • Assignee changed from Yukihiro Matsumoto to Yusuke Endoh

Hello,

I'll commit the following patch unless there is objection:

diff --git a/compile.c b/compile.c
index 32bda52..9ea2a3c 100644
--- a/compile.c
+++ b/compile.c
@@ -4644,6 +4644,8 @@ iseq_compile_each(rb_iseq_t *iseq, LINK_ANCHOR *ret, NODE * node, int poped)
case NODE_SPLAT:{
COMPILE(ret, "splat", node->nd_head);
ADD_INSN1(ret, nd_line(node), splatarray, Qfalse);
+ ADD_INSN1(ret, nd_line(node), newarray, INT2FIX(0));
+ ADD_INSN(ret, nd_line(node), concatarray);

if (poped) {
    ADD_INSN(ret, nd_line(node), pop);

Yusuke Endoh mame@tsg.ne.jp

#5 Updated by Nobuyoshi Nakada over 3 years ago

Hi,

(12/02/13 23:27), Yusuke Endoh wrote:

I'll commit the following patch unless there is objection:

Isn't the flag of splatarray for this purpose?

diff --git i/compile.c w/compile.c
index 32bda52..c8aced3 100644
--- i/compile.c
+++ w/compile.c
@@ -4643,7 +4643,7 @@ iseq_compile_each(rb_iseq_t *iseq, LINK_ANCHOR *ret, NODE * node, int poped)
}
case NODE_SPLAT:{
COMPILE(ret, "splat", node->nd_head);
- ADD_INSN1(ret, nd_line(node), splatarray, Qfalse);
+ ADD_INSN1(ret, nd_line(node), splatarray, Qtrue);

if (poped) {
    ADD_INSN(ret, nd_line(node), pop);

diff --git i/insns.def w/insns.def
index 8ec05de..59a98c0 100644
--- i/insns.def
+++ w/insns.def
@@ -533,6 +533,9 @@ splatarray
if (NIL_P(tmp)) {
tmp = rb_ary_new3(1, ary);
}
+ else if (RTEST(flag)) {
+ tmp = rb_ary_dup(tmp);
+ }
obj = tmp;
}

diff --git i/test/ruby/test_basicinstructions.rb w/test/ruby/test_basicinstructions.rb
index ff14e4a..fdcd1b0 100644
--- i/test/ruby/test_basicinstructions.rb
+++ w/test/ruby/test_basicinstructions.rb
@@ -665,12 +665,15 @@ class TestBasicInstructions < Test::Unit::TestCase
a = []
assert_equal [], [a]
assert_equal [1], [1, a]
+ assert_not_same a, [*a]
a = [2]
assert_equal [2], [
a]
assert_equal [1, 2], [1, *a]
+ assert_not_same a, [
a]
a = [2, 3]
assert_equal [2, 3], [a]
assert_equal [1, 2, 3], [1, *a]
+ assert_not_same a, [
a]

  a = nil
  assert_equal [], [*a]

--
Nobu Nakada

#6 Updated by Yusuke Endoh over 3 years ago

  • Assignee changed from Yusuke Endoh to Nobuyoshi Nakada

Hello,

2012/2/14 Nobuyoshi Nakada nobu@ruby-lang.org:

Isn't the flag of splatarray for this purpose?

I don't know. Please go ahead. You have control!

Yusuke Endoh mame@tsg.ne.jp

#7 Updated by Nobuyoshi Nakada over 3 years ago

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

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


  • insns.def (splatarray): make new array if flag is set.
  • compile.c (iseq_compile_each): make new array with splat. [Feature #1125]

Also available in: Atom PDF