Use compile-time flag to indicate dynamic CREFs

The inline constant cache previously used `RCLASS_SINGLETON_P` to detect
"unstable" CREFs that need ic_cref stored and checked on every IC hit.
This caused the `class << self` pattern to create inline caches which
requires extra checks and can't optimize as well by the JITs.

We can avoid this by replacing the `RCLASS_SINGLETON_P` check with a
`VM_DEFINECLASS_FLAG_DYNAMIC_CREF` flag added to the defineclass
instruction at compile time to indicate a dynamic class scope and
specifically avoid setting it for the `class << self` pattern.

We can apply the same logic to fix dynamic CREF on `module (expr)::Foo`.
We can say that any class definition which uses only constant references
is stable, or at least as stable as the cref it was declared inside.

[Bug #20948]

Author: jhawthorn

compile.c

@@ -6090,6 +6090,23 @@ compile_const_prefix(rb_iseq_t *iseq, const NODE *const node,
     return COMPILE_OK;
 }
 
+static int
+cpath_const_p(const NODE *node)
+{
+    switch (nd_type(node)) {
+      case NODE_CONST:
+      case NODE_COLON3:
+        return TRUE;
+      case NODE_COLON2:
+        if (RNODE_COLON2(node)->nd_head) {
+            return cpath_const_p(RNODE_COLON2(node)->nd_head);
+        }
+        return TRUE;
+      default:
+        return FALSE;
+    }
+}
+
 static int
 compile_cpath(LINK_ANCHOR *const ret, rb_iseq_t *iseq, const NODE *cpath)
 {
@@ -6099,9 +6116,13 @@ compile_cpath(LINK_ANCHOR *const ret, rb_iseq_t *iseq, const NODE *cpath)
         return VM_DEFINECLASS_FLAG_SCOPED;
     }
     else if (nd_type_p(cpath, NODE_COLON2) && RNODE_COLON2(cpath)->nd_head) {
-        /* Bar::Foo */
+        /* Bar::Foo or expr::Foo */
         NO_CHECK(COMPILE(ret, "nd_else->nd_head", RNODE_COLON2(cpath)->nd_head));
-        return VM_DEFINECLASS_FLAG_SCOPED;
+        int flags = VM_DEFINECLASS_FLAG_SCOPED;
+        if (!cpath_const_p(RNODE_COLON2(cpath)->nd_head)) {
+            flags |= VM_DEFINECLASS_FLAG_DYNAMIC_CREF;
+        }
+        return flags;
     }
     else {
         /* class at cbase Foo */
@@ -11477,9 +11498,20 @@ iseq_compile_each0(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const no
         CHECK(COMPILE(ret, "sclass#recv", RNODE_SCLASS(node)->nd_recv));
         ADD_INSN (ret, node, putnil);
         CONST_ID(singletonclass, "singletonclass");
+
+        /* `class << self` in a class body and `class << Foo` (constant
+           receiver) are stable. All other forms are potentially dynamic. */
+        int sclass_flags = VM_DEFINECLASS_TYPE_SINGLETON_CLASS;
+        const NODE *recv = RNODE_SCLASS(node)->nd_recv;
+        if (!(nd_type_p(recv, NODE_SELF) &&
+              ISEQ_BODY(iseq)->type == ISEQ_TYPE_CLASS) &&
+            !cpath_const_p(recv)) {
+            sclass_flags |= VM_DEFINECLASS_FLAG_DYNAMIC_CREF;
+        }
+
         ADD_INSN3(ret, node, defineclass,
                   ID2SYM(singletonclass), singleton_class,
-                  INT2FIX(VM_DEFINECLASS_TYPE_SINGLETON_CLASS));
+                  INT2FIX(sclass_flags));
         RB_OBJ_WRITTEN(iseq, Qundef, (VALUE)singleton_class);
 
         if (popped) {

eval_intern.h

@@ -172,9 +172,10 @@ rb_ec_tag_jump(const rb_execution_context_t *ec, enum ruby_tag_type st)
 
 /* CREF operators */
 
-#define CREF_FL_PUSHED_BY_EVAL IMEMO_FL_USER1
-#define CREF_FL_OMOD_SHARED    IMEMO_FL_USER2
-#define CREF_FL_SINGLETON      IMEMO_FL_USER3
+#define CREF_FL_PUSHED_BY_EVAL    IMEMO_FL_USER1
+#define CREF_FL_OMOD_SHARED      IMEMO_FL_USER2
+#define CREF_FL_SINGLETON        IMEMO_FL_USER3
+#define CREF_FL_DYNAMIC_CREF IMEMO_FL_USER4
 
 static inline int CREF_SINGLETON(const rb_cref_t *cref);
 
@@ -260,6 +261,18 @@ CREF_OMOD_SHARED_SET(rb_cref_t *cref)
     cref->flags |= CREF_FL_OMOD_SHARED;
 }
 
+static inline int
+CREF_DYNAMIC(const rb_cref_t *cref)
+{
+    return cref->flags & CREF_FL_DYNAMIC_CREF;
+}
+
+static inline void
+CREF_DYNAMIC_SET(rb_cref_t *cref)
+{
+    cref->flags |= CREF_FL_DYNAMIC_CREF;
+}
+
 static inline void
 CREF_OMOD_SHARED_UNSET(rb_cref_t *cref)
 {

insns.def

@@ -811,10 +811,16 @@ defineclass
 
     rb_iseq_check(class_iseq);
 
+    rb_cref_t *cref = vm_cref_push(ec, klass, NULL, FALSE, FALSE);
+
+    if (VM_DEFINECLASS_DYNAMIC_CREF_P(flags)) {
+        CREF_DYNAMIC_SET(cref);
+    }
+
     /* enter scope */
     vm_push_frame(ec, class_iseq, VM_FRAME_MAGIC_CLASS | VM_ENV_FLAG_LOCAL, klass,
                   GC_GUARDED_PTR(box),
-                  (VALUE)vm_cref_push(ec, klass, NULL, FALSE, FALSE),
+                  (VALUE)cref,
                   ISEQ_BODY(class_iseq)->iseq_encoded, GET_SP(),
                   ISEQ_BODY(class_iseq)->local_table_size,
                   ISEQ_BODY(class_iseq)->stack_max);

prism_compile.c

@@ -1377,16 +1377,37 @@ pm_new_child_iseq(rb_iseq_t *iseq, pm_scope_node_t *node, VALUE name, const rb_i
     return ret_iseq;
 }
 
+static int
+pm_cpath_const_p(const pm_node_t *node)
+{
+    switch (PM_NODE_TYPE(node)) {
+      case PM_CONSTANT_READ_NODE:
+        return TRUE;
+      case PM_CONSTANT_PATH_NODE:
+        {
+            const pm_node_t *parent = ((const pm_constant_path_node_t *) node)->parent;
+            if (!parent) return TRUE; /* ::Foo */
+            return pm_cpath_const_p(parent);
+        }
+      default:
+        return FALSE;
+    }
+}
+
 static int
 pm_compile_class_path(rb_iseq_t *iseq, const pm_node_t *node, const pm_node_location_t *node_location, LINK_ANCHOR *const ret, bool popped, pm_scope_node_t *scope_node)
 {
     if (PM_NODE_TYPE_P(node, PM_CONSTANT_PATH_NODE)) {
         const pm_node_t *parent = ((const pm_constant_path_node_t *) node)->parent;
 
         if (parent) {
-            /* Bar::Foo */
+            /* Bar::Foo or expr::Foo */
             PM_COMPILE(parent);
-            return VM_DEFINECLASS_FLAG_SCOPED;
+            int flags = VM_DEFINECLASS_FLAG_SCOPED;
+            if (!pm_cpath_const_p(parent)) {
+                flags |= VM_DEFINECLASS_FLAG_DYNAMIC_CREF;
+            }
+            return flags;
         }
         else {
             /* toplevel class ::Foo */
@@ -10342,7 +10363,17 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
 
         ID singletonclass;
         CONST_ID(singletonclass, "singletonclass");
-        PUSH_INSN3(ret, location, defineclass, ID2SYM(singletonclass), child_iseq, INT2FIX(VM_DEFINECLASS_TYPE_SINGLETON_CLASS));
+
+        /* `class << self` in a class body and `class << Foo` (constant
+           receiver) are stable. All other forms are potentially dynamic. */
+        int sclass_flags = VM_DEFINECLASS_TYPE_SINGLETON_CLASS;
+        if (!(PM_NODE_TYPE_P(cast->expression, PM_SELF_NODE) &&
+              ISEQ_BODY(iseq)->type == ISEQ_TYPE_CLASS) &&
+            !pm_cpath_const_p(cast->expression)) {
+            sclass_flags |= VM_DEFINECLASS_FLAG_DYNAMIC_CREF;
+        }
+
+        PUSH_INSN3(ret, location, defineclass, ID2SYM(singletonclass), child_iseq, INT2FIX(sclass_flags));
 
         if (popped) PUSH_INSN(ret, location, pop);
         RB_OBJ_WRITTEN(iseq, Qundef, (VALUE) child_iseq);

test/ruby/test_class.rb

@@ -733,6 +733,29 @@ def xyzzy
     }
   end
 
+  def test_dynamic_module_cpath_constant_namespace # [Bug #20948]
+    assert_separately([], <<~'RUBY')
+      module M1
+        module Foo
+          X = 1
+        end
+      end
+
+      module M2
+        module Foo
+          X = 2
+        end
+      end
+
+      results = [M1, M2].map do
+        module it::Foo
+          X
+        end
+      end
+      assert_equal([1, 2], results)
+    RUBY
+  end
+
   def test_namescope_error_message
     m = Module.new
     o = m.module_eval "class A\u{3042}; self; end.new"

test/ruby/test_yjit.rb

@@ -547,7 +547,7 @@ def get_foo
   end
 
   def test_opt_getconstant_path_slowpath
-    assert_compiles(<<~RUBY, exits: { opt_getconstant_path: 1 }, result: [42, 42, 1, 1], call_threshold: 2)
+    assert_compiles(<<~RUBY, result: [42, 42, 1, 1], call_threshold: 2)
       class A
         FOO = 42
         class << self

vm_core.h

@@ -1243,9 +1243,12 @@ typedef enum {
 #define VM_DEFINECLASS_TYPE(x) ((rb_vm_defineclass_type_t)(x) & VM_DEFINECLASS_TYPE_MASK)
 #define VM_DEFINECLASS_FLAG_SCOPED         0x08
 #define VM_DEFINECLASS_FLAG_HAS_SUPERCLASS 0x10
+#define VM_DEFINECLASS_FLAG_DYNAMIC_CREF  0x20
 #define VM_DEFINECLASS_SCOPED_P(x) ((x) & VM_DEFINECLASS_FLAG_SCOPED)
 #define VM_DEFINECLASS_HAS_SUPERCLASS_P(x) \
     ((x) & VM_DEFINECLASS_FLAG_HAS_SUPERCLASS)
+#define VM_DEFINECLASS_DYNAMIC_CREF_P(x) \
+    ((x) & VM_DEFINECLASS_FLAG_DYNAMIC_CREF)
 
 /* iseq.c */
 RUBY_SYMBOL_EXPORT_BEGIN

vm_insnhelper.c

@@ -989,14 +989,14 @@ vm_get_const_key_cref(const VALUE *ep)
     const rb_cref_t *key_cref = cref;
 
     while (cref) {
-        if (RCLASS_SINGLETON_P(CREF_CLASS(cref)) ||
-                RCLASS_CLONED_P(CREF_CLASS(cref)) ) {
+        if (CREF_DYNAMIC(cref) ||
+                RCLASS_CLONED_P(CREF_CLASS(cref))) {
             return key_cref;
         }
         cref = CREF_NEXT(cref);
     }
 
-    /* does not include singleton class */
+    /* no dynamic singleton class or cloned class found */
     return NULL;
 }