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#drop calls #dup on every element #4218

Closed
iconara opened this Issue Oct 10, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@iconara
Contributor

iconara commented Oct 10, 2016

Given:

class Stuff
  include Enumerable

  def initialize(stuff)
    @stuff = stuff
  end

  def each(&block)
    @stuff.each(&block)
  end
end

original_stuff = [Object.new, Object.new, Object.new]
enumerable_stuff = Stuff.new(original_stuff)

Expected Behavior

original_stuff[1].equal?(enumerable_stuff.to_a[1]) # => true
original_stuff[1].equal?(enumerable_stuff.drop(1).first) # => true
enumerable_stuff.to_a[1].equal?(enumerable_stuff.drop(1).first) # => true

After dropping an element from an Enumerable I expect that the remaining elements are the same as in the original Enumerable. Calling #drop should not modify the elements.

Actual Behavior

original_stuff[1].equal?(enumerable_stuff.to_a[1]) # => true
original_stuff[1].equal?(enumerable_stuff.drop(1).first) # => false
enumerable_stuff.to_a[1].equal?(enumerable_stuff.drop(1).first) # => false

Calling #drop on an Enumerable calls #dup on every non-dropped element. Besides the side-effect illustrated above it also expensive to do all that copying, and it doesn't work with objects whose #dup doesn't behave well, or objects that can't be #dup'ed.

I discovered this while writing a native extension with a class that was not allocatable. It took me a while to understand why my code failed with errors saying that my class wasn't allocatable – I wasn't trying to create instances.

It looks like this behaviour was introduced in 06f0441, but it seems like the fix for the problem described in the commit message was applied in the wrong place (it ended up in Enumerable instead of Enumerator).

Environment

  • jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 Java HotSpot(TM) 64-Bit Server VM 25.0-b70 on 1.8.0-b132 +jit [darwin-x86_64]
  • jruby 1.7.25 (1.9.3p551) 2016-04-13 867cb81 on Java HotSpot(TM) 64-Bit Server VM 1.8.0-b132 +jit [darwin-x86_64]
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 11, 2016

Member

Nice find, great homework. Thanks! Maybe you know enough to attempt a PR?

Member

headius commented Oct 11, 2016

Nice find, great homework. Thanks! Maybe you know enough to attempt a PR?

@iconara

This comment has been minimized.

Show comment
Hide comment
@iconara

iconara Oct 11, 2016

Contributor

I'd like to, but I'm not sure I could do it without creating more bugs. I don't really understand the reasoning behind why it works the way it works now (for example: why only #drop, why wouldn't it be a problem with #take too?). I can make an attempt, but I can't promise that it will happen quickly.

Contributor

iconara commented Oct 11, 2016

I'd like to, but I'm not sure I could do it without creating more bugs. I don't really understand the reasoning behind why it works the way it works now (for example: why only #drop, why wouldn't it be a problem with #take too?). I can make an attempt, but I can't promise that it will happen quickly.

@iconara

This comment has been minimized.

Show comment
Hide comment
@iconara

iconara Oct 13, 2016

Contributor

The fix seems to be to change Signature.NO_ARGUMENTS to Signature.ONE_REQUIRED (in 9K, Arity.ON_REQUIRED in 1.7), which I don't understand the mechanics behind. I've opened two PRs with fixes #4225 & #4226.

Contributor

iconara commented Oct 13, 2016

The fix seems to be to change Signature.NO_ARGUMENTS to Signature.ONE_REQUIRED (in 9K, Arity.ON_REQUIRED in 1.7), which I don't understand the mechanics behind. I've opened two PRs with fixes #4225 & #4226.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 18, 2016

Member

I'm going to restart the travis build and see if it improves. Otherwise we'll need to figure out what's not right here.

1.7 also started failing after I merged that PR but it was not clear that it had anything to do with the changes.

Member

headius commented Oct 18, 2016

I'm going to restart the travis build and see if it improves. Otherwise we'll need to figure out what's not right here.

1.7 also started failing after I merged that PR but it was not clear that it had anything to do with the changes.

@headius headius closed this in #4225 Nov 8, 2016

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