From de1a586ecc2ee7f465f0c0a69291054136a3a819 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 12 Feb 2024 12:03:36 +0100 Subject: [PATCH] proc.c: get rid of `CLONESETUP` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [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é --- internal/object.h | 2 + object.c | 30 ++++++++----- proc.c | 51 +++++++++++----------- spec/ruby/core/binding/clone_spec.rb | 6 +++ spec/ruby/core/binding/dup_spec.rb | 6 +++ spec/ruby/core/binding/shared/clone.rb | 22 ++++++++++ spec/ruby/core/method/clone_spec.rb | 15 +++---- spec/ruby/core/method/dup_spec.rb | 15 +++++++ spec/ruby/core/method/shared/dup.rb | 32 ++++++++++++++ spec/ruby/core/proc/clone_spec.rb | 9 ++++ spec/ruby/core/proc/dup_spec.rb | 7 +++ spec/ruby/core/proc/shared/dup.rb | 23 ++++++++++ spec/ruby/core/unboundmethod/clone_spec.rb | 13 +++--- spec/ruby/core/unboundmethod/dup_spec.rb | 15 +++++++ spec/ruby/core/unboundmethod/shared/dup.rb | 32 ++++++++++++++ 15 files changed, 228 insertions(+), 50 deletions(-) create mode 100644 spec/ruby/core/method/dup_spec.rb create mode 100644 spec/ruby/core/method/shared/dup.rb create mode 100644 spec/ruby/core/unboundmethod/dup_spec.rb create mode 100644 spec/ruby/core/unboundmethod/shared/dup.rb diff --git a/internal/object.h b/internal/object.h index 06595bdd91a329..903e2d29a5871a 100644 --- a/internal/object.h +++ b/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); diff --git a/object.c b/object.c index 26ced2c8d89734..7d4b11ee48fe1c 100644 --- a/object.c +++ b/object.c @@ -454,15 +454,12 @@ immutable_obj_clone(VALUE obj, VALUE kwfreeze) return obj; } -static VALUE -mutable_obj_clone(VALUE obj, VALUE kwfreeze) +VALUE +rb_obj_clone_setup(VALUE obj, VALUE clone, VALUE kwfreeze) { - VALUE clone, singleton; VALUE argv[2]; - clone = rb_obj_alloc(rb_obj_class(obj)); - - singleton = rb_singleton_class_clone_and_attach(obj, clone); + VALUE 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); - rb_funcall(dup, id_init_dup, 1, obj); - - return dup; + return rb_obj_dup_setup(obj, dup); } /* diff --git a/proc.c b/proc.c index 34d2d4e71f6b8e..8d958d41531a41 100644 --- a/proc.c +++ b/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); - rb_check_funcall(procval, idInitialize_clone, 1, &self); - return procval; + return rb_obj_clone_setup(self, procval, Qnil); } /* :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 procval; + return rb_obj_dup_setup(self, 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 bindval; + return rb_obj_clone_setup(self, bindval, Qnil); } 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); diff --git a/spec/ruby/core/binding/clone_spec.rb b/spec/ruby/core/binding/clone_spec.rb index ebd40f5377e167..f1769ac6dea5f6 100644 --- a/spec/ruby/core/binding/clone_spec.rb +++ b/spec/ruby/core/binding/clone_spec.rb @@ -4,4 +4,10 @@ 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 diff --git a/spec/ruby/core/binding/dup_spec.rb b/spec/ruby/core/binding/dup_spec.rb index 43968213c8f643..55fac6e3332a84 100644 --- a/spec/ruby/core/binding/dup_spec.rb +++ b/spec/ruby/core/binding/dup_spec.rb @@ -4,4 +4,10 @@ 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 diff --git a/spec/ruby/core/binding/shared/clone.rb b/spec/ruby/core/binding/shared/clone.rb index 0e934ac1b52c39..1224b8ec7d0a15 100644 --- a/spec/ruby/core/binding/shared/clone.rb +++ b/spec/ruby/core/binding/shared/clone.rb @@ -31,4 +31,26 @@ 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 diff --git a/spec/ruby/core/method/clone_spec.rb b/spec/ruby/core/method/clone_spec.rb index 3fe4000fb798d7..b0eb5751a992fc 100644 --- a/spec/ruby/core/method/clone_spec.rb +++ b/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 - m1 = MethodSpecs::Methods.new.method(:foo) - m2 = m1.clone + it_behaves_like :method_dup, :clone - m1.should == m2 - m1.should_not equal(m2) - - m1.call.should == m2.call + it "preserves frozen status" do + method = Object.new.method(:method) + method.freeze + method.frozen?.should == true + method.clone.frozen?.should == true end end diff --git a/spec/ruby/core/method/dup_spec.rb b/spec/ruby/core/method/dup_spec.rb new file mode 100644 index 00000000000000..e3e29d8a68bae7 --- /dev/null +++ b/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 diff --git a/spec/ruby/core/method/shared/dup.rb b/spec/ruby/core/method/shared/dup.rb new file mode 100644 index 00000000000000..1a10b90689f7a8 --- /dev/null +++ b/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 diff --git a/spec/ruby/core/proc/clone_spec.rb b/spec/ruby/core/proc/clone_spec.rb index a1a1292654700e..7eca9c561e280f 100644 --- a/spec/ruby/core/proc/clone_spec.rb +++ b/spec/ruby/core/proc/clone_spec.rb @@ -3,4 +3,13 @@ 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 diff --git a/spec/ruby/core/proc/dup_spec.rb b/spec/ruby/core/proc/dup_spec.rb index 6da2f3080c8261..dd19b3c1e97497 100644 --- a/spec/ruby/core/proc/dup_spec.rb +++ b/spec/ruby/core/proc/dup_spec.rb @@ -3,4 +3,11 @@ 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 diff --git a/spec/ruby/core/proc/shared/dup.rb b/spec/ruby/core/proc/shared/dup.rb index 4480f3d0c98f87..c419a4078af7f5 100644 --- a/spec/ruby/core/proc/shared/dup.rb +++ b/spec/ruby/core/proc/shared/dup.rb @@ -15,4 +15,27 @@ 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 diff --git a/spec/ruby/core/unboundmethod/clone_spec.rb b/spec/ruby/core/unboundmethod/clone_spec.rb index 098ee61476bc0a..1e7fb18744e8fc 100644 --- a/spec/ruby/core/unboundmethod/clone_spec.rb +++ b/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 - um1 = UnboundMethodSpecs::Methods.instance_method(:foo) - um2 = um1.clone + it_behaves_like :unboundmethod_dup, :clone - (um1 == um2).should == true - um1.bind(UnboundMethodSpecs::Methods.new).call.should == um2.bind(UnboundMethodSpecs::Methods.new).call + it "preserves frozen status" do + method = Class.instance_method(:instance_method) + method.freeze + method.frozen?.should == true + method.clone.frozen?.should == true end end diff --git a/spec/ruby/core/unboundmethod/dup_spec.rb b/spec/ruby/core/unboundmethod/dup_spec.rb new file mode 100644 index 00000000000000..5a78dd8e3694ac --- /dev/null +++ b/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 diff --git a/spec/ruby/core/unboundmethod/shared/dup.rb b/spec/ruby/core/unboundmethod/shared/dup.rb new file mode 100644 index 00000000000000..943a7faaa397e7 --- /dev/null +++ b/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