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

Enumerable#drop calls #dup on every element #4218

Closed
iconara opened this issue Oct 10, 2016 · 4 comments · Fixed by #4225
Closed

Enumerable#drop calls #dup on every element #4218

iconara opened this issue Oct 10, 2016 · 4 comments · Fixed by #4225

Comments

@iconara
Copy link
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
Copy link
Member

headius commented Oct 11, 2016

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

@iconara
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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.

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

Successfully merging a pull request may close this issue.

2 participants