Simplify RUBY_RACTOR_ISOLATION code for readability

Make strict and warning-only Proc checks explicit, simplify warning control flow, and consolidate method-call isolation checks. Remove an unnecessary Proc-copy wrapper while preserving behavior.

Author: yaroslav-shopify

ractor.c

@@ -4293,7 +4293,9 @@ static const struct st_hash_type isolation_warn_hash_type = {
 static bool
 isolation_warnings_enabled_p(void)
 {
-    return !GET_EC()->ractor_isolation_warning && !NIL_P(ruby_verbose) &&
+    if (GET_EC()->ractor_isolation_warning) return false;
+
+    return !NIL_P(ruby_verbose) &&
         rb_warning_category_enabled_p(RB_WARN_CATEGORY_RACTOR_ISOLATION);
 }
 
@@ -4302,24 +4304,24 @@ static VALUE
 isolation_source_location(int *line)
 {
     const rb_execution_context_t *ec = GET_EC();
-    const rb_control_frame_t *cfp = ec->cfp;
-
-    while (!RUBY_VM_CONTROL_FRAME_STACK_OVERFLOW_P(ec, cfp)) {
-        if (VM_FRAME_RUBYFRAME_P(cfp) && CFP_ISEQ(cfp)) {
-            VALUE path = rb_iseq_path(CFP_ISEQ(cfp));
-            if (strncmp("<internal:", RSTRING_PTR(path), 10) != 0) {
-                *line = rb_vm_get_sourceline(cfp);
-                return path;
-            }
-        }
-        cfp = RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp);
+
+    for (const rb_control_frame_t *cfp = ec->cfp;
+         !RUBY_VM_CONTROL_FRAME_STACK_OVERFLOW_P(ec, cfp);
+         cfp = RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp)) {
+        if (!VM_FRAME_RUBYFRAME_P(cfp) || !CFP_ISEQ(cfp)) continue;
+
+        VALUE path = rb_iseq_path(CFP_ISEQ(cfp));
+        if (strncmp("<internal:", RSTRING_PTR(path), 10) == 0) continue;
+
+        *line = rb_vm_get_sourceline(cfp);
+        return path;
     }
     *line = 0;
     return Qnil;
 }
 
 static bool
-isolation_warn_first_p(VALUE message, VALUE path, int line)
+isolation_warn_first_occurrence_p(VALUE message, VALUE path, int line)
 {
     bool first = true;
     RB_VM_LOCKING() {
@@ -4334,6 +4336,7 @@ isolation_warn_first_p(VALUE message, VALUE path, int line)
             isolation_warn_suppressed++;
         }
         else {
+            // Own copies of both strings outside Ruby's heap for this VM-wide table.
             size_t file_size = strlen(lookup.file) + 1;
             size_t message_size = strlen(lookup.message) + 1;
             struct isolation_warn_key *key = malloc(sizeof(*key) + file_size + message_size);
@@ -4345,6 +4348,7 @@ isolation_warn_first_p(VALUE message, VALUE path, int line)
                 *key = (struct isolation_warn_key){file, text, line};
                 st_add_direct(isolation_warn_tbl, (st_data_t)key, 0);
             }
+            // If allocation fails, still report the warning.
         }
     }
     RB_GC_GUARD(message);
@@ -4358,7 +4362,9 @@ isolation_warn_emit(VALUE message)
     int line;
     VALUE path = isolation_source_location(&line);
     StringValueCStr(message);
-    if (ruby_ractor_isolation_enabled < 2 && !isolation_warn_first_p(message, path, line)) return Qnil;
+    if (ruby_ractor_isolation_enabled < 2 && !isolation_warn_first_occurrence_p(message, path, line)) {
+        return Qnil;
+    }
 
     const char *file = NIL_P(path) ? NULL : RSTRING_PTR(path);
     rb_category_compile_warn(RB_WARN_CATEGORY_RACTOR_ISOLATION, file, line, "%s", RSTRING_PTR(message));
@@ -4368,7 +4374,7 @@ isolation_warn_emit(VALUE message)
 }
 
 static VALUE
-isolation_warn_end(VALUE ec_ptr)
+isolation_warn_clear_guard(VALUE ec_ptr)
 {
     ((rb_execution_context_t *)ec_ptr)->ractor_isolation_warning = false;
     return Qnil;
@@ -4384,7 +4390,7 @@ rb_ractor_isolation_warn(VALUE message)
     // Keep the guard fiber-local and restore it even if the hook raises.
     rb_execution_context_t *ec = GET_EC();
     ec->ractor_isolation_warning = true;
-    rb_ensure(isolation_warn_emit, message, isolation_warn_end, (VALUE)ec);
+    rb_ensure(isolation_warn_emit, message, isolation_warn_clear_guard, (VALUE)ec);
 }
 
 void

