Usage of `#[]=` assignment in method argument broken #4323

Closed
gettalong opened this Issue Nov 20, 2016 · 3 comments

Projects

None yet

2 participants

@gettalong

Environment

  • jruby 9.1.6.0-SNAPSHOT (2.3.1) 2016-11-08 cb5a7be Java HotSpot(TM) 64-Bit Server VM 25.91-b14 on 1.8.0_91-b14 +jit [linux-x86_64]
  • Linux noweto 4.4.0-25-generic #44-Ubuntu SMP Fri Jun 10 18:19:48 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

Expected Behavior

The following worked at least until JRuby 9.0.0.0 and works in MRI:

def modify(x)
  x << 'a'
end

modify(u = '')     # => "a"
u                  # => "a"
x = {}             # => {}
modify(x[5] = '')  # => "a"
x                  # => {5=>"a"}

Actual Behavior

At least from JRuby 9.1.5.0 onwards including the above listed JRuby 9.1.6.0 snapshot the following happens:

def modify(x)
  x << 'a'
end

modify(u = '')     # => "a"
u                  # => "a"
x = {}             # => {}
modify(x[5] = '')  # => "a"
x                  # => {5=>""}

The difference is in the last line where the string assigned to x[5] isn't modified anymore, leading to erroneous behaviour.

@enebo enebo added this to the JRuby 9.1.7.0 milestone Nov 21, 2016
@enebo enebo added the ir label Nov 21, 2016
@enebo
Member
enebo commented Nov 21, 2016

This is a weird bug and one I think I can confidently say is an IR one. If we move the assignment out of the method call it works and as shown using an ordinary string assignment works. Our hash assignment when an arg call must be dup'ing the value somewhere so we << on a copy.

@enebo enebo added a commit that closed this issue Nov 21, 2016
@enebo enebo Fixes #4323. Usage of `#[]=` assignment in method argument broken.
I had introduced a bad optimization during IR build phase.  It assumed
all Strings could be built out of order but I did not account for the
fact that the same StrNode could be referenced by two instructions at
the same time (since each occurrence will perform a dup).

Note: It might look like I removed FrozenStrings from building out of
order but the ordinary logic in buildWithOrder will end up creating
the operand in place and not generating the copy since it is an
ImmutableLiteral.
89759e4
@enebo enebo closed this in 89759e4 Nov 21, 2016
@enebo
Member
enebo commented Nov 21, 2016

Yeah commit message explains it in more detail but it was indded IR performing a dup on the string which it shouldn't been doing.

@gettalong

@enebo Thanks for the fast fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment