Enumerator#count returns nil #922

Closed
agrimm opened this Issue Jul 27, 2013 · 6 comments

Projects

None yet

5 participants

@agrimm
Contributor
agrimm commented Jul 27, 2013

In MRI 2.0, [].each.count returns 0.

By contrast, in JRuby under 2.0 mode, [].each.count returns nil.

1.9 and 1.8 modes are not affected.

JRuby version 1.7.3 is not affected, but JRuby version 1.7.4 is affected.

@komax
Member
komax commented Jul 27, 2013

On latest jruby master I can reproduce this as well:

% jruby --2.0 -e 'f = [].each.count; puts f.nil?'
true
@atambo
Member
atambo commented Jul 30, 2013

@dmarcotte, do your Enumerable fixes cover this as well?

@dmarcotte
Contributor

Unfortunately no. I've been looking at this, and so far it looks pretty odd... it seems like for some reason Enumerable#count isn't called at all for RubyArray#each in 2.0 mode (Edit: oops. It's totally called. This was the old breakpoint-in-the-wrong-place red herring). This behavior even manifests for non-empty arrays:

% jruby --2.0 -e 'f = [1,2,3].each.count; puts f.nil?'
true

I'll poke at this some more and let you know if I get any insight.

@dmarcotte
Contributor

Got it:

In RubyEnumerable#count(ThreadContext context, IRubyObject self, final Block block), for blockless calls it invokes #size on its object if it exists. With the new 2.0 Enumerator#size method, it now exists for Enumerators, but when we enumeratorize the Array in RubyArray#each we don't initialize the size, so it returns nil for the count.

Seems like we need to port rb_enumeratorize_with_size and apply it in the right places to make sure these no wonkiness like this in the future.

I can put together a pull for this, but it will likely be a little while before I can send it. That work? Or does someone else want to grab it sooner?

@dmarcotte
Contributor

Finally scratched out some time to have a look at this, and good news: there turned out to be a relatively small fix. See #979.

The enumeratorize_with_size issue I mention above does still exist though. Opened #980 to track it.

@headius headius closed this in #979 Aug 31, 2013
@headius
Member
headius commented Aug 31, 2013

Merged the referenced PR.

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