vm.c

@@ -1400,21 +1400,15 @@ rb_proc_dup_0(VALUE self)
     return proc_dup_0(self, true);
 }
 
-static VALUE
-proc_dup(VALUE self, bool preserve_shareability)
+VALUE
+rb_proc_dup(VALUE self)
 {
-    VALUE procval = proc_dup_0(self, preserve_shareability);
+    VALUE procval = rb_proc_dup_0(self);
     VALUE recipe = rb_proc_refinements_recipe(self);
     if (!NIL_P(recipe)) rb_proc_set_refinements_recipe(procval, recipe);
     return procval;
 }
 
-VALUE
-rb_proc_dup(VALUE self)
-{
-    return proc_dup(self, true);
-}
-
 /* Proc#refined: build a Proc that runs `iseq` with the refinements of
  * `recipe`, sharing self's environment.  `iseq` is normally self's own block
  * iseq, which the copy replaces on the first call. */
@@ -1483,10 +1477,15 @@ collect_outer_variable_names(ID id, VALUE val, void *ptr)
     return ID_TABLE_CONTINUE;
 }
 
+enum proc_isolation_mode {
+    PROC_ISOLATION_STRICT,
+    PROC_ISOLATION_WARN_ONLY,
+};
+
 static void
-proc_isolation_violation_str(VALUE message, bool warn)
+proc_isolation_violation_str(VALUE message, enum proc_isolation_mode mode)
 {
-    if (warn) {
+    if (mode == PROC_ISOLATION_WARN_ONLY) {
         rb_ractor_isolation_warn(message);
     }
     else {
@@ -1495,7 +1494,7 @@ proc_isolation_violation_str(VALUE message, bool warn)
 }
 
 static const rb_env_t *
-env_copy(const VALUE *src_ep, VALUE read_only_variables, bool warn, bool *valid)
+env_copy(const VALUE *src_ep, VALUE read_only_variables, enum proc_isolation_mode mode, bool *can_isolate)
 {
     const rb_env_t *src_env = (rb_env_t *)VM_ENV_ENVVAL(src_ep);
     VM_ASSERT(src_env->ep == src_ep);
@@ -1535,16 +1534,16 @@ env_copy(const VALUE *src_ep, VALUE read_only_variables, bool warn, bool *valid)
                         VALUE name = rb_id2str(id);
                         VALUE msg = rb_sprintf("cannot make a shareable Proc because "
                                                "the outer variable '%" PRIsVALUE "' may be reassigned.", name);
-                        proc_isolation_violation_str(msg, warn);
-                        *valid = false;
+                        proc_isolation_violation_str(msg, mode);
+                        *can_isolate = false;
                     }
 
                     // check shareable
                     VALUE v = src_env->env[j];
                     if (!rb_ractor_shareable_p(v)) {
                         VALUE name = rb_id2str(id);
                         VALUE msg;
-                        if (warn) {
+                        if (mode == PROC_ISOLATION_WARN_ONLY) {
                             msg = rb_sprintf("cannot make a shareable Proc because it can refer "
                                              "an unshareable object of class %+" PRIsVALUE " from ",
                                              rb_class_of(v));
@@ -1557,8 +1556,8 @@ env_copy(const VALUE *src_ep, VALUE read_only_variables, bool warn, bool *valid)
                             rb_str_catf(msg, "variable '%" PRIsVALUE "'", name);
                         else
                             rb_str_cat_cstr(msg, "a hidden variable");
-                        proc_isolation_violation_str(msg, warn);
-                        *valid = false;
+                        proc_isolation_violation_str(msg, mode);
+                        *can_isolate = false;
                     }
                     RB_OBJ_WRITE((VALUE)copied_env, &env_body[j], v);
                     rb_ary_delete_at(read_only_variables, i);
@@ -1570,7 +1569,7 @@ env_copy(const VALUE *src_ep, VALUE read_only_variables, bool warn, bool *valid)
 
     if (!VM_ENV_LOCAL_P(src_ep)) {
         const VALUE *prev_ep = VM_ENV_PREV_EP(src_env->ep);
-        const rb_env_t *new_prev_env = env_copy(prev_ep, read_only_variables, warn, valid);
+        const rb_env_t *new_prev_env = env_copy(prev_ep, read_only_variables, mode, can_isolate);
         ep[VM_ENV_DATA_INDEX_SPECVAL] = VM_GUARDED_PREV_EP(new_prev_env->ep);
         RB_OBJ_WRITTEN(copied_env, Qundef, new_prev_env);
         VM_ENV_FLAGS_UNSET(ep, VM_ENV_FLAG_LOCAL);
@@ -1586,18 +1585,18 @@ env_copy(const VALUE *src_ep, VALUE read_only_variables, bool warn, bool *valid)
         ep[VM_ENV_DATA_INDEX_SPECVAL] = VM_BLOCK_HANDLER_NONE;
     }
 
-    if (*valid) {
+    if (*can_isolate) {
         RB_OBJ_SET_SHAREABLE((VALUE)copied_env);
     }
     return copied_env;
 }
 
 static bool
-proc_isolate_env(VALUE self, rb_proc_t *proc, VALUE read_only_variables, bool warn, bool valid)
+proc_isolate_env(VALUE self, rb_proc_t *proc, VALUE read_only_variables, enum proc_isolation_mode mode, bool can_isolate)
 {
     const struct rb_captured_block *captured = &proc->block.as.captured;
-    const rb_env_t *env = env_copy(captured->ep, read_only_variables, warn, &valid);
-    if (!valid) return false;
+    const rb_env_t *env = env_copy(captured->ep, read_only_variables, mode, &can_isolate);
+    if (!can_isolate) return false;
 
     *((const VALUE **)&proc->block.as.captured.ep) = env->ep;
     RB_OBJ_WRITTEN(self, Qundef, env);
@@ -1615,22 +1614,23 @@ proc_has_ivar_i(ID name, VALUE val, st_data_t arg)
 }
 
 static void
-proc_check_isolation_ivars(VALUE self, bool warn)
+proc_check_isolation_ivars(VALUE self, enum proc_isolation_mode mode)
 {
+    if (LIKELY(!rb_obj_shape_has_ivars(self))) return;
+
     /* ivars are not traversed here, so their values may be unshareable */
-    if (UNLIKELY(rb_obj_shape_has_ivars(self))) {
-        bool has_ivar = false;
-        rb_ivar_foreach(self, proc_has_ivar_i, (st_data_t)&has_ivar);
+    bool has_ivar = false;
+    rb_ivar_foreach(self, proc_has_ivar_i, (st_data_t)&has_ivar);
 
-        if (has_ivar) {
-            proc_isolation_violation_str(
-                rb_str_new_cstr("can not isolate a Proc because it has instance variables"), warn);
-        }
+    if (has_ivar) {
+        proc_isolation_violation_str(
+            rb_str_new_cstr("can not isolate a Proc because it has instance variables"), mode);
     }
 }
 
 static VALUE
-proc_shared_outer_variables(struct rb_id_table *outer_variables, bool isolate, const char *message, bool warn, bool *valid)
+proc_shared_outer_variables(struct rb_id_table *outer_variables, bool isolate, const char *message,
+                            enum proc_isolation_mode mode, bool *can_isolate)
 {
     struct collect_outer_variable_name_data data = {
         .isolate = isolate,
@@ -1653,13 +1653,13 @@ proc_shared_outer_variables(struct rb_id_table *outer_variables, bool isolate, c
         }
         if (*sep == ',') rb_str_cat_cstr(str, ")");
         rb_str_cat_cstr(str, data.yield ? " and uses 'yield'." : ".");
-        proc_isolation_violation_str(str, warn);
-        if (valid) *valid = false;
+        proc_isolation_violation_str(str, mode);
+        if (can_isolate) *can_isolate = false;
     }
     else if (data.yield) {
         VALUE str = rb_sprintf("can not %s because it uses 'yield'.", message);
-        proc_isolation_violation_str(str, warn);
-        if (valid) *valid = false;
+        proc_isolation_violation_str(str, mode);
+        if (can_isolate) *can_isolate = false;
     }
 
     return data.read_only;
@@ -1681,15 +1681,16 @@ rb_proc_isolate_bang(VALUE self, VALUE replace_self)
         }
 
         if (ISEQ_BODY(iseq)->outer_variables) {
-            proc_shared_outer_variables(ISEQ_BODY(iseq)->outer_variables, true, "isolate a Proc", false, NULL);
+            proc_shared_outer_variables(ISEQ_BODY(iseq)->outer_variables, true, "isolate a Proc",
+                                        PROC_ISOLATION_STRICT, NULL);
         }
 
-        if (!proc_isolate_env(self, proc, Qfalse, false, true)) return self;
+        if (!proc_isolate_env(self, proc, Qfalse, PROC_ISOLATION_STRICT, true)) return self;
         proc->header.is_isolated = TRUE;
         RB_OBJ_WRITE(self, &proc->block.as.captured.self, Qnil);
     }
 
-    proc_check_isolation_ivars(self, false);
+    proc_check_isolation_ivars(self, PROC_ISOLATION_STRICT);
 
     RB_OBJ_SET_SHAREABLE(self);
     return self;
@@ -1712,46 +1713,48 @@ rb_proc_check_isolation_warn(VALUE self)
     if (iseq) {
         rb_proc_t *proc = (rb_proc_t *)RTYPEDDATA_DATA(self);
         if (proc->block.type == block_type_iseq && ISEQ_BODY(iseq)->outer_variables) {
-            proc_shared_outer_variables(ISEQ_BODY(iseq)->outer_variables, true, "isolate a Proc", true, NULL);
+            proc_shared_outer_variables(ISEQ_BODY(iseq)->outer_variables, true, "isolate a Proc",
+                                        PROC_ISOLATION_WARN_ONLY, NULL);
         }
     }
-    proc_check_isolation_ivars(self, true);
+    proc_check_isolation_ivars(self, PROC_ISOLATION_WARN_ONLY);
 }
 
 VALUE
 rb_proc_ractor_make_shareable(VALUE self, VALUE replace_self)
 {
     const rb_iseq_t *iseq = vm_proc_iseq(self);
-    bool warn = rb_ractor_isolation_check_p();
+    enum proc_isolation_mode mode = rb_ractor_isolation_check_p() ?
+        PROC_ISOLATION_WARN_ONLY : PROC_ISOLATION_STRICT;
 
     if (iseq) {
         rb_proc_t *proc = (rb_proc_t *)RTYPEDDATA_DATA(self);
 
         if (proc->block.type != block_type_iseq) rb_raise(rb_eRuntimeError, "not supported yet");
 
-        bool valid = true;
+        bool can_isolate = true;
         VALUE proc_self = UNDEF_P(replace_self) ? vm_block_self(&proc->block) : replace_self;
         if (!rb_ractor_shareable_p(proc_self)) {
             VALUE message = rb_sprintf("Proc's self is not shareable: %" PRIsVALUE, self);
-            proc_isolation_violation_str(message, warn);
-            valid = false;
+            proc_isolation_violation_str(message, mode);
+            can_isolate = false;
         }
 
         VALUE read_only_variables = Qfalse;
 
         if (ISEQ_BODY(iseq)->outer_variables) {
             read_only_variables =
                 proc_shared_outer_variables(ISEQ_BODY(iseq)->outer_variables, false,
-                                            "make a Proc shareable", warn, &valid);
+                                            "make a Proc shareable", mode, &can_isolate);
         }
 
-        valid = proc_isolate_env(self, proc, read_only_variables, warn, valid);
+        can_isolate = proc_isolate_env(self, proc, read_only_variables, mode, can_isolate);
         if (!UNDEF_P(replace_self)) {
             RB_OBJ_WRITE(self, &proc->block.as.captured.self, replace_self);
         }
         // Diagnostic fallback keeps the original closure and requested receiver,
         // but must not mark an invalid Proc isolated or shareable.
-        if (!valid) return self;
+        if (!can_isolate) return self;
         proc->header.is_isolated = TRUE;
     }
     else {
@@ -1761,8 +1764,8 @@ rb_proc_ractor_make_shareable(VALUE self, VALUE replace_self)
         VALUE proc_self = vm_block_self(block);
         if (!rb_ractor_shareable_p(proc_self)) {
             VALUE message = rb_sprintf("Proc's self is not shareable: %" PRIsVALUE, self);
-            proc_isolation_violation_str(message, warn);
-            if (warn) return self;
+            proc_isolation_violation_str(message, mode);
+            return self;
         }
     }
 
@@ -1775,7 +1778,9 @@ rb_proc_ractor_make_shareable_copy(VALUE self, VALUE replace_self)
 {
     // Rebinding self can make a shareable Proc unshareable in diagnostic mode.
     // The copy must only become shareable after its new receiver is validated.
-    VALUE copy = proc_dup(self, false);
+    VALUE copy = proc_dup_0(self, false);
+    VALUE recipe = rb_proc_refinements_recipe(self);
+    if (!NIL_P(recipe)) rb_proc_set_refinements_recipe(copy, recipe);
     return rb_proc_ractor_make_shareable(copy, replace_self);
 }