Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arrays duplicated by splats (e.g. a_dup = [*a]) share structure, but shouldn't #1118

Closed
gamache opened this Issue Oct 11, 2013 · 9 comments

Comments

Projects
None yet
4 participants
@gamache
Copy link

gamache commented Oct 11, 2013

I've often seen the idiom a_dup = [*a] to duplicate arrays. JRuby seems to optimize [*a] to a, which results in a and a_dup pointing to the same array object. Awkward!

a = [1, 2, 3]
a_dup = [*a]
a << 4 # => [1, 2, 3, 4]
a_dup # => [1, 2, 3, 4]             ### a_dup should not have been mutated!

This bug appears in at least JRuby 1.7.4 to present. It does not appear in MRI.

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Oct 13, 2013

I reformatted the description so that we can copy and evaluate in IRB.

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Oct 13, 2013

This is how it worked in 1.8.7.

irb(main):001:0> a = [1, 2, 3]
=> [1, 2, 3]
irb(main):002:0> a_dup = [*a]
=> [1, 2, 3]
irb(main):003:0> a << 4 
=> [1, 2, 3, 4]
irb(main):004:0> a_dup
=> [1, 2, 3, 4]
irb(main):005:0> RUBY_DESCRIPTION
=> "ruby 1.8.7 (2012-02-08 patchlevel 358) [universal-darwin12.0]"
@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 21, 2013

Looks like a behavioral change some time in 1.9 then. Definitely a bug.

I wonder if these are parsing right? I get largely the same AST for both 1.8 and 1.9, which may account for identical behavior:

$ ast -e "[*a]"
AST:
RootNode 0
  NewlineNode 0
    Splat19Node 0
      VCallNode:a 0

$ jruby --1.8 -S ast -e "[*a]"
AST:
RootNode 0
  NewlineNode 0
    SplatNode 0
      VCallNode:a 0
@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 21, 2013

Pinging @enebo about the parsing question.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Oct 21, 2013

Hmmm, this appears to be an 1.8.7 and lower optimization which our 1.9+ runtimes are also doing. I suspect this will be pretty simple.

@ghost ghost assigned enebo Oct 22, 2013

@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 22, 2013

So is this a parser change or an executable change? It seems like it should be a parser change to make it not collapse [*a] into a single splat node, yeah?

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Oct 22, 2013

This should be an AST construction change in the parser. yeah it should be a literal array wrapping a splat and not just a splat. 1.9 must have undone this optimization. Actually, I will fix this now to get it in 1.7.6.

enebo added a commit that referenced this issue Oct 22, 2013

@enebo enebo closed this in 0ed823a Oct 22, 2013

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Oct 22, 2013

This was not a parser problem. We matched MRI in the tree we generated. In 1.9+ the logic for a splat is to construct a new array.

@gamache

This comment has been minimized.

Copy link
Author

gamache commented Oct 22, 2013

Loved the details here. Thanks for the quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.