ruby-changes:59613
From: Jeremy <ko1@a...>
Date: Sat, 4 Jan 2020 13:13:39 +0900 (JST)
Subject: [ruby-changes:59613] 04eb7c7e46 (master): Call initialize_clone with freeze: false if clone called with freeze: false
https://git.ruby-lang.org/ruby.git/commit/?id=04eb7c7e46 From 04eb7c7e462d1fcbab9efc7022c1b43636ab878a Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@j...> Date: Sun, 25 Aug 2019 21:11:46 -0700 Subject: Call initialize_clone with freeze: false if clone called with freeze: false This makes it possible to initialize_clone to correctly not freeze internal state if the freeze: false keyword is passed to clone. If clone is called with freeze: true or no keyword, do not pass a second argument to initialize_clone to keep backwards compatibility. This makes it so that external libraries that override initialize_clone but do not support the freeze keyword will fail with ArgumentError if passing freeze: false to clone. I think that is better than the current behavior, which succeeds but results in an unfrozen object with frozen internals. Fix related issues in set and delegate in stdlib. Fixes [Bug #14266] diff --git a/lib/delegate.rb b/lib/delegate.rb index 8c176dc..0bbe211 100644 --- a/lib/delegate.rb +++ b/lib/delegate.rb @@ -211,8 +211,8 @@ class Delegator < BasicObject https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L211 end end - def initialize_clone(obj) # :nodoc: - self.__setobj__(obj.__getobj__.clone) + def initialize_clone(obj, freeze: true) # :nodoc: + self.__setobj__(obj.__getobj__.clone(freeze: freeze)) end def initialize_dup(obj) # :nodoc: self.__setobj__(obj.__getobj__.dup) diff --git a/lib/set.rb b/lib/set.rb index e7d1be4..6841155 100644 --- a/lib/set.rb +++ b/lib/set.rb @@ -137,9 +137,9 @@ class Set https://github.com/ruby/ruby/blob/trunk/lib/set.rb#L137 end # Clone internal hash. - def initialize_clone(orig) + def initialize_clone(orig, freeze: true) super - @hash = orig.instance_variable_get(:@hash).clone + @hash = orig.instance_variable_get(:@hash).clone(freeze: freeze) end def freeze # :nodoc: diff --git a/object.c b/object.c index f02b45a..0080c35 100644 --- a/object.c +++ b/object.c @@ -476,11 +476,25 @@ mutable_obj_clone(VALUE obj, int kwfreeze) https://github.com/ruby/ruby/blob/trunk/object.c#L476 } init_copy(clone, obj); - rb_funcall(clone, id_init_clone, 1, obj); if (kwfreeze) { + rb_funcall(clone, id_init_clone, 1, obj); RBASIC(clone)->flags |= RBASIC(obj)->flags & FL_FREEZE; } + else { + static VALUE freeze_false_hash; + VALUE argv[2]; + if (!freeze_false_hash) { + freeze_false_hash = rb_hash_new(); + rb_hash_aset(freeze_false_hash, ID2SYM(rb_intern("freeze")), Qfalse); + rb_obj_freeze(freeze_false_hash); + rb_gc_register_mark_object(freeze_false_hash); + } + + argv[0] = obj; + argv[1] = freeze_false_hash; + rb_funcallv_kw(clone, id_init_clone, 2, argv, RB_PASS_CALLED_KEYWORDS); + } return clone; } @@ -637,10 +651,10 @@ rb_obj_init_copy(VALUE obj, VALUE orig) https://github.com/ruby/ruby/blob/trunk/object.c#L651 /*! * :nodoc: *-- - * Default implementation of \c #initialize_dup and \c #initialize_clone + * Default implementation of \c #initialize_dup * * \param[in,out] obj the receiver being initialized - * \param[in] orig the object to be dup or cloned from. + * \param[in] orig the object to be dup from. *++ **/ VALUE @@ -650,6 +664,27 @@ rb_obj_init_dup_clone(VALUE obj, VALUE orig) https://github.com/ruby/ruby/blob/trunk/object.c#L664 return obj; } +/*! + * :nodoc: + *-- + * Default implementation of \c #initialize_clone + * + * \param[in] The number of arguments + * \param[in] The array of arguments + * \param[in] obj the receiver being initialized + *++ + **/ +static VALUE +rb_obj_init_clone(int argc, VALUE *argv, VALUE obj) +{ + VALUE orig, opts; + rb_scan_args(argc, argv, "1:", &orig, &opts); + /* Ignore a freeze keyword */ + if (argc == 2) (void)freeze_opt(1, &opts); + rb_funcall(obj, id_init_copy, 1, orig); + return obj; +} + /** * call-seq: * obj.to_s -> string @@ -1968,10 +2003,11 @@ rb_mod_initialize(VALUE module) https://github.com/ruby/ruby/blob/trunk/object.c#L2003 /* :nodoc: */ static VALUE -rb_mod_initialize_clone(VALUE clone, VALUE orig) +rb_mod_initialize_clone(int argc, VALUE* argv, VALUE clone) { - VALUE ret; - ret = rb_obj_init_dup_clone(clone, orig); + VALUE ret, orig, opts; + rb_scan_args(argc, argv, "1:", &orig, &opts); + ret = rb_obj_init_clone(argc, argv, clone); if (OBJ_FROZEN(orig)) rb_class_name(clone); return ret; @@ -4579,7 +4615,7 @@ InitVM_Object(void) https://github.com/ruby/ruby/blob/trunk/object.c#L4615 rb_define_method(rb_mKernel, "then", rb_obj_yield_self, 0); rb_define_method(rb_mKernel, "initialize_copy", rb_obj_init_copy, 1); rb_define_method(rb_mKernel, "initialize_dup", rb_obj_init_dup_clone, 1); - rb_define_method(rb_mKernel, "initialize_clone", rb_obj_init_dup_clone, 1); + rb_define_method(rb_mKernel, "initialize_clone", rb_obj_init_clone, -1); rb_define_method(rb_mKernel, "taint", rb_obj_taint, 0); rb_define_method(rb_mKernel, "tainted?", rb_obj_tainted, 0); @@ -4666,7 +4702,7 @@ InitVM_Object(void) https://github.com/ruby/ruby/blob/trunk/object.c#L4702 rb_define_alloc_func(rb_cModule, rb_module_s_alloc); rb_define_method(rb_cModule, "initialize", rb_mod_initialize, 0); - rb_define_method(rb_cModule, "initialize_clone", rb_mod_initialize_clone, 1); + rb_define_method(rb_cModule, "initialize_clone", rb_mod_initialize_clone, -1); rb_define_method(rb_cModule, "instance_methods", rb_class_instance_methods, -1); /* in class.c */ rb_define_method(rb_cModule, "public_instance_methods", rb_class_public_instance_methods, -1); /* in class.c */ diff --git a/test/ruby/test_module.rb b/test/ruby/test_module.rb index 2e7e580..9373c48 100644 --- a/test/ruby/test_module.rb +++ b/test/ruby/test_module.rb @@ -2491,6 +2491,12 @@ class TestModule < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_module.rb#L2491 assert_equal :M2, CloneTestC2.new.foo, '[Bug #15877]' end + def test_clone_freeze + m = Module.new.freeze + assert_predicate m.clone, :frozen? + assert_not_predicate m.clone(freeze: false), :frozen? + end + private def assert_top_method_is_private(method) diff --git a/test/ruby/test_object.rb b/test/ruby/test_object.rb index add5b9f..bfc7d7d 100644 --- a/test/ruby/test_object.rb +++ b/test/ruby/test_object.rb @@ -75,6 +75,28 @@ class TestObject < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_object.rb#L75 assert_raise_with_message(ArgumentError, /\u{1f4a9}/) do Object.new.clone(freeze: x) end + + c = Class.new do + attr_reader :f + end + o = c.new + f = true + def o.initialize_clone(_, freeze: true) + @f = freeze + super + end + clone = o.clone + assert_kind_of c, clone + assert_equal true, clone.f + clone = o.clone(freeze: false) + assert_kind_of c, clone + assert_equal false, clone.f + + def o.initialize_clone(_) + super + end + assert_kind_of c, o.clone + assert_raise(ArgumentError) { o.clone(freeze: false) } end def test_init_dupclone diff --git a/test/test_delegate.rb b/test/test_delegate.rb index 303cd16..80bf416 100644 --- a/test/test_delegate.rb +++ b/test/test_delegate.rb @@ -50,6 +50,21 @@ class TestDelegateClass < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/test_delegate.rb#L50 assert_equal(SimpleDelegator,simple.clone.class) end + def test_simpledelegator_clone + simple=SimpleDelegator.new([]) + simple.freeze + + clone = simple.clone + assert_predicate clone, :frozen? + assert_predicate clone.__getobj__, :frozen? + assert_equal true, Kernel.instance_method(:frozen?).bind(clone).call + + clone = simple.clone(freeze: false) + assert_not_predicate clone, :frozen? + assert_not_predicate clone.__getobj__, :frozen? + assert_equal false, Kernel.instance_method(:frozen?).bind(clone).call + end + class Object def m :o diff --git a/test/test_set.rb b/test/test_set.rb index b0f669c..68ee7ce 100644 --- a/test/test_set.rb +++ b/test/test_set.rb @@ -730,6 +730,17 @@ class TC_Set < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/test_set.rb#L730 } end + def test_freeze_clone_false + set1 = Set[1,2,3] + set1.freeze + set2 = set1.clone(freeze: false) + + assert_not_predicate set2, :frozen? + set2.add 5 + assert_equal Set[1,2,3,5], set2 + assert_equal Set[1,2,3], set1 + end + def test_inspect set1 = Set[1, 2] assert_equal('#<Set: {1, 2}>', set1.inspect) -- cgit v0.10.2 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/