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

Enumerable#each_entry behaves differently from MRI #4532

Closed
toy opened this Issue Mar 15, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@toy

toy commented Mar 15, 2017

Enumerable#each_entry is supposed to convert multiple arguments to an array, but the behaviour is different from MRI and jruby 1.7 when multiple arguments are yielded.

jruby -v:
jruby 9.0.5.0 (2.2.3) 2016-01-26 7bee00d Java HotSpot(TM) 64-Bit Server VM 24.76-b04 on 1.7.0_76-b13 +jit [linux-amd64]
jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 Java HotSpot(TM) 64-Bit Server VM 24.76-b04 on 1.7.0_76-b13 +jit [linux-x86_64]

test file:

class Foo
  include Enumerable

  def each
    yield 1
    yield 1, 2
    yield
  end
end

out = []
Foo.new.each_entry{ |*args| out << args }
puts "each_entry: #{out.inspect}"

Expected output:

==[1.9.3-p551]==
each_entry: [[1], [[1, 2]], [nil]]

==[2.0.0-p648]==
each_entry: [[1], [[1, 2]], [nil]]

==[2.1.10]==
each_entry: [[1], [[1, 2]], [nil]]

==[2.2.6]==
each_entry: [[1], [[1, 2]], [nil]]

==[2.3.3]==
each_entry: [[1], [[1, 2]], [nil]]

==[2.4.0]==
each_entry: [[1], [[1, 2]], [nil]]

==[jruby-1.7.26]==
each_entry: [[1], [[1, 2]], [nil]]

jruby 9.x output:

==[jruby-9.0.5.0]==
each_entry: [[1], [1, 2], [nil]]

==[jruby-9.1.7.0]==
each_entry: [[1], [1, 2], [nil]]

toy added a commit to toy/in_threads that referenced this issue Mar 16, 2017

toy added a commit to toy/in_threads that referenced this issue Mar 16, 2017

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 16, 2017

Member

confirmed on latest 9K (9.1.8.0) as well: each_entry: [[1], [1, 2], [nil]]

Member

kares commented Mar 16, 2017

confirmed on latest 9K (9.1.8.0) as well: each_entry: [[1], [1, 2], [nil]]

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 16, 2017

Member

I pushed a possible fix. Feels like we are endlessly tweaking this logic back and forth.

I did try to do a Ruby implementation, and I don't think it's possible without extra logic to tell the kind of arguments passed in from each. If I implement each_entry as Ruby code, it behaves like JRuby.

~/projects/jruby $ cat blah.rb
class Foo
  include Enumerable

  def each
    yield 1
    yield 1, 2
    yield
  end

  def each_entry
    each {|*a| yield(*a)}
  end
end

out = []
Foo.new.each_entry{ |*args| out << args }
puts "each_entry: #{out.inspect}"

~/projects/jruby $ jruby blah.rb
each_entry: [[1], [1, 2], []]
Member

headius commented Mar 16, 2017

I pushed a possible fix. Feels like we are endlessly tweaking this logic back and forth.

I did try to do a Ruby implementation, and I don't think it's possible without extra logic to tell the kind of arguments passed in from each. If I implement each_entry as Ruby code, it behaves like JRuby.

~/projects/jruby $ cat blah.rb
class Foo
  include Enumerable

  def each
    yield 1
    yield 1, 2
    yield
  end

  def each_entry
    each {|*a| yield(*a)}
  end
end

out = []
Foo.new.each_entry{ |*args| out << args }
puts "each_entry: #{out.inspect}"

~/projects/jruby $ jruby blah.rb
each_entry: [[1], [1, 2], []]
@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 17, 2017

Member

@headius I commented on the PR but we seemingly changed all other yield calls in RubyEnumerable to yield from yieldSpecific already. This is the last one in that file. Glancing at other yieldSpecific in other files and they mostly seem ok since we know the argument we are passing internally.

Member

enebo commented Mar 17, 2017

@headius I commented on the PR but we seemingly changed all other yield calls in RubyEnumerable to yield from yieldSpecific already. This is the last one in that file. Glancing at other yieldSpecific in other files and they mostly seem ok since we know the argument we are passing internally.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 17, 2017

Member

@enebo Ok, might as well go with it then.

Member

headius commented Mar 17, 2017

@enebo Ok, might as well go with it then.

@toy

This comment has been minimized.

Show comment
Hide comment
@toy

toy Mar 18, 2017

@headius each_entry is always yielding one argument, so 1 argument is passed as is, 2 or more arguments are converted to array and 0 arguments will yield nil. So your ruby implementation should be like:

def each_entry
  each {|*a| yield(a.size > 1 ? a : a[0])}
end

toy commented Mar 18, 2017

@headius each_entry is always yielding one argument, so 1 argument is passed as is, 2 or more arguments are converted to array and 0 arguments will yield nil. So your ruby implementation should be like:

def each_entry
  each {|*a| yield(a.size > 1 ? a : a[0])}
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment