merge revision(s) ebb96fa880: [Backport #17321]

Fix singleton class cloning

	Before this commit, `clone` gave different results depending on whether the original object
	had an attached singleton class or not.

	Consider the following setup:
	```
	class Foo; end
	Foo.singleton_class.define_method(:foo) {}

	obj = Foo.new

	obj.singleton_class if $call_singleton

	clone = obj.clone
	```

	When `$call_singleton = false`, neither `obj.singleton_class.singleton_class` nor
	`clone.singleton_class.singleton_class` own any methods.

	However, when `$call_singleton = true`, `clone.singleton_class.singleton_class` would own a copy of
	`foo` from `Foo.singleton_class`, even though `obj.singleton_class.singleton_class` does not.

	The latter case is unexpected and results in a visibly different clone, depending on if the original object
	had an attached class or not.

	Co-authored-by: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>
	---
	 class.c                 | 31 ++++++++++++++++++++++---------
	 test/ruby/test_class.rb | 47 +++++++++++++++++++++++++++++++++++++++++++++++
	 2 files changed, 69 insertions(+), 9 deletions(-)

class.c

@@ -32,6 +32,9 @@
 
 #define id_attached id__attached__
 
+#define METACLASS_OF(k) RBASIC(k)->klass
+#define SET_METACLASS_OF(k, cls) RBASIC_SET_CLASS(k, cls)
+
 void
 rb_class_subclass_add(VALUE super, VALUE klass)
 {
@@ -372,22 +375,35 @@ rb_singleton_class_clone(VALUE obj)
     return rb_singleton_class_clone_and_attach(obj, Qundef);
 }
 
+// Clone and return the singleton class of `obj` if it has been created and is attached to `obj`.
 VALUE
 rb_singleton_class_clone_and_attach(VALUE obj, VALUE attach)
 {
     const VALUE klass = RBASIC(obj)->klass;
 
-    if (!FL_TEST(klass, FL_SINGLETON))
-	return klass;
+    // Note that `rb_singleton_class()` can create situations where `klass` is
+    // attached to an object other than `obj`. In which case `obj` does not have
+    // a material singleton class attached yet and there is no singleton class
+    // to clone.
+    if (!(FL_TEST(klass, FL_SINGLETON) && rb_attr_get(klass, id_attached) == obj)) {
+        // nothing to clone
+        return klass;
+    }
     else {
 	/* copy singleton(unnamed) class */
+        bool klass_of_clone_is_new;
 	VALUE clone = class_alloc(RBASIC(klass)->flags, 0);
 
 	if (BUILTIN_TYPE(obj) == T_CLASS) {
+            klass_of_clone_is_new = true;
 	    RBASIC_SET_CLASS(clone, clone);
 	}
 	else {
-	    RBASIC_SET_CLASS(clone, rb_singleton_class_clone(klass));
+            VALUE klass_metaclass_clone = rb_singleton_class_clone(klass);
+            // When `METACLASS_OF(klass) == klass_metaclass_clone`, it means the
+            // recursive call did not clone `METACLASS_OF(klass)`.
+            klass_of_clone_is_new = (METACLASS_OF(klass) != klass_metaclass_clone);
+            RBASIC_SET_CLASS(clone, klass_metaclass_clone);
 	}
 
 	RCLASS_SET_SUPER(clone, RCLASS_SUPER(klass));
@@ -411,7 +427,9 @@ rb_singleton_class_clone_and_attach(VALUE obj, VALUE attach)
 	    arg.new_klass = clone;
 	    rb_id_table_foreach(RCLASS_M_TBL(klass), clone_method_i, &arg);
 	}
-	rb_singleton_class_attached(RBASIC(clone)->klass, clone);
+        if (klass_of_clone_is_new) {
+            rb_singleton_class_attached(RBASIC(clone)->klass, clone);
+        }
 	FL_SET(clone, FL_SINGLETON);
 
 	return clone;
@@ -433,11 +451,6 @@ rb_singleton_class_attached(VALUE klass, VALUE obj)
     }
 }
 
-
-
-#define METACLASS_OF(k) RBASIC(k)->klass
-#define SET_METACLASS_OF(k, cls) RBASIC_SET_CLASS(k, cls)
-
 /*!
  * whether k is a meta^(n)-class of Class class
  * @retval 1 if \a k is a meta^(n)-class of Class class (n >= 0)

test/ruby/test_class.rb

@@ -483,6 +483,53 @@ class TestClass < Test::Unit::TestCase
     assert_equal(:foo, d.foo)
   end
 
+  def test_clone_singleton_class_exists
+    klass = Class.new do
+      def self.bar; :bar; end
+    end
+
+    o = klass.new
+    o.singleton_class
+    clone = o.clone
+
+    assert_empty(o.singleton_class.instance_methods(false))
+    assert_empty(clone.singleton_class.instance_methods(false))
+    assert_empty(o.singleton_class.singleton_class.instance_methods(false))
+    assert_empty(clone.singleton_class.singleton_class.instance_methods(false))
+  end
+
+  def test_clone_when_singleton_class_of_singleton_class_exists
+    klass = Class.new do
+      def self.bar; :bar; end
+    end
+
+    o = klass.new
+    o.singleton_class.singleton_class
+    clone = o.clone
+
+    assert_empty(o.singleton_class.instance_methods(false))
+    assert_empty(clone.singleton_class.instance_methods(false))
+    assert_empty(o.singleton_class.singleton_class.instance_methods(false))
+    assert_empty(clone.singleton_class.singleton_class.instance_methods(false))
+  end
+
+  def test_clone_when_method_exists_on_singleton_class_of_singleton_class
+    klass = Class.new do
+      def self.bar; :bar; end
+    end
+
+    o = klass.new
+    o.singleton_class.singleton_class.define_method(:s2_method) { :s2 }
+    clone = o.clone
+
+    assert_empty(o.singleton_class.instance_methods(false))
+    assert_empty(clone.singleton_class.instance_methods(false))
+    assert_equal(:s2, o.singleton_class.s2_method)
+    assert_equal(:s2, clone.singleton_class.s2_method)
+    assert_equal([:s2_method], o.singleton_class.singleton_class.instance_methods(false))
+    assert_equal([:s2_method], clone.singleton_class.singleton_class.instance_methods(false))
+  end
+
   def test_singleton_class_p
     feature7609 = '[ruby-core:51087] [Feature #7609]'
     assert_predicate(self.singleton_class, :singleton_class?, feature7609)

version.h

@@ -2,7 +2,7 @@
 # define RUBY_VERSION_MINOR RUBY_API_VERSION_MINOR
 #define RUBY_VERSION_TEENY 3
 #define RUBY_RELEASE_DATE RUBY_RELEASE_YEAR_STR"-"RUBY_RELEASE_MONTH_STR"-"RUBY_RELEASE_DAY_STR
-#define RUBY_PATCHLEVEL 168
+#define RUBY_PATCHLEVEL 169
 
 #define RUBY_RELEASE_YEAR 2021
 #define RUBY_RELEASE_MONTH 3