Invoke inherited callbacks before const_added

[Misc #21143]

Conceptually this makes sense and is more consistent with using
the `Name = Class.new(Superclass)` alternative method.

However the new class is still named before `inherited` is called.

Author: byroot

class.c

@@ -1004,8 +1004,8 @@ rb_define_class(const char *name, VALUE super)
     }
     klass = rb_define_class_id(id, super);
     rb_vm_register_global_object(klass);
-    rb_const_set(rb_cObject, id, klass);
     rb_class_inherited(super, klass);
+    rb_const_set(rb_cObject, id, klass);
 
     return klass;
 }
@@ -1043,8 +1043,8 @@ rb_define_class_id_under_no_pin(VALUE outer, ID id, VALUE super)
     }
     klass = rb_define_class_id(id, super);
     rb_set_class_path_string(klass, outer, rb_id2str(id));
-    rb_const_set(outer, id, klass);
     rb_class_inherited(super, klass);
+    rb_const_set(outer, id, klass);
 
     return klass;
 }

spec/ruby/core/class/fixtures/callback_order.rb

@@ -0,0 +1,21 @@
+module CoreClassSpecs
+  module Callbacks
+    class Base
+      def self.inherited(subclass)
+        subclass.const_set(:INHERITED_NAME, subclass.name)
+        ORDER << [:inherited, subclass, eval("defined?(#{subclass.name})")]
+        super
+      end
+    end
+
+    ORDER = []
+
+    def self.const_added(const_name)
+      ORDER << [:const_added, const_name, eval("defined?(#{const_name})")]
+      super
+    end
+
+    class Child < Base
+    end
+  end
+end

spec/ruby/core/class/inherited_spec.rb

@@ -1,5 +1,6 @@
 require_relative '../../spec_helper'
 require_relative 'fixtures/classes'
+require_relative 'fixtures/callback_order'
 
 describe "Class.inherited" do
 
@@ -98,4 +99,16 @@ class << top; protected :inherited; end
     -> { Class.new(top) }.should_not raise_error
   end
 
+  it "is invoked after the class is named when using class definition syntax" do
+    CoreClassSpecs::Callbacks::Child::INHERITED_NAME.should == "CoreClassSpecs::Callbacks::Child"
+  end
+
+  ruby_version_is "3.5" do # https://bugs.ruby-lang.org/issues/21143
+    it "is invoked before `const_added`" do
+      CoreClassSpecs::Callbacks::ORDER.should == [
+        [:inherited, CoreClassSpecs::Callbacks::Child, nil],
+        [:const_added, :Child, "constant"],
+      ]
+    end
+  end
 end

spec/ruby/optional/capi/class_spec.rb

@@ -383,6 +383,17 @@
         CApiClassSpecs.const_get(cls.name)
       }.should raise_error(NameError, /wrong constant name/)
     end
+
+    ruby_version_is "3.5" do
+      it "calls .inherited before .const_added" do
+        ScratchPad.record([])
+        @s.rb_define_class_id_under(CApiClassSpecs::Callbacks, :Subclass, CApiClassSpecs::Callbacks)
+        ScratchPad.recorded.should == [
+          [:inherited, "CApiClassSpecs::Callbacks::Subclass"],
+          [:const_added, :Subclass],
+        ]
+      end
+    end
   end
 
   describe "rb_define_class_id_under" do

spec/ruby/optional/capi/fixtures/class.rb

@@ -101,4 +101,14 @@ class B
     module M
     end
   end
+
+  class Callbacks
+    def self.inherited(child)
+      ScratchPad << [:inherited, child.name]
+    end
+
+    def self.const_added(const_name)
+      ScratchPad << [:const_added, const_name]
+    end
+  end
 end

test/ruby/test_settracefunc.rb

@@ -168,14 +168,14 @@ def test_class
                  events.shift)
     assert_equal(["line", 4, __method__, self.class],
                  events.shift)
