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

kwargs in Array.each mutates argument #5788

Closed
amacleay opened this issue Jul 10, 2019 · 4 comments
Closed

kwargs in Array.each mutates argument #5788

amacleay opened this issue Jul 10, 2019 · 4 comments
Milestone

Comments

@amacleay
Copy link

@amacleay amacleay commented Jul 10, 2019

Environment

From https://oss.sonatype.org/content/repositories/snapshots/org/jruby/jruby-dist/9000.dev-SNAPSHOT/jruby-dist-9000.dev-20140908.223539-1-bin.tar.gz

$ bash jruby-9000.dev-SNAPSHOT/bin/jruby --version
jruby 9000.dev-SNAPSHOT (2.1.2p142) 2014-09-04 177ed02 Java HotSpot(TM) 64-Bit Server VM 25.181-b13 on 1.8.0_181-b13 [linux-amd64]
09:56:01 ~/repro
$ uname -a
Linux amacleay-cht-tp 4.15.0-54-generic #58-Ubuntu SMP Mon Jun 24 10:55:24 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Script:

# $ cat repro.rb 
list = Array.new(200, { a: 1, b: 2, c: 3 })
list.each do |a:, b:, **|
  puts "#{a} #{b}"
end

Expected Behavior

It should print 1 2 200 times and exit happily, as does MRI:

$ rvm ruby-2.5.0 do ruby repro.rb | nl | tail -n1
   200  1 2

Actual Behavior

Runs once and then fails. Further investigation shows that the runtime is mutating the invocant to .each

$ bash jruby-9000.dev-SNAPSHOT/bin/jruby repro.rb 
1 2
ArgumentError: missing keyword: a
  (root) at repro.rb:2
    each at org/jruby/RubyArray.java:1546
  (root) at repro.rb:2

More recent releases run a variable number of iterations and then fail:
9.2.5.0:

$ rvm jruby-9.2.5.0 do ruby repro.rb | nl | tail -n1
ArgumentError: missing keyword: a
  <main> at repro.rb:2
    each at org/jruby/RubyArray.java:1792
  <main> at repro.rb:2
   177  1 2
10:03:08 ~/repro
$ rvm jruby-9.2.5.0 do ruby repro.rb | nl | tail -n1
ArgumentError: missing keyword: a
  <main> at repro.rb:2
    each at org/jruby/RubyArray.java:1792
  <main> at repro.rb:2
   143  1 2
10:03:13 ~/repro
$ rvm jruby-9.2.5.0 do ruby repro.rb | nl | tail -n1
ArgumentError: missing keyword: a
  <main> at repro.rb:2
    each at org/jruby/RubyArray.java:1792
  <main> at repro.rb:2
   142  1 2

9.2.7.0:

$ rvm jruby-9.2.7.0 do ruby repro.rb | nl | tail -n1
ArgumentError: missing keyword: a
  <main> at repro.rb:2
    each at org/jruby/RubyArray.java:1792
  <main> at repro.rb:2
   151  1 2
10:03:38 ~/repro
$ rvm jruby-9.2.7.0 do ruby repro.rb | nl | tail -n1
ArgumentError: missing keyword: a
  <main> at repro.rb:2
    each at org/jruby/RubyArray.java:1792
  <main> at repro.rb:2
   193  1 2
10:03:42 ~/repro
$ rvm jruby-9.2.7.0 do ruby repro.rb | nl | tail -n1
ArgumentError: missing keyword: a
  <main> at repro.rb:2
    each at org/jruby/RubyArray.java:1792
  <main> at repro.rb:2
   171  1 2
@enebo
Copy link
Member

@enebo enebo commented Jul 10, 2019

Just a quick note on this. It appears to be an issue with how kwargs work once we make it to a full build. If we disable full builds it works as expected:

jruby -Xjit.threshold=-1 --dev ../snippets/kwargs3.rb  | wc -l
200

I would predict the helper we use changes and it is not performing a dup somewhere.

@headius
Copy link
Member

@headius headius commented Jul 12, 2019

I did remove some dup'ing several months back, to reduce the cost of kwargs. Perhaps I removed too much.

@amacleay
Copy link
Author

@amacleay amacleay commented Jul 15, 2019

@enebo I can confirm that -Xjit.threshold=-1 --dev seems to solve the issue for 9.2.5.0 and 9.2.7.0, but I get the same bad behavior with the snapshot I pulled down.

@enebo enebo added this to the JRuby 9.2.8.0 milestone Jul 29, 2019
@enebo enebo closed this in 200430d Jul 29, 2019
@enebo
Copy link
Member

@enebo enebo commented Jul 29, 2019

There were two separate things at play and we shall see what impact there is here. fwiw, we never passed this in the JIT unless it was a method and not a block receiving the kwargs. So this exposed what I am guessing is a fairly uncommon combination? In any case it works now. Second, the full interpreter was not dup'ing the hash if we applied call protocol instructions and I honestly don't know why that would break anything. Combine this with the fact that the JIT does not have this check at all and I am guessing it probably does not matter.

As an aside I would like to add a prelude instr which is what frobnicate keywords does today but it will return something which can be a handle to processing keyword arguments without neccesarily performing any dupe of the incoming hash OR use a destructive hash for figuring out if all required kwargs were passed in. In cases where we know we need not do kwargs processing (like we realize we are passing in a literal hash to the call; then we will just noop the instr and replace it will some null object pattern for the operand we pass to the kwargs recv instrs.

enebo added a commit that referenced this issue Jul 29, 2019
…gs. wallpaper since upcoming kwargs work will be happening somewhat soon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants