proc.c: get rid of CLONESETUP

[Bug #20253]

All the way down to Ruby 1.9, `Proc`, `Method`, `UnboundMethod`
and `Binding` always had their own specific clone and dup routine.

This caused various discrepancies with how other objects behave
on `dup` and `clone. [Bug #20250], [Bug #20253].

This commit get rid of `CLONESETUP` and use the the same codepath
as all other types, so ensure consistency.

NB: It's still not accepting the `freeze` keyword argument on `clone`.

Co-Authored-By: Étienne Barrié <etienne.barrie@gmail.com>

internal/object.h

@@ -16,6 +16,8 @@ VALUE rb_class_search_ancestor(VALUE klass, VALUE super);
 NORETURN(void rb_undefined_alloc(VALUE klass));
 double rb_num_to_dbl(VALUE val);
 VALUE rb_obj_dig(int argc, VALUE *argv, VALUE self, VALUE notfound);
+VALUE rb_obj_clone_setup(VALUE obj, VALUE clone, VALUE kwfreeze);
+VALUE rb_obj_dup_setup(VALUE obj, VALUE dup);
 VALUE rb_immutable_obj_clone(int, VALUE *, VALUE);
 VALUE rb_check_convert_type_with_id(VALUE,int,const char*,ID);
 int rb_bool_expected(VALUE, const char *, int raise);

object.c

@@ -454,15 +454,12 @@ immutable_obj_clone(VALUE obj, VALUE kwfreeze)
     return obj;
 }
 
-static VALUE
+VALUE
-mutable_obj_clone(VALUE obj, VALUE kwfreeze)
+rb_obj_clone_setup(VALUE obj, VALUE clone, VALUE kwfreeze)
 {
-    VALUE clone, singleton;
     VALUE argv[2];
 
-    clone = rb_obj_alloc(rb_obj_class(obj));
+    VALUE singleton = rb_singleton_class_clone_and_attach(obj, clone);
-
-    singleton = rb_singleton_class_clone_and_attach(obj, clone);
     RBASIC_SET_CLASS(clone, singleton);
     if (FL_TEST(singleton, FL_SINGLETON)) {
         rb_singleton_class_attached(singleton, clone);
@@ -529,6 +526,13 @@ mutable_obj_clone(VALUE obj, VALUE kwfreeze)
     return clone;
 }
 
+static VALUE
+mutable_obj_clone(VALUE obj, VALUE kwfreeze)
+{
+    VALUE clone = rb_obj_alloc(rb_obj_class(obj));
+    return rb_obj_clone_setup(obj, clone, kwfreeze);
+}
+
 VALUE
 rb_obj_clone(VALUE obj)
 {
@@ -536,6 +540,15 @@ rb_obj_clone(VALUE obj)
     return mutable_obj_clone(obj, Qnil);
 }
 
+VALUE
+rb_obj_dup_setup(VALUE obj, VALUE dup)
+{
+    init_copy(dup, obj);
+    rb_funcall(dup, id_init_dup, 1, obj);
+
+    return dup;
+}
+
 /*
  *  call-seq:
  *     obj.dup -> an_object
@@ -584,10 +597,7 @@ rb_obj_dup(VALUE obj)
         return obj;
     }
     dup = rb_obj_alloc(rb_obj_class(obj));
-    init_copy(dup, obj);
+    return rb_obj_dup_setup(obj, dup);
-    rb_funcall(dup, id_init_dup, 1, obj);
-
-    return dup;
 }
 
 /*

proc.c

@@ -51,23 +51,6 @@ static VALUE proc_binding(VALUE self);
 
 #define IS_METHOD_PROC_IFUNC(ifunc) ((ifunc)->func == bmcall)
 
-/* :FIXME: The way procs are cloned has been historically different from the
- * way everything else are.  @shyouhei is not sure for the intention though.
- */
-#undef CLONESETUP
-static inline void
-CLONESETUP(VALUE clone, VALUE obj)
-{
-    RBIMPL_ASSERT_OR_ASSUME(! RB_SPECIAL_CONST_P(obj));
-    RBIMPL_ASSERT_OR_ASSUME(! RB_SPECIAL_CONST_P(clone));
-
-    const VALUE flags = RUBY_FL_PROMOTED | RUBY_FL_FINALIZE;
-    rb_obj_setup(clone, rb_singleton_class_clone(obj),
-                 RB_FL_TEST_RAW(obj, ~flags));
-    rb_singleton_class_attached(RBASIC_CLASS(clone), clone);
-    if (RB_FL_TEST(obj, RUBY_FL_EXIVAR)) rb_copy_generic_ivar(clone, obj);
-}
-
 static void
 block_mark_and_move(struct rb_block *block)
 {
@@ -142,9 +125,7 @@ static VALUE
 proc_clone(VALUE self)
 {
     VALUE procval = rb_proc_dup(self);
-    CLONESETUP(procval, self);
+    return rb_obj_clone_setup(self, procval, Qnil);
-    rb_check_funcall(procval, idInitialize_clone, 1, &self);
-    return procval;
 }
 
 /* :nodoc: */
@@ -152,8 +133,7 @@ static VALUE
 proc_dup(VALUE self)
 {
     VALUE procval = rb_proc_dup(self);
-    rb_check_funcall(procval, idInitialize_dup, 1, &self);
+    return rb_obj_dup_setup(self, procval);
-    return procval;
 }
 
 /*
@@ -328,7 +308,7 @@ binding_dup(VALUE self)
     rb_vm_block_copy(bindval, &dst->block, &src->block);
     RB_OBJ_WRITE(bindval, &dst->pathobj, src->pathobj);
     dst->first_lineno = src->first_lineno;
-    return bindval;
+    return rb_obj_dup_setup(self, bindval);
 }
 
 /* :nodoc: */
@@ -336,8 +316,7 @@ static VALUE
 binding_clone(VALUE self)
 {
     VALUE bindval = binding_dup(self);
-    CLONESETUP(bindval, self);
+    return rb_obj_clone_setup(self, bindval, Qnil);
-    return bindval;
 }
 
 VALUE
@@ -2385,7 +2364,25 @@ method_clone(VALUE self)
 
     TypedData_Get_Struct(self, struct METHOD, &method_data_type, orig);
     clone = TypedData_Make_Struct(CLASS_OF(self), struct METHOD, &method_data_type, data);
-    CLONESETUP(clone, self);
+    rb_obj_clone_setup(self, clone, Qnil);
+    RB_OBJ_WRITE(clone, &data->recv, orig->recv);
+    RB_OBJ_WRITE(clone, &data->klass, orig->klass);
+    RB_OBJ_WRITE(clone, &data->iclass, orig->iclass);
+    RB_OBJ_WRITE(clone, &data->owner, orig->owner);
+    RB_OBJ_WRITE(clone, &data->me, rb_method_entry_clone(orig->me));
+    return clone;
+}
+
+/* :nodoc: */
+static VALUE
+method_dup(VALUE self)
+{
+    VALUE clone;
+    struct METHOD *orig, *data;
+
+    TypedData_Get_Struct(self, struct METHOD, &method_data_type, orig);
+    clone = TypedData_Make_Struct(CLASS_OF(self), struct METHOD, &method_data_type, data);
+    rb_obj_dup_setup(self, clone);
     RB_OBJ_WRITE(clone, &data->recv, orig->recv);
     RB_OBJ_WRITE(clone, &data->klass, orig->klass);
     RB_OBJ_WRITE(clone, &data->iclass, orig->iclass);
@@ -4295,6 +4292,7 @@ Init_Proc(void)
     rb_define_method(rb_cMethod, "eql?", method_eq, 1);
     rb_define_method(rb_cMethod, "hash", method_hash, 0);
     rb_define_method(rb_cMethod, "clone", method_clone, 0);
+    rb_define_method(rb_cMethod, "dup", method_dup, 0);
     rb_define_method(rb_cMethod, "call", rb_method_call_pass_called_kw, -1);
     rb_define_method(rb_cMethod, "===", rb_method_call_pass_called_kw, -1);
     rb_define_method(rb_cMethod, "curry", rb_method_curry, -1);
@@ -4325,6 +4323,7 @@ Init_Proc(void)
     rb_define_method(rb_cUnboundMethod, "eql?", unbound_method_eq, 1);
     rb_define_method(rb_cUnboundMethod, "hash", method_hash, 0);
     rb_define_method(rb_cUnboundMethod, "clone", method_clone, 0);
+    rb_define_method(rb_cUnboundMethod, "dup", method_dup, 0);
     rb_define_method(rb_cUnboundMethod, "arity", method_arity_m, 0);
     rb_define_method(rb_cUnboundMethod, "inspect", method_inspect, 0);
     rb_define_method(rb_cUnboundMethod, "to_s", method_inspect, 0);

spec/ruby/core/binding/clone_spec.rb

@@ -4,4 +4,10 @@ require_relative 'shared/clone'
 
 describe "Binding#clone" do
   it_behaves_like :binding_clone, :clone
+
+  it "preserves frozen status" do
+    bind = binding.freeze
+    bind.frozen?.should == true
+    bind.clone.frozen?.should == true
+  end
 end

spec/ruby/core/binding/dup_spec.rb

@@ -4,4 +4,10 @@ require_relative 'shared/clone'
 
 describe "Binding#dup" do
   it_behaves_like :binding_clone, :dup
+
+  it "resets frozen status" do
+    bind = binding.freeze
+    bind.frozen?.should == true
+    bind.dup.frozen?.should == false
+  end
 end

spec/ruby/core/binding/shared/clone.rb

@@ -31,4 +31,26 @@ describe :binding_clone, shared: true do
       b2.local_variable_defined?(:x).should == false
     end
   end
+
+  ruby_version_is "3.4" do
+    it "copies instance variables" do
+      @b1.instance_variable_set(:@ivar, 1)
+      cl = @b1.send(@method)
+      cl.instance_variables.should == [:@ivar]
+    end
+
+    it "copies the finalizer" do
+      code = <<-RUBY
+        obj = binding
+
+        ObjectSpace.define_finalizer(obj, Proc.new { STDOUT.write "finalized\n" })
+
+        obj.clone
+
+        exit 0
+      RUBY
+
+      ruby_exe(code).lines.sort.should == ["finalized\n", "finalized\n"]
+    end
+  end
 end

spec/ruby/core/method/clone_spec.rb

@@ -1,14 +1,13 @@
 require_relative '../../spec_helper'
-require_relative 'fixtures/classes'
+require_relative 'shared/dup'
 
 describe "Method#clone" do
-  it "returns a copy of the method" do
+  it_behaves_like :method_dup, :clone
-    m1 = MethodSpecs::Methods.new.method(:foo)
-    m2 = m1.clone
 
-    m1.should == m2
+  it "preserves frozen status" do
-    m1.should_not equal(m2)
+    method = Object.new.method(:method)
-
+    method.freeze
-    m1.call.should == m2.call
+    method.frozen?.should == true
+    method.clone.frozen?.should == true
   end
 end

spec/ruby/core/method/dup_spec.rb

@@ -0,0 +1,15 @@
+require_relative '../../spec_helper'
+require_relative 'shared/dup'
+
+describe "Method#dup" do
+  ruby_version_is "3.4" do
+    it_behaves_like :method_dup, :dup
+
+    it "resets frozen status" do
+      method = Object.new.method(:method)
+      method.freeze
+      method.frozen?.should == true
+      method.dup.frozen?.should == false
+    end
+  end
+end

spec/ruby/core/method/shared/dup.rb

@@ -0,0 +1,32 @@
+describe :method_dup, shared: true do
+  it "returns a copy of self" do
+    a = Object.new.method(:method)
+    b = a.send(@method)
+
+    a.should == b
+    a.should_not equal(b)
+  end
+
+  ruby_version_is "3.4" do
+    it "copies instance variables" do
+      method = Object.new.method(:method)
+      method.instance_variable_set(:@ivar, 1)
+      cl = method.send(@method)
+      cl.instance_variables.should == [:@ivar]
+    end
+
+    it "copies the finalizer" do
+      code = <<-RUBY
+        obj = Object.new.method(:method)
+
+        ObjectSpace.define_finalizer(obj, Proc.new { STDOUT.write "finalized\n" })
+
+        obj.clone
+
+        exit 0
+      RUBY
+
+      ruby_exe(code).lines.sort.should == ["finalized\n", "finalized\n"]
+    end
+  end
+end

spec/ruby/core/proc/clone_spec.rb

@@ -3,4 +3,13 @@ require_relative 'shared/dup'
 
 describe "Proc#clone" do
   it_behaves_like :proc_dup, :clone
+
+  ruby_bug "cloning a frozen proc is broken on Ruby 3.3", "3.3"..."3.4" do
+    it "preserves frozen status" do
+      proc = Proc.new { }
+      proc.freeze
+      proc.frozen?.should == true
+      proc.clone.frozen?.should == true
+    end
+  end
 end

spec/ruby/core/proc/dup_spec.rb

@@ -3,4 +3,11 @@ require_relative 'shared/dup'
 
 describe "Proc#dup" do
   it_behaves_like :proc_dup, :dup
+
+  it "resets frozen status" do
+    proc = Proc.new { }
+    proc.freeze
+    proc.frozen?.should == true
+    proc.dup.frozen?.should == false
+  end
 end

spec/ruby/core/proc/shared/dup.rb

@@ -15,4 +15,27 @@ describe :proc_dup, shared: true do
       cl.new{}.send(@method).class.should == cl
     end
   end
+
+  ruby_version_is "3.4" do
+    it "copies instance variables" do
+      proc = -> { "hello" }
+      proc.instance_variable_set(:@ivar, 1)
+      cl = proc.send(@method)
+      cl.instance_variables.should == [:@ivar]
+    end
+
+    it "copies the finalizer" do
+      code = <<-RUBY
+        obj = Proc.new { }
+
+        ObjectSpace.define_finalizer(obj, Proc.new { STDOUT.write "finalized\n" })
+
+        obj.clone
+
+        exit 0
+      RUBY
+
+      ruby_exe(code).lines.sort.should == ["finalized\n", "finalized\n"]
+    end
+  end
 end

spec/ruby/core/unboundmethod/clone_spec.rb

@@ -1,12 +1,13 @@
 require_relative '../../spec_helper'
-require_relative 'fixtures/classes'
+require_relative 'shared/dup'
 
 describe "UnboundMethod#clone" do
-  it "returns a copy of the UnboundMethod" do
+  it_behaves_like :unboundmethod_dup, :clone
-    um1 = UnboundMethodSpecs::Methods.instance_method(:foo)
-    um2 = um1.clone
 
-    (um1 == um2).should == true
+  it "preserves frozen status" do
-    um1.bind(UnboundMethodSpecs::Methods.new).call.should == um2.bind(UnboundMethodSpecs::Methods.new).call
+    method = Class.instance_method(:instance_method)
+    method.freeze
+    method.frozen?.should == true
+    method.clone.frozen?.should == true
   end
 end

spec/ruby/core/unboundmethod/dup_spec.rb

@@ -0,0 +1,15 @@
+require_relative '../../spec_helper'
+require_relative 'shared/dup'
+
+describe "UnboundMethod#dup" do
+  ruby_version_is "3.4" do
+    it_behaves_like :unboundmethod_dup, :dup
+
+    it "resets frozen status" do
+      method = Class.instance_method(:instance_method)
+      method.freeze
+      method.frozen?.should == true
+      method.dup.frozen?.should == false
+    end
+  end
+end

spec/ruby/core/unboundmethod/shared/dup.rb

@@ -0,0 +1,32 @@
+describe :unboundmethod_dup, shared: true do
+  it "returns a copy of self" do
+    a = Class.instance_method(:instance_method)
+    b = a.send(@method)
+
+    a.should == b
+    a.should_not equal(b)
+  end
+
+  ruby_version_is "3.4" do
+    it "copies instance variables" do
+      method = Class.instance_method(:instance_method)
+      method.instance_variable_set(:@ivar, 1)
+      cl = method.send(@method)
+      cl.instance_variables.should == [:@ivar]
+    end
+
+    it "copies the finalizer" do
+      code = <<-RUBY
+        obj = Class.instance_method(:instance_method)
+
+        ObjectSpace.define_finalizer(obj, Proc.new { STDOUT.write "finalized\n" })
+
+        obj.clone
+
+        exit 0
+      RUBY
+
+      ruby_exe(code).lines.sort.should == ["finalized\n", "finalized\n"]
+    end
+  end
+end