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

Avoid duping the String class when deep-duping #20640

Closed
wants to merge 1 commit into from

Conversation

rnubel
Copy link
Contributor

@rnubel rnubel commented Jun 20, 2015

On Rubinius, deep_dup'ing any structure containing the String class will explode with a TypeError; e.g.,

irb(main):004:0> { :foo => String }.deep_dup
TypeError: Unable to clone/dup String class
        from kernel/common/string.rb:39:in `dup (clone)'
        from /home/robert/Sandbox/ruby/rails/activesupport/lib/active_support/core_ext/object/deep_dup.rb:14:in `deep_dup'
        from /home/robert/Sandbox/ruby/rails/activesupport/lib/active_support/core_ext/object/deep_dup.rb:44:in `deep_dup'
        from kernel/common/enumerable.rb:83:in `each_with_object'
        from kernel/common/hash.rb:342:in `each'
        from kernel/common/enumerable.rb:81:in `each_with_object'
        from /home/robert/Sandbox/ruby/rails/activesupport/lib/active_support/core_ext/object/deep_dup.rb:42:in `deep_dup'
        from (irb):4

The cause of this is that String.dup cannot work in Rubinius, due to this code. Per a discussion with Rubinius' maintainer, that behavior is unlikely to change.

This patch fixes the issue by simply not dup-ing the String class (which could be made Rubinius-specific, if desired). To be honest, I'd rather we re-visit #20008, since dup-ing classes seems pretty pointless and produces unexpected results:

irb(main):004:0> NewString = String.dup
=> NewString
irb(main):005:0> v = NewString.new("test")
=> "test"
irb(main):006:0> v.is_a?(String)
=> false

Anyway, I've proposed this same change just for Grape in intridea/grape#1038, but it seemed like the type of thing to push upstream. Feedback welcome!

On Rubinius, attempting to call `String.dup` will raise a TypeError.
This has been its behavior for over five years, and was a special
case added just for String. Personally, I think duping classes is
ill-advised, as the resulting class produces instances which can't
be sanely type-checked, but this fix is the minimal requirement to
fix the bug.
@rnubel rnubel force-pushed the fix_rbx_deep_dup_of_string_class branch from f6d2049 to 1cd60b5 Compare June 20, 2015 03:36
@rnubel rnubel closed this Jun 20, 2015
@rnubel rnubel reopened this Jun 20, 2015
@arthurnn
Copy link
Member

arthurnn commented Oct 1, 2015

I wonder why that code is there in the first place (in Rubinius). I understand that dupping a base class like String could cause problems, but TBH, there are LOTS of other things can cause problems in ruby environment.
reading @brixen 's comment , seems like he is not sure why this was added to Rubinius too.

My opinion on this, is that, I dont wanna add a code into Rails that we are not sure WHY was there (in Rubinius), and we would not be sure why it is in Rails itself.

Thanks, though, for the PR, and good job investigating the problem

@arthurnn arthurnn closed this Oct 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants