[GR-34365] Handle Kernel#clone with freeze: true argument (#2512)

PullRequest: truffleruby/3031

CHANGELOG.md

@@ -47,6 +47,7 @@ Compatibility:
 * Update `MatchData` methods to return `String` instances when called on a subclass (#2453, @bjfish).
 * Implement `Proc#{==,eql?}` (#2453).
 * Implement all `StringScanner` methods (#2520, @eregon).
+* Handle `Kernel#clone(freeze: true)` (#2512, @andrykonchin).
 
 Performance:
 

spec/ruby/core/kernel/clone_spec.rb

@@ -31,10 +31,10 @@ def klass.allocate
   describe "with no arguments" do
     it "copies frozen state from the original" do
       o2 = @obj.clone
+      o2.should_not.frozen?
+
       @obj.freeze
       o3 = @obj.clone
-
-      o2.should_not.frozen?
       o3.should.frozen?
     end
 
@@ -44,6 +44,30 @@ def klass.allocate
     end
   end
 
+  describe "with freeze: nil" do
+    ruby_version_is ""..."3.0" do
+      it "raises ArgumentError" do
+        -> { @obj.clone(freeze: nil) }.should raise_error(ArgumentError, /unexpected value for freeze: NilClass/)
+      end
+    end
+
+    ruby_version_is "3.0" do
+      it "copies frozen state from the original, like #clone without arguments" do
+        o2 = @obj.clone(freeze: nil)
+        o2.should_not.frozen?
+
+        @obj.freeze
+        o3 = @obj.clone(freeze: nil)
+        o3.should.frozen?
+      end
+
+      it "copies frozen?" do
+        o = "".freeze.clone(freeze: nil)
+        o.frozen?.should be_true
+      end
+    end
+  end
+
   describe "with freeze: true" do
     it 'makes a frozen copy if the original is frozen' do
       @obj.freeze
@@ -112,6 +136,14 @@ def klass.allocate
     end
   end
 
+  describe "with freeze: anything else" do
+    it 'raises ArgumentError when passed not true/false/nil' do
+      -> { @obj.clone(freeze: 1) }.should raise_error(ArgumentError, /unexpected value for freeze: Integer/)
+      -> { @obj.clone(freeze: "") }.should raise_error(ArgumentError, /unexpected value for freeze: String/)
+      -> { @obj.clone(freeze: Object.new) }.should raise_error(ArgumentError, /unexpected value for freeze: Object/)
+    end
+  end
+
   it "copies instance variables" do
     clone = @obj.clone
     clone.one.should == 1

spec/ruby/core/kernel/fixtures/classes.rb

@@ -281,9 +281,14 @@ def initialize(one, two)
       @two = two
     end
 
-    def initialize_copy(other)
+    def initialize_copy(other, **kw)
       ScratchPad.record object_id
     end
+
+    # define to support calling #clone with optional :freeze keyword argument
+    def initialize_clone(other, **kw)
+      super(other) # to call #initialize_copy
+    end
   end
 
   class Clone

spec/ruby/core/kernel/initialize_clone_spec.rb

@@ -0,0 +1,28 @@
+require_relative '../../spec_helper'
+
+describe "Kernel#initialize_clone" do
+  it "is a private instance method" do
+    Kernel.should have_private_instance_method(:initialize_clone)
+  end
+
+  it "returns the receiver" do
+    a = Object.new
+    b = Object.new
+    a.send(:initialize_clone, b).should == a
+  end
+
+  it "calls #initialize_copy" do
+    a = Object.new
+    b = Object.new
+    a.should_receive(:initialize_copy).with(b)
+    a.send(:initialize_clone, b)
+  end
+
+  ruby_version_is "3.0" do
+    it "accepts a :freeze keyword argument for obj.clone(freeze: value)" do
+      a = Object.new
+      b = Object.new
+      a.send(:initialize_clone, b, freeze: true).should == a
+    end
+  end
+end

spec/ruby/core/kernel/initialize_dup_spec.rb

@@ -0,0 +1,20 @@
+require_relative '../../spec_helper'
+
+describe "Kernel#initialize_dup" do
+  it "is a private instance method" do
+    Kernel.should have_private_instance_method(:initialize_dup)
+  end
+
+  it "returns the receiver" do
+    a = Object.new
+    b = Object.new
+    a.send(:initialize_dup, b).should == a
+  end
+
+  it "calls #initialize_copy" do
+    a = Object.new
+    b = Object.new
+    a.should_receive(:initialize_copy).with(b)
+    a.send(:initialize_dup, b)
+  end
+end

spec/ruby/core/numeric/clone_spec.rb

@@ -22,4 +22,11 @@
   it "raises ArgumentError if passed freeze: false" do
     -> { 1.clone(freeze: false) }.should raise_error(ArgumentError, /can't unfreeze/)
   end
+
+  ruby_version_is "3.0" do
+    it "does not change frozen status if passed freeze: nil" do
+      value = 1
+      value.clone(freeze: nil).should equal(value)
+    end
+  end
 end

src/main/java/org/truffleruby/core/kernel/KernelNodes.java

@@ -55,6 +55,9 @@
 import org.truffleruby.core.format.exceptions.InvalidFormatException;
 import org.truffleruby.core.format.printf.PrintfCompiler;
 import org.truffleruby.core.hash.HashOperations;
+import org.truffleruby.core.hash.HashingNodes;
+import org.truffleruby.core.hash.RubyHash;
+import org.truffleruby.core.hash.library.PackedHashStoreLibrary;
 import org.truffleruby.core.inlined.AlwaysInlinedMethodNode;
 import org.truffleruby.core.inlined.InlinedDispatchNode;
 import org.truffleruby.core.inlined.InlinedMethodNode;
@@ -552,21 +555,16 @@ private DispatchingNode allocateNode() {
     @NodeChild(value = "freeze", type = RubyBaseNodeWithExecute.class)
     public abstract static class CloneNode extends PrimitiveNode {
 
-        @Child private CopyNode copyNode = CopyNode.create();
-        @Child private DispatchNode initializeCloneNode = DispatchNode.create();
         @Child private SingletonClassNode singletonClassNode;
-
-        @CreateCast("freeze")
-        protected RubyBaseNodeWithExecute coerceToBoolean(RubyBaseNodeWithExecute freeze) {
-            return BooleanCastWithDefaultNode.create(true, freeze);
-        }
+        private final BranchProfile cantUnfreezeErrorProfile = BranchProfile.create();
 
         @Specialization(limit = "getRubyLibraryCacheLimit()")
-        protected RubyDynamicObject clone(RubyDynamicObject object, boolean freeze,
+        protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze,
+                @Cached CopyNode copyNode,
+                @Cached DispatchNode initializeCloneNode,
                 @Cached ConditionProfile isSingletonProfile,
-                @Cached ConditionProfile freezeProfile,
-                @Cached ConditionProfile isFrozenProfile,
                 @Cached ConditionProfile isRubyClass,
+                @Cached HashingNodes.ToHashByHashCode hashNode,
                 @CachedLibrary("object") RubyLibrary rubyLibrary,
                 @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryFreeze) {
             final RubyDynamicObject newObject = copyNode.executeCopy(object);
@@ -578,9 +576,18 @@ protected RubyDynamicObject clone(RubyDynamicObject object, boolean freeze,
                 newObjectMetaClass.fields.initCopy(selfMetaClass);
             }
 
-            initializeCloneNode.call(newObject, "initialize_clone", object);
+            final boolean copyFrozen = freeze instanceof Nil;
 
-            if (freezeProfile.profile(freeze) && isFrozenProfile.profile(rubyLibrary.isFrozen(object))) {
+            if (copyFrozen) {
+                initializeCloneNode.call(newObject, "initialize_clone", object);
+            } else {
+                // pass :freeze keyword argument to #initialize_clone
+                final RubyHash keywordArguments = createFreezeBooleanHash((boolean) freeze, hashNode);
+                initializeCloneNode.call(newObject, "initialize_clone", object, keywordArguments);
+            }
+
+            // Default behavior - is just to copy the frozen state of the original object
+            if (forceFrozen(freeze) || (copyFrozen && rubyLibrary.isFrozen(object))) {
                 rubyLibraryFreeze.freeze(newObject);
             }
 
@@ -592,64 +599,78 @@ protected RubyDynamicObject clone(RubyDynamicObject object, boolean freeze,
         }
 
         @Specialization
-        protected Object cloneBoolean(boolean object, boolean freeze,
-                @Cached ConditionProfile freezeProfile) {
-            if (freezeProfile.profile(!freeze)) {
+        protected boolean cloneBoolean(boolean object, Object freeze) {
+            if (forceNotFrozen(freeze)) {
                 raiseCantUnfreezeError(object);
             }
             return object;
         }
 
         @Specialization
-        protected Object cloneInteger(int object, boolean freeze,
-                @Cached ConditionProfile freezeProfile) {
-            if (freezeProfile.profile(!freeze)) {
+        protected int cloneInteger(int object, Object freeze) {
+            if (forceNotFrozen(freeze)) {
                 raiseCantUnfreezeError(object);
             }
             return object;
         }
 
         @Specialization
-        protected Object cloneLong(long object, boolean freeze,
-                @Cached ConditionProfile freezeProfile) {
-            if (freezeProfile.profile(!freeze)) {
+        protected long cloneLong(long object, Object freeze) {
+            if (forceNotFrozen(freeze)) {
                 raiseCantUnfreezeError(object);
             }
             return object;
         }
 
         @Specialization
-        protected Object cloneFloat(double object, boolean freeze,
-                @Cached ConditionProfile freezeProfile) {
-            if (freezeProfile.profile(!freeze)) {
+        protected double cloneFloat(double object, Object freeze) {
+            if (forceNotFrozen(freeze)) {
                 raiseCantUnfreezeError(object);
             }
             return object;
         }
 
         @Specialization(guards = "!isImmutableRubyString(object)")
-        protected Object cloneImmutableObject(ImmutableRubyObject object, boolean freeze,
-                @Cached ConditionProfile freezeProfile) {
-            if (freezeProfile.profile(!freeze)) {
+        protected ImmutableRubyObject cloneImmutableObject(ImmutableRubyObject object, Object freeze) {
+            if (forceNotFrozen(freeze)) {
                 raiseCantUnfreezeError(object);
             }
             return object;
         }
 
         @Specialization
-        protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, boolean freeze,
-                @Cached ConditionProfile freezeProfile,
+        protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, Object freeze,
                 @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryFreeze,
                 @Cached MakeStringNode makeStringNode) {
             final RubyDynamicObject newObject = makeStringNode.fromRope(object.rope, object.encoding);
-            if (freezeProfile.profile(freeze)) {
+            if (!forceNotFrozen(freeze)) {
                 rubyLibraryFreeze.freeze(newObject);
             }
 
             return newObject;
         }
 
+        private RubyHash createFreezeBooleanHash(boolean freeze, HashingNodes.ToHashByHashCode hashNode) {
+            final RubySymbol key = coreSymbols().FREEZE;
+
+            final Object[] newStore = PackedHashStoreLibrary.createStore();
+            final int hashed = hashNode.execute(key);
+            PackedHashStoreLibrary.setHashedKeyValue(newStore, 0, hashed, key, freeze);
+
+            return new RubyHash(coreLibrary().hashClass, getLanguage().hashShape, getContext(), newStore, 1);
+        }
+
+        private boolean forceFrozen(Object freeze) {
+            return freeze instanceof Boolean && (boolean) freeze;
+
+        }
+
+        private boolean forceNotFrozen(Object freeze) {
+            return freeze instanceof Boolean && !(boolean) freeze;
+        }
+
         private void raiseCantUnfreezeError(Object object) {
+            cantUnfreezeErrorProfile.enter();
             throw new RaiseException(getContext(), coreExceptions().argumentErrorCantUnfreeze(object, this));
         }
 
@@ -708,7 +729,7 @@ protected DispatchingNode initializeDupNode() {
                 initializeDupNode = insert(
                         new InlinedDispatchNode(
                                 getLanguage(),
-                                InitializeDupCloneNode.create()));
+                                InitializeDupNode.create()));
             }
             return initializeDupNode;
         }
@@ -1104,11 +1125,11 @@ public InternalMethod getMethod() {
         }
     }
 
-    @CoreMethod(names = { "initialize_dup", "initialize_clone" }, required = 1)
-    public abstract static class InitializeDupCloneNode extends InlinedMethodNode {
+    @CoreMethod(names = "initialize_dup", required = 1)
+    public abstract static class InitializeDupNode extends InlinedMethodNode {
 
-        public static InitializeDupCloneNode create() {
-            return KernelNodesFactory.InitializeDupCloneNodeFactory.create(null);
+        public static InitializeDupNode create() {
+            return KernelNodesFactory.InitializeDupNodeFactory.create(null);
         }
 
         @Child private DispatchingNode initializeCopyNode;

src/main/java/org/truffleruby/core/support/TypeNodes.java

@@ -175,6 +175,24 @@ protected boolean isNil(Object value) {
         }
     }
 
+    @Primitive(name = "boolean_or_nil?")
+    public abstract static class IsBooleanOrNilNode extends PrimitiveArrayArgumentsNode {
+        @Specialization
+        protected boolean bool(boolean value) {
+            return true;
+        }
+
+        @Specialization
+        protected boolean nil(Nil value) {
+            return true;
+        }
+
+        @Fallback
+        protected boolean other(Object value) {
+            return false;
+        }
+    }
+
     @Primitive(name = "object_ivars")
     public abstract static class ObjectInstanceVariablesNode extends PrimitiveArrayArgumentsNode {
 

src/main/ruby/truffleruby/core/kernel.rb

@@ -749,10 +749,18 @@ def fork
   Primitive.method_unimplement method(:fork)
   Primitive.method_unimplement nil.method(:fork)
 
-  def clone(freeze: true)
+  def clone(freeze: nil)
+    unless Primitive.boolean_or_nil?(freeze)
+      raise ArgumentError, "unexpected value for freeze: #{freeze.class}"
+    end
+
     Primitive.object_clone self, freeze
   end
 
+  private def initialize_clone(from, freeze: nil)
+    initialize_copy(from)
+  end
+
   Truffle::Boot.delay do
     if Truffle::Boot.get_option('gets-loop')
       def chomp(separator=$/)

src/main/ruby/truffleruby/core/numeric.rb

@@ -29,8 +29,8 @@
 class Numeric
   include Comparable
 
-  def clone(freeze: true)
-    unless freeze
+  def clone(freeze: nil)
+    if Primitive.object_equal(freeze, false)
       raise ArgumentError, "can't unfreeze #{self.class.name}"
     end
     self

test/mri/excludes/TestNumeric.rb

@@ -1,4 +1,3 @@
-exclude :test_clone, "needs investigation"
 exclude :test_coerce, "needs investigation"
 exclude :test_coerced_remainder, "needs investigation"
 exclude :test_dummynumeric, "needs investigation"