Combination of Enumerable#each_slice and Array#transpose raises TypeError #529

Closed
rdvdijk opened this Issue Feb 6, 2013 · 6 comments

Projects

None yet

2 participants

@rdvdijk

I ran into this strange behaviour while slicing and transposing large nested arrays. I've narrowed it down to this small test case:

[[1],[2]].each_slice(1) { |slice| p slice.transpose }

Which gives the following output:

[[1]]
TypeError: can't convert nil into Array
    from org/jruby/RubyArray.java:1030:in `transpose'
    from (irb):1:in `evaluate'
    from org/jruby/RubyArray.java:1653:in `each_slice'
    from (irb):1:in `evaluate'
    from org/jruby/RubyKernel.java:1066:in `eval'
    from org/jruby/RubyKernel.java:1392:in `loop'
    from org/jruby/RubyKernel.java:1174:in `catch'
    from org/jruby/RubyKernel.java:1174:in `catch'
    from /Users/roel/.rvm/rubies/jruby-1.7.2/bin/irb:13:in `(root)'

So, the first slice.transpose works, the second throws the TypeError.

In MRI (1.8, 1.9, head) and Rubinius the example works fine (and prints [[1]] and then [[2]]).

@BanzaiMan
JRuby Team member

Confirmed on master. Looks like the internal structure of the array is inconsistent; not sure yet how things break down.

@rdvdijk

I'm looking into this as well. It seems that the private begin isn't reset properly to 0.

@rdvdijk

I did some digging, what I have found so far is that eachSlice yields "window" copies of the array, right here: https://github.com/jruby/jruby/blob/master/src/org/jruby/RubyArray.java#L1636

The begin variable of the yielded array is set to localBegin += size. In my example above, this sets the begin variable of the second yield to 1.

Later, in the transpose method, elt is called in a for loop, starting with the begin value, which is 1. (Code: https://github.com/jruby/jruby/blob/master/src/org/jruby/RubyArray.java#L1030)

As we can see in the elt method, this returns nil, because the offset (=begin =1) is equal to the realLength. (Code: https://github.com/jruby/jruby/blob/master/src/org/jruby/RubyArray.java#L744-L749)

Then convertToArray() is called on the nil value, which fails.


Now that we know what causes the error, how do we fix this? I'd love to propose a fix for this.. I'll look into it more tomorrow.

@BanzaiMan
JRuby Team member

@rdvdijk I think I have a fix. I can push it now, but if you want, I can certainly wait. 😄

Consider how elt() is defined, how it is called in transpose(), and how they disagree.

@rdvdijk

I would say that in this case elt should never be called with integers larger than 0, since the arrays in this case are always length 1. Maybe the for loop should just start at 0 and end at realLength? But that breaks tests..

Be free to push your fix, sensei! 🙇

@BanzaiMan BanzaiMan added a commit that closed this issue Feb 7, 2013
@BanzaiMan BanzaiMan Fix #529.
Here, elt() needs an offset for the effective array we are considering. This was not a problem since 'begin' is often 0.
6203f30
@BanzaiMan BanzaiMan closed this in 6203f30 Feb 7, 2013
@rdvdijk

Ah! Nice fix 👍

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