Restore the original order of const_added and inherited callbacks

Originally, if a class was defined with the class keyword, the cref had a
const_added callback, and the superclass an inherited callback, const_added was
called first, and inherited second.

This was discussed in

    https://bugs.ruby-lang.org/issues/21143

and an attempt at changing this order was made.

While both constant assignment and inheritance have happened before these
callbacks are invoked, it was deemed nice to have the same order as in

    C = Class.new

This was mostly for alignment: In that last use case things happen at different
times and therefore the order of execution is kind of obvious, whereas when the
class keyword is involved, the order is opaque to the user and it is up to the
interpreter.

However, soon in

    https://bugs.ruby-lang.org/issues/21193

Matz decided to play safe and keep the existing order.

This reverts commits:

    de097fbe5f3df105bd2a26e72db06b0f5139bc1a
    de48e47ddf78aba02fd9623bc7ce685540a10743

class.c

@@ -1004,9 +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_raw(rb_cObject, id, klass);
+    rb_const_set(rb_cObject, id, klass);
     rb_class_inherited(super, klass);
-    rb_const_added(klass, id);
 
     return klass;
 }
@@ -1044,10 +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_raw(outer, id, klass);
+    rb_const_set(outer, id, klass);
     rb_class_inherited(super, klass);
-    rb_const_added(outer, id);
 
     return klass;
 }

internal/variable.h

@@ -16,8 +16,6 @@
 #include "shape.h"              /* for rb_shape_t */
 
 /* variable.c */
-void rb_const_added(VALUE klass, ID const_name);
-void rb_const_set_raw(VALUE klass, ID id, VALUE val);
 void rb_gc_mark_global_tbl(void);
 void rb_gc_update_global_tbl(void);
 size_t rb_generic_ivar_memsize(VALUE);

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

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

spec/ruby/core/class/inherited_spec.rb

@@ -1,6 +1,5 @@
 require_relative '../../spec_helper'
 require_relative 'fixtures/classes'
-require_relative 'fixtures/callback_order'
 
 describe "Class.inherited" do
 
@@ -99,16 +98,4 @@ describe "Class.inherited" do
     -> { 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, "constant", :location],
-        [:const_added, :Child, "constant", :location],
-      ]
-    end
-  end
 end

spec/ruby/optional/capi/class_spec.rb

@@ -385,17 +385,6 @@ describe "C-API Class function" do
         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", :location],
-          [:const_added, :Subclass, :location],
-        ]
-      end
-    end
   end
 
   describe "rb_define_class_id_under" do

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

@@ -101,14 +101,4 @@ class CApiClassSpecs
     module M
     end
   end
-
-  class Callbacks
-    def self.inherited(child)
-      ScratchPad << [:inherited, child.name, Object.const_source_location(child.name) ? :location : :unknown_location]
-    end
-
-    def self.const_added(const_name)
-      ScratchPad << [:const_added, const_name, const_source_location(const_name) ? :location : :unknown_location]
-    end
-  end
 end

test/ruby/test_settracefunc.rb

@@ -168,14 +168,14 @@ class TestSetTraceFunc < Test::Unit::TestCase
                  events.shift)
     assert_equal(["line", 4, __method__, self.class],
                  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(["c-call", 4, :inherited, Class],
+                 events.shift)
+    assert_equal(["c-return", 4, :inherited, Class],
+                 events.shift)
     assert_equal(["class", 4, nil, nil],
                  events.shift)
     assert_equal(["line", 5, nil, nil],
@@ -411,10 +411,10 @@ class TestSetTraceFunc < Test::Unit::TestCase
 
     [["c-return", 2, :add_trace_func, Thread],
      ["line", 3, __method__, self.class],
-     ["c-call", 3, :inherited, Class],
-     ["c-return", 3, :inherited, Class],
      ["c-call", 3, :const_added, Module],
      ["c-return", 3, :const_added, Module],
+     ["c-call", 3, :inherited, Class],
+     ["c-return", 3, :inherited, Class],
      ["class", 3, nil, nil],
      ["line", 4, nil, nil],
      ["c-call", 4, :method_added, Module],
@@ -558,10 +558,10 @@ class TestSetTraceFunc < Test::Unit::TestCase
      [: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", 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],
+     [:c_call,   7, "xyzzy", Class,       :inherited,       Object,      nil, :nothing],
+     [:c_return, 7, "xyzzy", Class,       :inherited,       Object,      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],

variable.c

@@ -2624,6 +2624,7 @@ rb_autoload(VALUE module, ID name, const char *feature)
 }
 
 static void const_set(VALUE klass, ID id, VALUE val);
+static void const_added(VALUE klass, ID const_name);
 
 struct autoload_arguments {
     VALUE module;
@@ -2732,7 +2733,7 @@ rb_autoload_str(VALUE module, ID name, VALUE feature)
     VALUE result = rb_mutex_synchronize(autoload_mutex, autoload_synchronized, (VALUE)&arguments);
 
     if (result == Qtrue) {
-        rb_const_added(module, name);
+        const_added(module, name);
     }
 }
 
@@ -3603,8 +3604,8 @@ set_namespace_path(VALUE named_namespace, VALUE namespace_path)
     RB_VM_LOCK_LEAVE();
 }
 
-void
-rb_const_added(VALUE klass, ID const_name)
+static void
+const_added(VALUE klass, ID const_name)
 {
     if (GET_VM()->running) {
         VALUE name = ID2SYM(const_name);
@@ -3679,17 +3680,11 @@ const_set(VALUE klass, ID id, VALUE val)
     }
 }
 
-void
-rb_const_set_raw(VALUE klass, ID id, VALUE val)
-{
-    const_set(klass, id, val);
-}
-
 void
 rb_const_set(VALUE klass, ID id, VALUE val)
 {
     const_set(klass, id, val);
-    rb_const_added(klass, id);
+    const_added(klass, id);
 }
 
 static struct autoload_data *

vm_insnhelper.c

@@ -5750,27 +5750,30 @@ 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 = rb_define_class_id(id, s);
+    VALUE c = declare_under(id, cbase, 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_const_set_raw(cbase, id, c);
     rb_class_inherited(s, c);
-    rb_const_added(cbase, id);
     return c;
 }
 
 static VALUE
 vm_declare_module(ID id, VALUE cbase)
 {
-    VALUE m = rb_module_new();
-    rb_set_class_path_string(m, cbase, rb_id2str(id));
-    rb_const_set(cbase, id, m);
-    return m;
+    /* new module declaration */
+    return declare_under(id, cbase, rb_module_new());
 }
 
 NORETURN(static void unmatched_redefinition(const char *type, VALUE cbase, ID id, VALUE old));