-    assert_equal(["c-call", 4, :const_added, Module],
-                 events.shift)
-    assert_equal(["c-return", 4, :const_added, Module],
-                 events.shift)
     assert_equal(["c-call", 4, :inherited, Class],
                  events.shift)
     assert_equal(["c-return", 4, :inherited, Class],
                  events.shift)
+    assert_equal(["c-call", 4, :const_added, Module],
+                 events.shift)
+    assert_equal(["c-return", 4, :const_added, Module],
+                 events.shift)
     assert_equal(["class", 4, nil, nil],
                  events.shift)
     assert_equal(["line", 5, nil, nil],
@@ -411,10 +411,10 @@ def test_thread_trace
 
     [["c-return", 2, :add_trace_func, Thread],
      ["line", 3, __method__, self.class],
-     ["c-call", 3, :const_added, Module],
-     ["c-return", 3, :const_added, Module],
      ["c-call", 3, :inherited, Class],
      ["c-return", 3, :inherited, Class],
+     ["c-call", 3, :const_added, Module],
+     ["c-return", 3, :const_added, Module],
      ["class", 3, nil, nil],
      ["line", 4, nil, nil],
      ["c-call", 4, :method_added, Module],
@@ -558,10 +558,10 @@ def trace_by_tracepoint *trace_events
      [:line,     5, 'xyzzy', self.class,  method,           self,        :inner, :nothing],
      [:c_return, 4, "xyzzy", Array,       :reverse_each,    [1],         nil, [1]],
      [:line,     7, 'xyzzy', self.class,  method,           self,        :outer, :nothing],
-     [:c_call,   7, "xyzzy", Module,      :const_added,     TestSetTraceFunc, nil, :nothing],
-     [:c_return, 7, "xyzzy", Module,      :const_added,     TestSetTraceFunc, nil, nil],
      [:c_call,   7, "xyzzy", Class,       :inherited,       Object,      nil, :nothing],
      [:c_return, 7, "xyzzy", Class,       :inherited,       Object,      nil, nil],
+     [:c_call,   7, "xyzzy", Module,      :const_added,     TestSetTraceFunc, nil, :nothing],
+     [:c_return, 7, "xyzzy", Module,      :const_added,     TestSetTraceFunc, nil, nil],
      [:class,    7, "xyzzy", nil,         nil,              xyzzy.class, nil,    :nothing],
      [:line,     8, "xyzzy", nil,         nil,              xyzzy.class, nil,    :nothing],
      [:line,     9, "xyzzy", nil,         nil,              xyzzy.class, :XYZZY_outer, :nothing],

vm_insnhelper.c

@@ -5744,30 +5744,26 @@ vm_check_if_module(ID id, VALUE mod)
     }
 }
 
-static VALUE
-declare_under(ID id, VALUE cbase, VALUE c)
-{
-    rb_set_class_path_string(c, cbase, rb_id2str(id));
-    rb_const_set(cbase, id, c);
-    return c;
-}
-
 static VALUE
 vm_declare_class(ID id, rb_num_t flags, VALUE cbase, VALUE super)
 {
     /* new class declaration */
     VALUE s = VM_DEFINECLASS_HAS_SUPERCLASS_P(flags) ? super : rb_cObject;
-    VALUE c = declare_under(id, cbase, rb_define_class_id(id, s));
+    VALUE c = rb_define_class_id(id, s);
     rb_define_alloc_func(c, rb_get_alloc_func(c));
+    rb_set_class_path_string(c, cbase, rb_id2str(id));
     rb_class_inherited(s, c);
+    rb_const_set(cbase, id, c);
     return c;
 }
 
 static VALUE
 vm_declare_module(ID id, VALUE cbase)
 {
-    /* new module declaration */
-    return declare_under(id, cbase, rb_module_new());
+    VALUE m = rb_module_new();
+    rb_set_class_path_string(m, cbase, rb_id2str(id));
+    rb_const_set(cbase, id, m);
+    return m;
 }
 
 NORETURN(static void unmatched_redefinition(const char *type, VALUE cbase, ID id, VALUE old));