From 5c9fcd65a0df32f46c80e82909e5b11b2184c9d2 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 20 Oct 2021 02:15:39 +0300 Subject: [PATCH 01/14] Handle Kernel#clone with `freeze: true` argument --- .../truffleruby/core/kernel/KernelNodes.java | 66 +++++++++++++------ src/main/ruby/truffleruby/core/kernel.rb | 2 +- src/main/ruby/truffleruby/core/numeric.rb | 4 +- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index 3044f7c981e4..5508f91668f6 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -556,13 +556,8 @@ public abstract static class CloneNode extends PrimitiveNode { @Child private DispatchNode initializeCloneNode = DispatchNode.create(); @Child private SingletonClassNode singletonClassNode; - @CreateCast("freeze") - protected RubyBaseNodeWithExecute coerceToBoolean(RubyBaseNodeWithExecute freeze) { - return BooleanCastWithDefaultNode.create(true, freeze); - } - @Specialization(limit = "getRubyLibraryCacheLimit()") - protected RubyDynamicObject clone(RubyDynamicObject object, boolean freeze, + protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, @Cached ConditionProfile isSingletonProfile, @Cached ConditionProfile freezeProfile, @Cached ConditionProfile isFrozenProfile, @@ -580,7 +575,8 @@ protected RubyDynamicObject clone(RubyDynamicObject object, boolean freeze, initializeCloneNode.call(newObject, "initialize_clone", object); - if (freezeProfile.profile(freeze) && isFrozenProfile.profile(rubyLibrary.isFrozen(object))) { + // Default behavior - is just to copy the frozen state of the original object + if (toForceFreezing(freeze) || (toNotForceChangingFreezeState(freeze) && rubyLibrary.isFrozen(object))) { rubyLibraryFreeze.freeze(newObject); } @@ -592,63 +588,95 @@ protected RubyDynamicObject clone(RubyDynamicObject object, boolean freeze, } @Specialization - protected Object cloneBoolean(boolean object, boolean freeze, + protected Object cloneBoolean(boolean object, Object freeze, @Cached ConditionProfile freezeProfile) { - if (freezeProfile.profile(!freeze)) { + if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization - protected Object cloneInteger(int object, boolean freeze, + protected Object cloneInteger(int object, Object freeze, @Cached ConditionProfile freezeProfile) { - if (freezeProfile.profile(!freeze)) { + if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization - protected Object cloneLong(long object, boolean freeze, + protected Object cloneLong(long object, Object freeze, @Cached ConditionProfile freezeProfile) { - if (freezeProfile.profile(!freeze)) { + if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization - protected Object cloneFloat(double object, boolean freeze, + protected Object cloneFloat(double object, Object freeze, @Cached ConditionProfile freezeProfile) { - if (freezeProfile.profile(!freeze)) { + if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization(guards = "!isImmutableRubyString(object)") - protected Object cloneImmutableObject(ImmutableRubyObject object, boolean freeze, + protected Object cloneImmutableObject(ImmutableRubyObject object, Object freeze, @Cached ConditionProfile freezeProfile) { - if (freezeProfile.profile(!freeze)) { + if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization - protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, boolean freeze, + protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, Object freeze, @Cached ConditionProfile freezeProfile, @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryFreeze, @Cached MakeStringNode makeStringNode) { final RubyDynamicObject newObject = makeStringNode.fromRope(object.rope, object.encoding); - if (freezeProfile.profile(freeze)) { + if (!toForceUnfreezing(freeze)) { rubyLibraryFreeze.freeze(newObject); } return newObject; } + private boolean toForceFreezing(Object freeze) { + if (freeze instanceof Nil) { + return false; + } + + if (freeze instanceof Boolean) { + return (boolean) freeze; + } + + throw new RaiseException(getContext(), coreExceptions().argumentError( + "Kernel#clone expects :freeze to be boolean or nil", + this)); + } + + private boolean toForceUnfreezing(Object freeze) { + if (freeze instanceof Nil) { + return false; + } + + if (freeze instanceof Boolean) { + return !(boolean) freeze; + } + + throw new RaiseException(getContext(), coreExceptions().argumentError( + "Kernel#clone expects :freeze to be boolean or nil", + this)); + } + + private boolean toNotForceChangingFreezeState(Object freeze) { + return freeze instanceof Nil; + } + private void raiseCantUnfreezeError(Object object) { throw new RaiseException(getContext(), coreExceptions().argumentErrorCantUnfreeze(object, this)); } diff --git a/src/main/ruby/truffleruby/core/kernel.rb b/src/main/ruby/truffleruby/core/kernel.rb index 00cec2625676..54bb07821292 100644 --- a/src/main/ruby/truffleruby/core/kernel.rb +++ b/src/main/ruby/truffleruby/core/kernel.rb @@ -749,7 +749,7 @@ def fork Primitive.method_unimplement method(:fork) Primitive.method_unimplement nil.method(:fork) - def clone(freeze: true) + def clone(freeze: nil) Primitive.object_clone self, freeze end diff --git a/src/main/ruby/truffleruby/core/numeric.rb b/src/main/ruby/truffleruby/core/numeric.rb index 9baa3fc51511..af608257f834 100644 --- a/src/main/ruby/truffleruby/core/numeric.rb +++ b/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 freeze == false raise ArgumentError, "can't unfreeze #{self.class.name}" end self From 6d5cce80368671410067cbecaabd4676c5779592 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Fri, 22 Oct 2021 14:17:34 +0300 Subject: [PATCH 02/14] Add specs for Kernel#clone(freeze: nil) --- spec/ruby/core/kernel/clone_spec.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/ruby/core/kernel/clone_spec.rb b/spec/ruby/core/kernel/clone_spec.rb index f9daa2badc62..62f6fb60c440 100644 --- a/spec/ruby/core/kernel/clone_spec.rb +++ b/spec/ruby/core/kernel/clone_spec.rb @@ -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" do + o2 = @obj.clone(freeze: nil) + @obj.freeze + o3 = @obj.clone(freeze: nil) + + o2.should_not.frozen? + 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 From c4cb96cf939491b0b888a9e85d261c4e589be68c Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Fri, 22 Oct 2021 14:32:22 +0300 Subject: [PATCH 03/14] Remove fixed test name from the kernel/clone_tags.txt file --- spec/tags/core/kernel/clone_tags.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/tags/core/kernel/clone_tags.txt b/spec/tags/core/kernel/clone_tags.txt index a6736412f3d2..1c4a03d12092 100644 --- a/spec/tags/core/kernel/clone_tags.txt +++ b/spec/tags/core/kernel/clone_tags.txt @@ -1,4 +1,3 @@ -fails:Kernel#clone with freeze: true freezes the copy even if the original was not frozen fails:Kernel#clone with freeze: true calls #initialize_clone with kwargs freeze: true fails:Kernel#clone with freeze: true calls #initialize_clone with kwargs freeze: true even if #initialize_clone only takes a single argument fails:Kernel#clone with freeze: false calls #initialize_clone with kwargs freeze: false From 47ff7b68d8c44dfae11b7b2e8fb105480ca10978 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Fri, 22 Oct 2021 19:25:51 +0300 Subject: [PATCH 04/14] Address comments and fix linter warnings --- spec/ruby/core/kernel/clone_spec.rb | 10 ++--- .../truffleruby/core/kernel/KernelNodes.java | 40 +++++++++---------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/spec/ruby/core/kernel/clone_spec.rb b/spec/ruby/core/kernel/clone_spec.rb index 62f6fb60c440..95f2e221a97e 100644 --- a/spec/ruby/core/kernel/clone_spec.rb +++ b/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 @@ -52,12 +52,12 @@ def klass.allocate end ruby_version_is "3.0" do - it "copies frozen state from the original" 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) - - o2.should_not.frozen? o3.should.frozen? end diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index 5508f91668f6..1643b6cd509d 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -556,11 +556,11 @@ public abstract static class CloneNode extends PrimitiveNode { @Child private DispatchNode initializeCloneNode = DispatchNode.create(); @Child private SingletonClassNode singletonClassNode; + private final BranchProfile errorProfile = BranchProfile.create(); + @Specialization(limit = "getRubyLibraryCacheLimit()") protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, @Cached ConditionProfile isSingletonProfile, - @Cached ConditionProfile freezeProfile, - @Cached ConditionProfile isFrozenProfile, @Cached ConditionProfile isRubyClass, @CachedLibrary("object") RubyLibrary rubyLibrary, @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryFreeze) { @@ -588,8 +588,7 @@ protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, } @Specialization - protected Object cloneBoolean(boolean object, Object freeze, - @Cached ConditionProfile freezeProfile) { + protected Object cloneBoolean(boolean object, Object freeze) { if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } @@ -597,8 +596,7 @@ protected Object cloneBoolean(boolean object, Object freeze, } @Specialization - protected Object cloneInteger(int object, Object freeze, - @Cached ConditionProfile freezeProfile) { + protected Object cloneInteger(int object, Object freeze) { if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } @@ -606,8 +604,7 @@ protected Object cloneInteger(int object, Object freeze, } @Specialization - protected Object cloneLong(long object, Object freeze, - @Cached ConditionProfile freezeProfile) { + protected Object cloneLong(long object, Object freeze) { if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } @@ -615,8 +612,7 @@ protected Object cloneLong(long object, Object freeze, } @Specialization - protected Object cloneFloat(double object, Object freeze, - @Cached ConditionProfile freezeProfile) { + protected Object cloneFloat(double object, Object freeze) { if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } @@ -624,8 +620,7 @@ protected Object cloneFloat(double object, Object freeze, } @Specialization(guards = "!isImmutableRubyString(object)") - protected Object cloneImmutableObject(ImmutableRubyObject object, Object freeze, - @Cached ConditionProfile freezeProfile) { + protected Object cloneImmutableObject(ImmutableRubyObject object, Object freeze) { if (toForceUnfreezing(freeze)) { raiseCantUnfreezeError(object); } @@ -634,7 +629,6 @@ protected Object cloneImmutableObject(ImmutableRubyObject object, Object freeze, @Specialization protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, Object freeze, - @Cached ConditionProfile freezeProfile, @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryFreeze, @Cached MakeStringNode makeStringNode) { final RubyDynamicObject newObject = makeStringNode.fromRope(object.rope, object.encoding); @@ -646,17 +640,18 @@ protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, } private boolean toForceFreezing(Object freeze) { - if (freeze instanceof Nil) { - return false; - } + if (freeze instanceof Nil) { + return false; + } - if (freeze instanceof Boolean) { - return (boolean) freeze; - } + if (freeze instanceof Boolean) { + return (boolean) freeze; + } - throw new RaiseException(getContext(), coreExceptions().argumentError( - "Kernel#clone expects :freeze to be boolean or nil", - this)); + errorProfile.enter(); + throw new RaiseException(getContext(), coreExceptions().argumentError( + "Kernel#clone expects :freeze to be boolean or nil", + this)); } private boolean toForceUnfreezing(Object freeze) { @@ -668,6 +663,7 @@ private boolean toForceUnfreezing(Object freeze) { return !(boolean) freeze; } + errorProfile.enter(); throw new RaiseException(getContext(), coreExceptions().argumentError( "Kernel#clone expects :freeze to be boolean or nil", this)); From c71cc13514487099fff308863b43e5983e9c2d43 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 3 Nov 2021 00:07:43 +0200 Subject: [PATCH 05/14] Pass :freeze to #initialize_clone --- spec/ruby/core/kernel/fixtures/classes.rb | 7 ++++- spec/tags/core/kernel/clone_tags.txt | 4 --- .../truffleruby/core/kernel/KernelNodes.java | 29 ++++++++++++++++++- 3 files changed, 34 insertions(+), 6 deletions(-) delete mode 100644 spec/tags/core/kernel/clone_tags.txt diff --git a/spec/ruby/core/kernel/fixtures/classes.rb b/spec/ruby/core/kernel/fixtures/classes.rb index 8de1407b92ee..c2ea07017946 100644 --- a/spec/ruby/core/kernel/fixtures/classes.rb +++ b/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 to support calling #clone with optional :freeze keyword argument + def initialize_clone(other, **kw) + super(other) # to call #initialize_copy + end end class Clone diff --git a/spec/tags/core/kernel/clone_tags.txt b/spec/tags/core/kernel/clone_tags.txt deleted file mode 100644 index 1c4a03d12092..000000000000 --- a/spec/tags/core/kernel/clone_tags.txt +++ /dev/null @@ -1,4 +0,0 @@ -fails:Kernel#clone with freeze: true calls #initialize_clone with kwargs freeze: true -fails:Kernel#clone with freeze: true calls #initialize_clone with kwargs freeze: true even if #initialize_clone only takes a single argument -fails:Kernel#clone with freeze: false calls #initialize_clone with kwargs freeze: false -fails:Kernel#clone with freeze: false calls #initialize_clone with kwargs freeze: false even if #initialize_clone only takes a single argument diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index 1643b6cd509d..cf75b4814ccb 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -20,6 +20,7 @@ import com.oracle.truffle.api.interop.InteropLibrary; import com.oracle.truffle.api.interop.UnsupportedMessageException; import com.oracle.truffle.api.utilities.AssumedValue; +import org.jcodings.specific.USASCIIEncoding; import org.truffleruby.RubyContext; import org.truffleruby.builtins.CoreMethod; import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; @@ -55,6 +56,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; @@ -73,6 +77,7 @@ import org.truffleruby.core.proc.ProcOperations; import org.truffleruby.core.proc.RubyProc; import org.truffleruby.core.rope.CodeRange; +import org.truffleruby.core.rope.LeafRope; import org.truffleruby.core.rope.Rope; import org.truffleruby.core.rope.RopeNodes; import org.truffleruby.core.rope.RopeOperations; @@ -562,6 +567,7 @@ public abstract static class CloneNode extends PrimitiveNode { protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, @Cached ConditionProfile isSingletonProfile, @Cached ConditionProfile isRubyClass, + @Cached HashingNodes.ToHashByHashCode hashNode, @CachedLibrary("object") RubyLibrary rubyLibrary, @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryFreeze) { final RubyDynamicObject newObject = copyNode.executeCopy(object); @@ -573,7 +579,28 @@ protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, newObjectMetaClass.fields.initCopy(selfMetaClass); } - initializeCloneNode.call(newObject, "initialize_clone", object); + // pass :freeze keyword argument to #initialize_clone + if (toForceFreezing(freeze) || toForceUnfreezing(freeze)) { + final String string = "freeze"; + final LeafRope rope = RopeOperations.encodeAscii(string, USASCIIEncoding.INSTANCE); + final RubySymbol key = new RubySymbol(string, rope, Encodings.US_ASCII); + final boolean value = toForceFreezing(freeze); + + final Object[] newStore = PackedHashStoreLibrary.createStore(); + final int hashed = hashNode.execute(key); + PackedHashStoreLibrary.setHashedKeyValue(newStore, 0, hashed, key, value); + + final RubyHash hash = new RubyHash( + coreLibrary().hashClass, + getLanguage().hashShape, + getContext(), + newStore, + 1); + + initializeCloneNode.call(newObject, "initialize_clone", object, hash); + } else { + initializeCloneNode.call(newObject, "initialize_clone", object); + } // Default behavior - is just to copy the frozen state of the original object if (toForceFreezing(freeze) || (toNotForceChangingFreezeState(freeze) && rubyLibrary.isFrozen(object))) { From bed4de16690c7d4341093b230709e286a3466aa9 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 3 Nov 2021 22:31:14 +0200 Subject: [PATCH 06/14] Address comments --- spec/ruby/core/kernel/clone_spec.rb | 8 +++ spec/ruby/core/numeric/clone_spec.rb | 5 ++ .../truffleruby/core/kernel/KernelNodes.java | 57 +++++++------------ src/main/ruby/truffleruby/core/kernel.rb | 4 ++ src/main/ruby/truffleruby/core/numeric.rb | 2 +- 5 files changed, 40 insertions(+), 36 deletions(-) diff --git a/spec/ruby/core/kernel/clone_spec.rb b/spec/ruby/core/kernel/clone_spec.rb index 95f2e221a97e..38ae1984c09a 100644 --- a/spec/ruby/core/kernel/clone_spec.rb +++ b/spec/ruby/core/kernel/clone_spec.rb @@ -136,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 diff --git a/spec/ruby/core/numeric/clone_spec.rb b/spec/ruby/core/numeric/clone_spec.rb index e3bf0a9e7c55..5834b17089e7 100644 --- a/spec/ruby/core/numeric/clone_spec.rb +++ b/spec/ruby/core/numeric/clone_spec.rb @@ -22,4 +22,9 @@ it "raises ArgumentError if passed freeze: false" do -> { 1.clone(freeze: false) }.should raise_error(ArgumentError, /can't unfreeze/) end + + it "does not change frozen status if passed freeze: nil" do + value = 1 + value.clone(freeze: nil).should equal(value) + end end diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index cf75b4814ccb..b87b53c1572c 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -561,8 +561,6 @@ public abstract static class CloneNode extends PrimitiveNode { @Child private DispatchNode initializeCloneNode = DispatchNode.create(); @Child private SingletonClassNode singletonClassNode; - private final BranchProfile errorProfile = BranchProfile.create(); - @Specialization(limit = "getRubyLibraryCacheLimit()") protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, @Cached ConditionProfile isSingletonProfile, @@ -581,23 +579,8 @@ protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, // pass :freeze keyword argument to #initialize_clone if (toForceFreezing(freeze) || toForceUnfreezing(freeze)) { - final String string = "freeze"; - final LeafRope rope = RopeOperations.encodeAscii(string, USASCIIEncoding.INSTANCE); - final RubySymbol key = new RubySymbol(string, rope, Encodings.US_ASCII); - final boolean value = toForceFreezing(freeze); - - final Object[] newStore = PackedHashStoreLibrary.createStore(); - final int hashed = hashNode.execute(key); - PackedHashStoreLibrary.setHashedKeyValue(newStore, 0, hashed, key, value); - - final RubyHash hash = new RubyHash( - coreLibrary().hashClass, - getLanguage().hashShape, - getContext(), - newStore, - 1); - - initializeCloneNode.call(newObject, "initialize_clone", object, hash); + final RubyHash keywordArguments = createFreezeBooleanHash((boolean) freeze, hashNode); + initializeCloneNode.call(newObject, "initialize_clone", object, keywordArguments); } else { initializeCloneNode.call(newObject, "initialize_clone", object); } @@ -666,19 +649,30 @@ protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, return newObject; } + private RubyHash createFreezeBooleanHash(boolean freeze, HashingNodes.ToHashByHashCode hashNode) { + final RubySymbol key = coreSymbols().FREEZE; + final boolean value = freeze; + + final Object[] newStore = PackedHashStoreLibrary.createStore(); + final int hashed = hashNode.execute(key); + PackedHashStoreLibrary.setHashedKeyValue(newStore, 0, hashed, key, value); + + final RubyHash hash = new RubyHash( + coreLibrary().hashClass, + getLanguage().hashShape, + getContext(), + newStore, + 1); + + return hash; + } + private boolean toForceFreezing(Object freeze) { if (freeze instanceof Nil) { return false; } - if (freeze instanceof Boolean) { - return (boolean) freeze; - } - - errorProfile.enter(); - throw new RaiseException(getContext(), coreExceptions().argumentError( - "Kernel#clone expects :freeze to be boolean or nil", - this)); + return (boolean) freeze; } private boolean toForceUnfreezing(Object freeze) { @@ -686,14 +680,7 @@ private boolean toForceUnfreezing(Object freeze) { return false; } - if (freeze instanceof Boolean) { - return !(boolean) freeze; - } - - errorProfile.enter(); - throw new RaiseException(getContext(), coreExceptions().argumentError( - "Kernel#clone expects :freeze to be boolean or nil", - this)); + return !(boolean) freeze; } private boolean toNotForceChangingFreezeState(Object freeze) { diff --git a/src/main/ruby/truffleruby/core/kernel.rb b/src/main/ruby/truffleruby/core/kernel.rb index 54bb07821292..d24c9d517081 100644 --- a/src/main/ruby/truffleruby/core/kernel.rb +++ b/src/main/ruby/truffleruby/core/kernel.rb @@ -750,6 +750,10 @@ def fork Primitive.method_unimplement nil.method(:fork) def clone(freeze: nil) + unless TrueClass === freeze || FalseClass === freeze || NilClass === freeze + raise ArgumentError, "unexpected value for freeze: #{freeze.class}" + end + Primitive.object_clone self, freeze end diff --git a/src/main/ruby/truffleruby/core/numeric.rb b/src/main/ruby/truffleruby/core/numeric.rb index af608257f834..be0b30187aa6 100644 --- a/src/main/ruby/truffleruby/core/numeric.rb +++ b/src/main/ruby/truffleruby/core/numeric.rb @@ -30,7 +30,7 @@ class Numeric include Comparable def clone(freeze: nil) - if freeze == false + if Primitive.object_equal(freeze, false) raise ArgumentError, "can't unfreeze #{self.class.name}" end self From 10ec70fb84acf2a239dd1d74205956f78489d3b2 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 3 Nov 2021 22:41:26 +0200 Subject: [PATCH 07/14] Fix typo and remove redundant imports --- spec/ruby/core/kernel/fixtures/classes.rb | 2 +- src/main/java/org/truffleruby/core/kernel/KernelNodes.java | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/ruby/core/kernel/fixtures/classes.rb b/spec/ruby/core/kernel/fixtures/classes.rb index c2ea07017946..541a4c075e55 100644 --- a/spec/ruby/core/kernel/fixtures/classes.rb +++ b/spec/ruby/core/kernel/fixtures/classes.rb @@ -285,7 +285,7 @@ def initialize_copy(other, **kw) ScratchPad.record object_id end - # define to to support calling #clone with optional :freeze keyword argument + # define to support calling #clone with optional :freeze keyword argument def initialize_clone(other, **kw) super(other) # to call #initialize_copy end diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index b87b53c1572c..f2877f2e78e2 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -20,7 +20,6 @@ import com.oracle.truffle.api.interop.InteropLibrary; import com.oracle.truffle.api.interop.UnsupportedMessageException; import com.oracle.truffle.api.utilities.AssumedValue; -import org.jcodings.specific.USASCIIEncoding; import org.truffleruby.RubyContext; import org.truffleruby.builtins.CoreMethod; import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; @@ -77,7 +76,6 @@ import org.truffleruby.core.proc.ProcOperations; import org.truffleruby.core.proc.RubyProc; import org.truffleruby.core.rope.CodeRange; -import org.truffleruby.core.rope.LeafRope; import org.truffleruby.core.rope.Rope; import org.truffleruby.core.rope.RopeNodes; import org.truffleruby.core.rope.RopeOperations; From 1cd4c74fdee44bd78fe7917025e9bbd28bb88ded Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Thu, 4 Nov 2021 18:08:07 +0100 Subject: [PATCH 08/14] Cleanup Primitive.object_clone --- .../truffleruby/core/kernel/KernelNodes.java | 71 ++++++++----------- 1 file changed, 28 insertions(+), 43 deletions(-) diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index f2877f2e78e2..7c6c6f289755 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -555,12 +555,13 @@ 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; + private final BranchProfile cantUnfreezeErrorProfile = BranchProfile.create(); @Specialization(limit = "getRubyLibraryCacheLimit()") protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, + @Cached CopyNode copyNode, + @Cached DispatchNode initializeCloneNode, @Cached ConditionProfile isSingletonProfile, @Cached ConditionProfile isRubyClass, @Cached HashingNodes.ToHashByHashCode hashNode, @@ -575,16 +576,18 @@ protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, newObjectMetaClass.fields.initCopy(selfMetaClass); } - // pass :freeze keyword argument to #initialize_clone - if (toForceFreezing(freeze) || toForceUnfreezing(freeze)) { + final boolean copyFrozen = freeze instanceof Nil; + + 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); - } else { - initializeCloneNode.call(newObject, "initialize_clone", object); } // Default behavior - is just to copy the frozen state of the original object - if (toForceFreezing(freeze) || (toNotForceChangingFreezeState(freeze) && rubyLibrary.isFrozen(object))) { + if (forceFrozen(freeze) || (copyFrozen && rubyLibrary.isFrozen(object))) { rubyLibraryFreeze.freeze(newObject); } @@ -596,40 +599,40 @@ protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze, } @Specialization - protected Object cloneBoolean(boolean object, Object freeze) { - if (toForceUnfreezing(freeze)) { + protected boolean cloneBoolean(boolean object, Object freeze) { + if (forceNotFrozen(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization - protected Object cloneInteger(int object, Object freeze) { - if (toForceUnfreezing(freeze)) { + protected int cloneInteger(int object, Object freeze) { + if (forceNotFrozen(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization - protected Object cloneLong(long object, Object freeze) { - if (toForceUnfreezing(freeze)) { + protected long cloneLong(long object, Object freeze) { + if (forceNotFrozen(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization - protected Object cloneFloat(double object, Object freeze) { - if (toForceUnfreezing(freeze)) { + protected double cloneFloat(double object, Object freeze) { + if (forceNotFrozen(freeze)) { raiseCantUnfreezeError(object); } return object; } @Specialization(guards = "!isImmutableRubyString(object)") - protected Object cloneImmutableObject(ImmutableRubyObject object, Object freeze) { - if (toForceUnfreezing(freeze)) { + protected ImmutableRubyObject cloneImmutableObject(ImmutableRubyObject object, Object freeze) { + if (forceNotFrozen(freeze)) { raiseCantUnfreezeError(object); } return object; @@ -640,7 +643,7 @@ protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryFreeze, @Cached MakeStringNode makeStringNode) { final RubyDynamicObject newObject = makeStringNode.fromRope(object.rope, object.encoding); - if (!toForceUnfreezing(freeze)) { + if (!forceNotFrozen(freeze)) { rubyLibraryFreeze.freeze(newObject); } @@ -649,43 +652,25 @@ protected RubyDynamicObject cloneImmutableRubyString(ImmutableRubyString object, private RubyHash createFreezeBooleanHash(boolean freeze, HashingNodes.ToHashByHashCode hashNode) { final RubySymbol key = coreSymbols().FREEZE; - final boolean value = freeze; final Object[] newStore = PackedHashStoreLibrary.createStore(); final int hashed = hashNode.execute(key); - PackedHashStoreLibrary.setHashedKeyValue(newStore, 0, hashed, key, value); - - final RubyHash hash = new RubyHash( - coreLibrary().hashClass, - getLanguage().hashShape, - getContext(), - newStore, - 1); + PackedHashStoreLibrary.setHashedKeyValue(newStore, 0, hashed, key, freeze); - return hash; + return new RubyHash(coreLibrary().hashClass, getLanguage().hashShape, getContext(), newStore, 1); } - private boolean toForceFreezing(Object freeze) { - if (freeze instanceof Nil) { - return false; - } - - return (boolean) freeze; - } - - private boolean toForceUnfreezing(Object freeze) { - if (freeze instanceof Nil) { - return false; - } + private boolean forceFrozen(Object freeze) { + return freeze instanceof Boolean && (boolean) freeze; - return !(boolean) freeze; } - private boolean toNotForceChangingFreezeState(Object freeze) { - return freeze instanceof Nil; + 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)); } From bb9b468a989f78e1a1d56977beecaccc245af436 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Thu, 4 Nov 2021 18:11:46 +0100 Subject: [PATCH 09/14] Add Primitive.boolean_or_nil? to optimize check --- .../truffleruby/core/support/TypeNodes.java | 18 ++++++++++++++++++ src/main/ruby/truffleruby/core/kernel.rb | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/truffleruby/core/support/TypeNodes.java b/src/main/java/org/truffleruby/core/support/TypeNodes.java index 50b5f68bf35f..e487bb34d501 100644 --- a/src/main/java/org/truffleruby/core/support/TypeNodes.java +++ b/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 { diff --git a/src/main/ruby/truffleruby/core/kernel.rb b/src/main/ruby/truffleruby/core/kernel.rb index d24c9d517081..9b6fd508e8b4 100644 --- a/src/main/ruby/truffleruby/core/kernel.rb +++ b/src/main/ruby/truffleruby/core/kernel.rb @@ -750,7 +750,7 @@ def fork Primitive.method_unimplement nil.method(:fork) def clone(freeze: nil) - unless TrueClass === freeze || FalseClass === freeze || NilClass === freeze + unless Primitive.boolean_or_nil?(freeze) raise ArgumentError, "unexpected value for freeze: #{freeze.class}" end From 43d574c26e86a2e49c2a2e19aac52f2bff2a59a9 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Thu, 4 Nov 2021 18:21:08 +0100 Subject: [PATCH 10/14] Add ChangeLog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f949913672d4..38a4f2a452c6 100644 --- a/CHANGELOG.md +++ b/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: From 43183951383b18639c9329d40a639121cc47e2b7 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Fri, 5 Nov 2021 13:26:37 +0100 Subject: [PATCH 11/14] Add version guard --- spec/ruby/core/numeric/clone_spec.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/ruby/core/numeric/clone_spec.rb b/spec/ruby/core/numeric/clone_spec.rb index 5834b17089e7..c3b06ca0c903 100644 --- a/spec/ruby/core/numeric/clone_spec.rb +++ b/spec/ruby/core/numeric/clone_spec.rb @@ -23,8 +23,10 @@ -> { 1.clone(freeze: false) }.should raise_error(ArgumentError, /can't unfreeze/) end - it "does not change frozen status if passed freeze: nil" do - value = 1 - value.clone(freeze: nil).should equal(value) + 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 From 714f1f5fda4b0cb58e81476fd1fc0afab059d319 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Fri, 5 Nov 2021 13:32:19 +0100 Subject: [PATCH 12/14] Add specs for Kernel#{initialize_clone,initialize_dup} --- .../ruby/core/kernel/initialize_clone_spec.rb | 28 +++++++++++++++++++ spec/ruby/core/kernel/initialize_dup_spec.rb | 20 +++++++++++++ .../core/kernel/initialize_clone_tags.txt | 1 + 3 files changed, 49 insertions(+) create mode 100644 spec/ruby/core/kernel/initialize_clone_spec.rb create mode 100644 spec/ruby/core/kernel/initialize_dup_spec.rb create mode 100644 spec/tags/core/kernel/initialize_clone_tags.txt diff --git a/spec/ruby/core/kernel/initialize_clone_spec.rb b/spec/ruby/core/kernel/initialize_clone_spec.rb new file mode 100644 index 000000000000..2d889f5aadfe --- /dev/null +++ b/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 diff --git a/spec/ruby/core/kernel/initialize_dup_spec.rb b/spec/ruby/core/kernel/initialize_dup_spec.rb new file mode 100644 index 000000000000..6dff34b7ad8f --- /dev/null +++ b/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 diff --git a/spec/tags/core/kernel/initialize_clone_tags.txt b/spec/tags/core/kernel/initialize_clone_tags.txt new file mode 100644 index 000000000000..be636994dd6f --- /dev/null +++ b/spec/tags/core/kernel/initialize_clone_tags.txt @@ -0,0 +1 @@ +fails:Kernel#initialize_clone accepts a :freeze keyword argument for obj.clone(freeze: value) From add3b6bef989ad9a23d3cb7979e27516ca833142 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Fri, 5 Nov 2021 13:37:14 +0100 Subject: [PATCH 13/14] Accept the :freeze keyword argument for #initialize_clone --- spec/tags/core/kernel/initialize_clone_tags.txt | 1 - .../java/org/truffleruby/core/kernel/KernelNodes.java | 10 +++++----- src/main/ruby/truffleruby/core/kernel.rb | 4 ++++ 3 files changed, 9 insertions(+), 6 deletions(-) delete mode 100644 spec/tags/core/kernel/initialize_clone_tags.txt diff --git a/spec/tags/core/kernel/initialize_clone_tags.txt b/spec/tags/core/kernel/initialize_clone_tags.txt deleted file mode 100644 index be636994dd6f..000000000000 --- a/spec/tags/core/kernel/initialize_clone_tags.txt +++ /dev/null @@ -1 +0,0 @@ -fails:Kernel#initialize_clone accepts a :freeze keyword argument for obj.clone(freeze: value) diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index 7c6c6f289755..c0ba9003151c 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -729,7 +729,7 @@ protected DispatchingNode initializeDupNode() { initializeDupNode = insert( new InlinedDispatchNode( getLanguage(), - InitializeDupCloneNode.create())); + InitializeDupNode.create())); } return initializeDupNode; } @@ -1125,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; diff --git a/src/main/ruby/truffleruby/core/kernel.rb b/src/main/ruby/truffleruby/core/kernel.rb index 9b6fd508e8b4..c387a1814374 100644 --- a/src/main/ruby/truffleruby/core/kernel.rb +++ b/src/main/ruby/truffleruby/core/kernel.rb @@ -757,6 +757,10 @@ def clone(freeze: nil) 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=$/) From 00c7b28c9917fd80a059264677340d10e6799270 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Fri, 5 Nov 2021 13:45:48 +0100 Subject: [PATCH 14/14] Untag passing MRI tests --- test/mri/excludes/TestNumeric.rb | 1 - test/mri/excludes/TestObject.rb | 2 -- 2 files changed, 3 deletions(-) diff --git a/test/mri/excludes/TestNumeric.rb b/test/mri/excludes/TestNumeric.rb index 0f591de0ed16..47125280df97 100644 --- a/test/mri/excludes/TestNumeric.rb +++ b/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" diff --git a/test/mri/excludes/TestObject.rb b/test/mri/excludes/TestObject.rb index c49a51fa4330..b86a4a401d5d 100644 --- a/test/mri/excludes/TestObject.rb +++ b/test/mri/excludes/TestObject.rb @@ -1,5 +1,4 @@ exclude :test_bad_initialize_copy, "needs investigation" -exclude :test_clone, "needs investigation" exclude :test_frozen_error_message, "needs investigation" exclude :test_implicit_respond_to_arity_3, "needs investigation" exclude :test_method_missing, "needs investigation" @@ -9,4 +8,3 @@ exclude :test_respond_to_missing, "needs investigation" exclude :test_type_error_message, "needs investigation" exclude :test_check_to_integer, "needs investigation" -exclude :test_implicit_respond_to_arity_1, "needs investigation"