Fix class instance variable inside namespaces

Now that classes fields are delegated to an object with its own
shape_id, we no longer need to mark all classes as TOO_COMPLEX.

Author: byroot

imemo.c

@@ -155,11 +155,14 @@ rb_imemo_class_fields_clone(VALUE fields_obj)
 
     if (rb_shape_too_complex_p(shape_id)) {
         clone = rb_imemo_class_fields_new_complex(CLASS_OF(fields_obj), 0);
+        RBASIC_SET_SHAPE_ID(clone, shape_id);
+
         st_table *src_table = rb_imemo_class_fields_complex_tbl(fields_obj);
         st_replace(rb_imemo_class_fields_complex_tbl(clone), src_table);
     }
     else {
         clone = imemo_class_fields_new(CLASS_OF(fields_obj), RSHAPE_CAPACITY(shape_id));
+        RBASIC_SET_SHAPE_ID(clone, shape_id);
         MEMCPY(rb_imemo_class_fields_ptr(clone), rb_imemo_class_fields_ptr(fields_obj), VALUE, RSHAPE_LEN(shape_id));
     }
 

internal/class.h

@@ -403,10 +403,6 @@ RCLASS_EXT_WRITABLE_LOOKUP(VALUE obj, const rb_namespace_t *ns)
     if (ext)
         return ext;
 
-    if (!rb_shape_obj_too_complex_p(obj)) {
-        rb_evict_ivars_to_hash(obj); // fallback to ivptr for ivars from shapes
-    }
-
     RB_VM_LOCKING() {
         // re-check the classext is not created to avoid the multi-thread race
         ext = RCLASS_EXT_TABLE_LOOKUP_INTERNAL(obj, ns);
@@ -525,50 +521,44 @@ RCLASS_WRITE_SUPER(VALUE klass, VALUE super)
 }
 
 static inline VALUE
-RCLASS_FIELDS_OBJ(VALUE obj)
-{
-    RUBY_ASSERT(RB_TYPE_P(obj, RUBY_T_CLASS) || RB_TYPE_P(obj, RUBY_T_MODULE));
-    return RCLASSEXT_FIELDS_OBJ(RCLASS_EXT_READABLE(obj));
-}
-
-static inline VALUE
-RCLASS_ENSURE_FIELDS_OBJ(VALUE obj)
+RCLASS_WRITABLE_ENSURE_FIELDS_OBJ(VALUE obj)
 {
     RUBY_ASSERT(RB_TYPE_P(obj, RUBY_T_CLASS) || RB_TYPE_P(obj, RUBY_T_MODULE));
-    rb_classext_t *ext = RCLASS_EXT_READABLE(obj);
+    rb_classext_t *ext = RCLASS_EXT_WRITABLE(obj);
     if (!ext->fields_obj) {
         RB_OBJ_WRITE(obj, &ext->fields_obj, rb_imemo_class_fields_new(obj, 1));
     }
     return ext->fields_obj;
 }
 
-static inline VALUE
-RCLASS_WRITABLE_FIELDS_OBJ(VALUE obj)
+static inline void
+RCLASSEXT_SET_FIELDS_OBJ(VALUE obj, rb_classext_t *ext, VALUE fields_obj)
 {
     RUBY_ASSERT(RB_TYPE_P(obj, RUBY_T_CLASS) || RB_TYPE_P(obj, RUBY_T_MODULE));
-    return RCLASSEXT_FIELDS_OBJ(RCLASS_EXT_WRITABLE(obj));
+    RB_OBJ_WRITE(obj, &ext->fields_obj, fields_obj);
 }
 
-static inline void
-RCLASSEXT_SET_FIELDS_OBJ(VALUE obj, rb_classext_t *ext, VALUE fields_obj)
+static inline VALUE
+RCLASS_WRITABLE_FIELDS_OBJ(VALUE obj)
 {
     RUBY_ASSERT(RB_TYPE_P(obj, RUBY_T_CLASS) || RB_TYPE_P(obj, RUBY_T_MODULE));
-    RB_OBJ_WRITE(obj, &ext->fields_obj, fields_obj);
+    return RCLASSEXT_FIELDS_OBJ(RCLASS_EXT_WRITABLE(obj));
 }
 
 static inline void
-RCLASS_SET_FIELDS_OBJ(VALUE obj, VALUE fields_obj)
+RCLASS_WRITABLE_SET_FIELDS_OBJ(VALUE obj, VALUE fields_obj)
 {
     RUBY_ASSERT(RB_TYPE_P(obj, RUBY_T_CLASS) || RB_TYPE_P(obj, RUBY_T_MODULE));
 
-    RCLASSEXT_SET_FIELDS_OBJ(obj, RCLASS_EXT_PRIME(obj), fields_obj);
+    RCLASSEXT_SET_FIELDS_OBJ(obj, RCLASS_EXT_WRITABLE(obj), fields_obj);
 }
 
 static inline uint32_t
 RCLASS_FIELDS_COUNT(VALUE obj)
 {
     RUBY_ASSERT(RB_TYPE_P(obj, RUBY_T_CLASS) || RB_TYPE_P(obj, RUBY_T_MODULE));
-    VALUE fields_obj = RCLASS_FIELDS_OBJ(obj);
+
+    VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
     if (fields_obj) {
         if (rb_shape_obj_too_complex_p(fields_obj)) {
             return (uint32_t)rb_st_table_size(rb_imemo_class_fields_complex_tbl(fields_obj));

namespace.c

@@ -450,9 +450,6 @@ namespace_initialize(VALUE namespace)
     // If a code in the namespace adds a constant, the constant will be visible even from root/main.
     RCLASS_SET_PRIME_CLASSEXT_WRITABLE(namespace, true);
 
-    // fallback to ivptr for ivars from shapes to manipulate the constant table
-    rb_evict_ivars_to_hash(namespace);
-
     // Get a clean constant table of Object even by writable one
     // because ns was just created, so it has not touched any constants yet.
     object_classext = RCLASS_EXT_WRITABLE_IN_NS(rb_cObject, ns);

shape.c

@@ -397,7 +397,7 @@ rb_obj_shape_id(VALUE obj)
     }
 
     if (BUILTIN_TYPE(obj) == T_CLASS || BUILTIN_TYPE(obj) == T_MODULE) {
-        VALUE fields_obj = RCLASS_FIELDS_OBJ(obj);
+        VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
         if (fields_obj) {
             return RBASIC_SHAPE_ID(fields_obj);
         }

test/ruby/namespace/instance_variables.rb

@@ -0,0 +1,21 @@
+class String
+  class << self
+    attr_reader :str_ivar1
+
+    def str_ivar2
+      @str_ivar2
+    end
+  end
+
+  @str_ivar1 = 111
+  @str_ivar2 = 222
+end
+
+class StringDelegator < BasicObject
+private
+  def method_missing(...)
+    ::String.public_send(...)
+  end
+end
+
+StringDelegatorObj = StringDelegator.new

test/ruby/test_namespace.rb

@@ -222,6 +222,26 @@ def Target.foo
     end;
   end
 
+  def test_instance_variable
+    pend unless Namespace.enabled?
+
+    @n.require_relative('namespace/instance_variables')
+
+    assert_equal [], String.instance_variables
+    assert_equal [:@str_ivar1, :@str_ivar2], @n::StringDelegatorObj.instance_variables
+    assert_equal 111, @n::StringDelegatorObj.str_ivar1
+    assert_equal 222, @n::StringDelegatorObj.str_ivar2
+    assert_equal 222, @n::StringDelegatorObj.instance_variable_get(:@str_ivar2)
+
+    @n::StringDelegatorObj.instance_variable_set(:@str_ivar3, 333)
+    assert_equal 333, @n::StringDelegatorObj.instance_variable_get(:@str_ivar3)
+    @n::StringDelegatorObj.remove_instance_variable(:@str_ivar1)
+    assert_nil @n::StringDelegatorObj.str_ivar1
+    assert_equal [:@str_ivar2, :@str_ivar3], @n::StringDelegatorObj.instance_variables
+
+    assert_equal [], String.instance_variables
+  end
+
   def test_methods_added_in_namespace_are_invisible_globally
     pend unless Namespace.enabled?
 

variable.c

@@ -1307,7 +1307,7 @@ rb_obj_field_get(VALUE obj, shape_id_t target_shape_id)
 
     if (BUILTIN_TYPE(obj) == T_CLASS || BUILTIN_TYPE(obj) == T_MODULE) {
         ASSERT_vm_locking();
-        VALUE field_obj = RCLASS_FIELDS_OBJ(obj);
+        VALUE field_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
         if (field_obj) {
             return rb_obj_field_get(field_obj, target_shape_id);
         }
@@ -1375,7 +1375,7 @@ rb_ivar_lookup(VALUE obj, ID id, VALUE undef)
         VALUE val = undef;
         RB_VM_LOCK_ENTER();
         {
-            VALUE fields_obj = RCLASS_FIELDS_OBJ(obj);
+            VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
             if (fields_obj) {
                 val = rb_ivar_lookup(fields_obj, id, undef);
             }
@@ -1492,7 +1492,7 @@ rb_ivar_delete(VALUE obj, ID id, VALUE undef)
     if (BUILTIN_TYPE(obj) == T_CLASS || BUILTIN_TYPE(obj) == T_MODULE) {
         IVAR_ACCESSOR_SHOULD_BE_MAIN_RACTOR(id);
 
-        VALUE fields_obj = RCLASS_FIELDS_OBJ(obj);
+        VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
         if (fields_obj) {
             RB_VM_LOCK_ENTER();
             {
@@ -1614,8 +1614,7 @@ static shape_id_t
 obj_transition_too_complex(VALUE obj, st_table *table)
 {
     if (BUILTIN_TYPE(obj) == T_CLASS || BUILTIN_TYPE(obj) == T_MODULE) {
-        RUBY_ASSERT(RCLASS_FIELDS_OBJ(obj));
-        return obj_transition_too_complex(RCLASS_FIELDS_OBJ(obj), table);
+        return obj_transition_too_complex(RCLASS_WRITABLE_ENSURE_FIELDS_OBJ(obj), table);
     }
 
     RUBY_ASSERT(!rb_shape_obj_too_complex_p(obj));
@@ -2062,7 +2061,7 @@ rb_obj_set_shape_id(VALUE obj, shape_id_t shape_id)
     if (BUILTIN_TYPE(obj) == T_CLASS || BUILTIN_TYPE(obj) == T_MODULE) {
         // Avoid creating the fields_obj just to freeze the class
         if (!(shape_id == SPECIAL_CONST_SHAPE_ID && old_shape_id == ROOT_SHAPE_ID)) {
-            RBASIC_SET_SHAPE_ID(RCLASS_ENSURE_FIELDS_OBJ(obj), shape_id);
+            RBASIC_SET_SHAPE_ID(RCLASS_WRITABLE_ENSURE_FIELDS_OBJ(obj), shape_id);
         }
     }
     // FIXME: How to do multi-shape?
@@ -2201,7 +2200,7 @@ rb_ivar_defined(VALUE obj, ID id)
       case T_CLASS:
       case T_MODULE:
         RB_VM_LOCKING() {
-            VALUE fields_obj = RCLASS_FIELDS_OBJ(obj);
+            VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
             if (fields_obj) {
                 defined = ivar_defined0(fields_obj, id);
             }
@@ -2455,7 +2454,7 @@ rb_field_foreach(VALUE obj, rb_ivar_foreach_callback_func *func, st_data_t arg,
       case T_MODULE:
         IVAR_ACCESSOR_SHOULD_BE_MAIN_RACTOR(0);
         RB_VM_LOCKING() {
-            VALUE fields_obj = RCLASS_FIELDS_OBJ(obj);
+            VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
             if (fields_obj) {
                 class_fields_each(fields_obj, func, arg, ivar_only);
             }
@@ -2488,7 +2487,7 @@ rb_ivar_count(VALUE obj)
       case T_CLASS:
       case T_MODULE:
         {
-            VALUE fields_obj = RCLASS_FIELDS_OBJ(obj);
+            VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
             if (!fields_obj) {
                 return 0;
             }
@@ -4706,7 +4705,7 @@ static int
 class_ivar_set(VALUE obj, ID id, VALUE val)
 {
     bool existing = true;
-    const VALUE original_fields_obj = RCLASS_FIELDS_OBJ(obj);
+    const VALUE original_fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
     VALUE fields_obj = original_fields_obj ? original_fields_obj : rb_imemo_class_fields_new(obj, 1);
 
     shape_id_t next_shape_id = 0;
@@ -4758,7 +4757,7 @@ class_ivar_set(VALUE obj, ID id, VALUE val)
     }
 
     if (fields_obj != original_fields_obj) {
-        RCLASS_SET_FIELDS_OBJ(obj, fields_obj);
+        RCLASS_WRITABLE_SET_FIELDS_OBJ(obj, fields_obj);
         // TODO: What should we set as the T_CLASS shape_id?
         // In most case we can replicate the single `fields_obj` shape
         // but in namespaced case?
@@ -4777,7 +4776,7 @@ class_ivar_set(VALUE obj, ID id, VALUE val)
 
         if (fields_obj != original_fields_obj) {
             RBASIC_SET_SHAPE_ID(fields_obj, next_shape_id);
-            RCLASS_SET_FIELDS_OBJ(obj, fields_obj);
+            RCLASS_WRITABLE_SET_FIELDS_OBJ(obj, fields_obj);
             // TODO: What should we set as the T_CLASS shape_id?
             // In most case we can replicate the single `fields_obj` shape
             // but in namespaced case?
@@ -4809,7 +4808,7 @@ static void
 class_field_set(VALUE obj, shape_id_t target_shape_id, VALUE val)
 {
     RUBY_ASSERT(RB_TYPE_P(obj, T_CLASS) || RB_TYPE_P(obj, T_MODULE));
-    obj_field_set(RCLASS_ENSURE_FIELDS_OBJ(obj), target_shape_id, val);
+    obj_field_set(RCLASS_WRITABLE_ENSURE_FIELDS_OBJ(obj), target_shape_id, val);
 }
 
 static int

vm_insnhelper.c

@@ -1248,7 +1248,7 @@ vm_getivar(VALUE obj, ID id, const rb_iseq_t *iseq, IVC ic, const struct rb_call
                 }
             }
 
-            fields_obj = RCLASS_FIELDS_OBJ(obj);
+            fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj);
             if (!fields_obj) {
                 return default_value;
             }