Fixed visibility at method definition

There was a problem with visibility state from proc that straddles a fiber or is independent.

Therefore, it has been changed to give priority to env objects, if any.
Also, added "separate module" flag to block traversal to a higher level env object.

Note that the "separate module" flag is now set when calling blocks with the `mrb_yield_with_class()` function.

fixed https://github.com/mruby/mruby/issues/6494

include/mruby.h

@@ -176,7 +176,8 @@ typedef struct {
   uint8_t n:4;                  /* (15=*) c=n|nk<<4 */
   uint8_t nk:4;                 /* (15=*) */
   uint8_t cci;                  /* called from C function */
-  uint8_t vis;                  /* visibility in the current scope */
+  uint8_t vis;                  /* 5(ZERO):1(separate module):2(method visibility) */
+                                /* under 3-bit flags are copied to env, and after that, env takes precedence */
   mrb_sym mid;
   const struct RProc *proc;
   struct RProc *blk;

include/mruby/internal.h

@@ -191,9 +191,13 @@ size_t mrb_gc_mark_iv(mrb_state*, struct RObject*);
 void mrb_gc_free_iv(mrb_state*, struct RObject*);
 
 /* VM */
+#define MRB_CI_VISIBILITY(ci) MRB_FLAGS_GET((ci)->vis, 0, 2)
+#define MRB_CI_SET_VISIBILITY(ci, visi) MRB_FLAGS_SET((ci)->vis, 0, 2, visi)
+#define MRB_CI_SEPARATE_MODULE_P(ci) MRB_FLAG_P((ci)->vis, 2)
+#define MRB_CI_SET_SEPARATE_MODULE(ci) MRB_FLAG_ENABLE((ci)->vis, 2)
 mrb_int mrb_ci_bidx(mrb_callinfo *ci);
 mrb_int mrb_ci_nregs(mrb_callinfo *ci);
-mrb_value mrb_exec_irep(mrb_state *mrb, mrb_value self, const struct RProc *p);
+mrb_value mrb_exec_irep(mrb_state *mrb, mrb_value self, const struct RProc *p, mrb_bool separate_module);
 mrb_value mrb_obj_instance_eval(mrb_state*, mrb_value);
 mrb_value mrb_object_exec(mrb_state *mrb, mrb_value self, struct RClass *target_class);
 mrb_value mrb_mod_module_eval(mrb_state*, mrb_value);

include/mruby/proc.h

@@ -30,13 +30,17 @@ struct REnv {
   mrb_sym mid;
 };
 
-/* flags (21bits): 5(ZERO):8(cioff/bidx):8(stack_len) */
+/* flags (20bits): 1(ZERO):1(separate module):2(visibility):8(cioff/bidx):8(stack_len) */
 #define MRB_ENV_SET_LEN(e,len) ((e)->flags = (((e)->flags & ~0xff)|((unsigned int)(len) & 0xff)))
 #define MRB_ENV_LEN(e) ((mrb_int)((e)->flags & 0xff))
 #define MRB_ENV_CLOSE(e) ((e)->cxt = NULL)
 #define MRB_ENV_ONSTACK_P(e) ((e)->cxt != NULL)
 #define MRB_ENV_BIDX(e) (((e)->flags >> 8) & 0xff)
 #define MRB_ENV_SET_BIDX(e,idx) ((e)->flags = (((e)->flags & ~(0xff<<8))|((unsigned int)(idx) & 0xff)<<8))
+#define MRB_ENV_SET_VISIBILITY(e, vis) MRB_FLAGS_SET((e)->flags, 16, 2, vis)
+#define MRB_ENV_VISIBILITY(e) MRB_FLAGS_GET((e)->flags, 16, 2)
+#define MRB_ENV_SEPARATE_MODULE_P(e) MRB_FLAG_P((e)->flags, 18)
+#define MRB_ENV_COPY_FLAGS_FROM_CI(e, ci) MRB_FLAGS_SET((e)->flags, 16, 3, (ci)->vis)
 
 /*
  * Returns TRUE on success.

include/mruby/value.h

@@ -94,6 +94,14 @@ struct mrb_state;
 # define MRB_PRIx PRIx32
 #endif
 
+#define MRB_FLAGS_MASK(shift, width)    (~(~0U << (width)) << (shift))
+#define MRB_FLAGS_GET(b, s, w)          (((b) >> (s)) & MRB_FLAGS_MASK(0, w))
+#define MRB_FLAGS_SET(b, s, w, n)       ((b) = MRB_FLAGS_INVERT_TRIM(b, s, w) | MRB_FLAGS_MAKE(s, w, n))
+#define MRB_FLAGS_INVERT_TRIM(b, s, w)  ((b) & ~MRB_FLAGS_MASK(s, w))
+#define MRB_FLAGS_MAKE(s, w, n)         (((n) & MRB_FLAGS_MASK(0, w)) << (s))
+#define MRB_FLAG_ENABLE(b, s)           ((b) |= MRB_FLAGS_MASK(s, 1))
+#define MRB_FLAG_P(b, s)                (!!((b) & MRB_FLAGS_MASK(s, 1)))
+
 MRB_API mrb_bool mrb_read_int(const char *p, const char *e, char **endp, mrb_int *np);
 /* obsolete; do not use mrb_int_read() */
 MRB_API mrb_int mrb_int_read(const char*, const char*, char**);

mrbgems/mruby-eval/src/eval.c

@@ -147,7 +147,7 @@ exec_irep(mrb_state *mrb, mrb_value self, struct RProc *proc)
   mrb->c->ci->nk = 0;
   /* clear block */
   mrb->c->ci->stack[1] = mrb_nil_value();
-  return mrb_exec_irep(mrb, self, proc);
+  return mrb_exec_irep(mrb, self, proc, TRUE);
 }
 
 static void

mrbgems/mruby-eval/test/eval.rb

@@ -160,3 +160,26 @@ assert('Module#class_eval with string') do
   b = c.class_eval("class A; def a; 55; end; end; class B; def b; A; end; end; B")
   assert_equal 55, b.new.b.new.a
 end
+
+assert 'method visibility with eval' do
+  c = Class.new do
+    eval <<~CODE
+      private
+      def bad!
+        "BAD!"
+      end
+    CODE
+
+    def good!
+      "GOOD!"
+    end
+  end
+
+  assert_raise NoMethodError do
+    c.new.bad!
+  end
+
+  assert_equal "GOOD!" do
+    c.new.good!
+  end
+end

mrbgems/mruby-method/src/method.c

@@ -248,7 +248,7 @@ mcall(mrb_state *mrb, mrb_value self, mrb_value recv)
   mrb->c->ci->mid = mid;
   mrb->c->ci->u.target_class = tc;
 
-  return mrb_exec_irep(mrb, recv, proc);
+  return mrb_exec_irep(mrb, recv, proc, FALSE);
 }
 
 static mrb_value

mrbgems/mruby-struct/test/struct.rb

@@ -208,3 +208,24 @@ assert 'Struct#freeze' do
   assert_raise(FrozenError) { o[:m] = :modify }
   assert_equal :test, o.m
 end
+
+assert 'method visibility with Struct' do
+  c = Struct.new :r, :g, :b do
+    def good!
+      "GOOD!"
+    end
+
+    private
+    def bad!
+      "BAD!"
+    end
+  end
+
+  assert_equal "GOOD!" do
+    c.new.good!
+  end
+
+  assert_raise NoMethodError do
+    c.new.bad!
+  end
+end

src/class.c

@@ -743,30 +743,30 @@ mrb_define_class_under(mrb_state *mrb, struct RClass *outer, const char *name, s
 }
 
 static mrb_callinfo*
-find_visibility_ci(mrb_state *mrb, const struct RClass *c, int n)
+find_visibility_ci(mrb_state *mrb, const struct RClass *c, int n, struct REnv **e)
 {
-  mrb_callinfo *ci = mrb->c->ci - n;
-  const mrb_callinfo *cibase = mrb->c->cibase;
+  const struct mrb_context *ec = mrb->c;
+  mrb_callinfo *ci = ec->ci - n;
   const struct RProc *p = ci->proc;
 
-  mrb_assert(p != NULL);
   if (c == NULL) c = mrb_vm_ci_target_class(ci);
-  while (p->upper && cibase < ci) {
-    p = p->upper;
 
-    mrb_callinfo *upper_ci = ci - 1;
-    while (cibase < upper_ci) {
-      if (upper_ci->proc == p) {
-        if (mrb_vm_ci_target_class(upper_ci) != c) {
-          return ci;
-        }
-        ci = upper_ci;
-        break;
-      }
-      upper_ci--;
+  if (!p || p->upper == NULL || MRB_PROC_SCOPE_P(p) ||
+      p->e.env == NULL || !MRB_PROC_ENV_P(p) || mrb_vm_ci_target_class(ci) != c || MRB_CI_SEPARATE_MODULE_P(ci)) {
+    mrb_assert(ci->u.env);
+    *e = (ci->u.env->tt == MRB_TT_ENV ? ci->u.env : NULL);
+    return ci;
+  }
+
+  for (;;) {
+    struct REnv *env = p->e.env;
+    p = p->upper;
+    if (p->upper == NULL || MRB_PROC_SCOPE_P(p) ||
+        p->e.env == NULL || !MRB_PROC_ENV_P(p) || p->e.env->c != c || MRB_ENV_SEPARATE_MODULE_P(env)) {
+      *e = env;
+      return NULL;
     }
   }
-  return ci;
 }
 
 MRB_API void
@@ -813,8 +813,10 @@ mrb_define_method_raw(mrb_state *mrb, struct RClass *c, mrb_sym mid, mrb_method_
     MRB_SET_VISIBILITY(flags, MT_PRIVATE);
   }
   else if ((flags & MT_VMASK) == MT_VDEFAULT) {
-    mrb_callinfo *ci = find_visibility_ci(mrb, c, 0);
-    MRB_SET_VISIBILITY(flags, ci->vis);
+    struct REnv *e;
+    mrb_callinfo *ci = find_visibility_ci(mrb, c, 0, &e);
+    mrb_assert(ci || e);
+    MRB_SET_VISIBILITY(flags, (e ? MRB_ENV_VISIBILITY(e) : MRB_CI_VISIBILITY(ci)));
   }
   mt_put(mrb, h, mid, flags, ptr);
   mc_clear_by_id(mrb, mid);
@@ -1697,8 +1699,14 @@ mrb_mod_visibility(mrb_state *mrb, mrb_value mod, int vis)
 
   mrb_get_args(mrb, "*!", &argv, &argc);
   if (argc == 0) {
-    mrb_callinfo *ci = find_visibility_ci(mrb, NULL, 1);
-    ci->vis = vis;
+    struct REnv *e;
+    mrb_callinfo *ci = find_visibility_ci(mrb, NULL, 1, &e);
+    if (e) {
+      MRB_ENV_SET_VISIBILITY(e, vis);
+    }
+    else {
+      MRB_CI_SET_VISIBILITY(ci, vis);
+    }
   }
   else {
     mt_tbl *h = c->mt;

src/proc.c

@@ -86,6 +86,7 @@ mrb_env_new(mrb_state *mrb, struct mrb_context *c, mrb_callinfo *ci, int nstacks
   e->mid = ci->mid;
   e->stack = stack;
   e->cxt = c;
+  MRB_ENV_COPY_FLAGS_FROM_CI(e, ci);
 
   return e;
 }

src/vm.c

@@ -812,10 +812,13 @@ exec_irep(mrb_state *mrb, mrb_value self, const struct RProc *p)
 }
 
 mrb_value
-mrb_exec_irep(mrb_state *mrb, mrb_value self, const struct RProc *p)
+mrb_exec_irep(mrb_state *mrb, mrb_value self, const struct RProc *p, mrb_bool separate_module)
 {
   mrb_callinfo *ci = mrb->c->ci;
   if (ci->cci == CINFO_NONE) {
+    if (separate_module) {
+      MRB_CI_SET_SEPARATE_MODULE(ci);
+    }
     return exec_irep(mrb, self, p);
   }
   else {
@@ -824,14 +827,20 @@ mrb_exec_irep(mrb_state *mrb, mrb_value self, const struct RProc *p)
       if (MRB_PROC_NOARG_P(p) && (ci->n > 0 || ci->nk > 0)) {
         check_method_noarg(mrb, ci);
       }
-      cipush(mrb, 0, CINFO_DIRECT, CI_TARGET_CLASS(ci), p, NULL, ci->mid, ci->n|(ci->nk<<4));
+      ci = cipush(mrb, 0, CINFO_DIRECT, CI_TARGET_CLASS(ci), p, NULL, ci->mid, ci->n|(ci->nk<<4));
+      if (separate_module) {
+        MRB_CI_SET_SEPARATE_MODULE(ci);
+      }
       mrb->exc = NULL;
       ret = MRB_PROC_CFUNC(p)(mrb, self);
       cipop(mrb);
     }
     else {
       mrb_int keep = ci_bidx(ci) + 1; /* receiver + block */
-      cipush(mrb, 0, CINFO_SKIP, CI_TARGET_CLASS(ci), p, NULL, ci->mid, ci->n|(ci->nk<<4));
+      ci = cipush(mrb, 0, CINFO_SKIP, CI_TARGET_CLASS(ci), p, NULL, ci->mid, ci->n|(ci->nk<<4));
+      if (separate_module) {
+        MRB_CI_SET_SEPARATE_MODULE(ci);
+      }
       ret = mrb_vm_run(mrb, p, self, keep);
     }
     if (mrb->exc && mrb->jmp) {
@@ -855,7 +864,7 @@ mrb_object_exec(mrb_state *mrb, mrb_value self, struct RClass *target_class)
   mrb_gc_protect(mrb, blk);
   ci->stack[bidx] = mrb_nil_value();
   mrb_vm_ci_target_class_set(ci, target_class);
-  return mrb_exec_irep(mrb, self, mrb_proc_ptr(blk));
+  return mrb_exec_irep(mrb, self, mrb_proc_ptr(blk), FALSE);
 }
 
 /* 15.3.1.3.4  */
@@ -977,6 +986,7 @@ eval_under(mrb_state *mrb, mrb_value self, mrb_value blk, struct RClass *c)
   ci->n = 1;
   ci->nk = 0;
   ci->mid = ci[-1].mid;
+  MRB_CI_SET_SEPARATE_MODULE(ci);
   if (MRB_PROC_CFUNC_P(p)) {
     stack_extend(mrb, 4);
     mrb->c->ci->stack[0] = self;
@@ -1048,8 +1058,9 @@ mrb_obj_instance_eval(mrb_state *mrb, mrb_value self)
   return eval_under(mrb, self, b, mrb_singleton_class_ptr(mrb, self));
 }
 
-MRB_API mrb_value
-mrb_yield_with_class(mrb_state *mrb, mrb_value b, mrb_int argc, const mrb_value *argv, mrb_value self, struct RClass *c)
+static mrb_value
+yield_with_attr(mrb_state *mrb, mrb_value b, mrb_int argc, const mrb_value *argv, mrb_value self, struct RClass *c,
+                mrb_bool separate_module)
 {
   check_block(mrb, b);
 
@@ -1068,6 +1079,9 @@ mrb_yield_with_class(mrb_state *mrb, mrb_value b, mrb_int argc, const mrb_value
   funcall_args_capture(mrb, 0, argc, argv, mrb_nil_value(), ci);
   ci->u.target_class = c;
   ci->proc = p;
+  if (separate_module) {
+    MRB_CI_SET_SEPARATE_MODULE(ci);
+  }
 
   mrb_value val;
   if (MRB_PROC_CFUNC_P(p)) {
@@ -1086,6 +1100,12 @@ mrb_yield_with_class(mrb_state *mrb, mrb_value b, mrb_int argc, const mrb_value
   return val;
 }
 
+MRB_API mrb_value
+mrb_yield_with_class(mrb_state *mrb, mrb_value b, mrb_int argc, const mrb_value *argv, mrb_value self, struct RClass *c)
+{
+  return yield_with_attr(mrb, b, argc, argv, self, c, TRUE);
+}
+
 MRB_API mrb_value
 mrb_yield_argv(mrb_state *mrb, mrb_value b, mrb_int argc, const mrb_value *argv)
 {
@@ -1093,7 +1113,7 @@ mrb_yield_argv(mrb_state *mrb, mrb_value b, mrb_int argc, const mrb_value *argv)
   struct RClass *tc;
   mrb_value self = mrb_proc_get_self(mrb, p, &tc);
 
-  return mrb_yield_with_class(mrb, b, argc, argv, self, tc);
+  return yield_with_attr(mrb, b, argc, argv, self, tc, FALSE);
 }
 
 MRB_API mrb_value
@@ -1103,7 +1123,7 @@ mrb_yield(mrb_state *mrb, mrb_value b, mrb_value arg)
   struct RClass *tc;
   mrb_value self = mrb_proc_get_self(mrb, p, &tc);
 
-  return mrb_yield_with_class(mrb, b, 1, &arg, self, tc);
+  return yield_with_attr(mrb, b, 1, &arg, self, tc, FALSE);
 }
 
 mrb_value

test/t/module.rb

@@ -831,6 +831,51 @@ assert('method visibility') do
   assert_equal :test, v.test_private { :test }
 end
 
+assert('method visibility with meta programming') do
+  assert_equal "GOOD!" do
+    f = nil
+    c = Class.new {
+      private
+      f = ->(&blk) {
+        class_eval(&blk)
+      }
+    }
+    f.call {
+      def good!
+        "GOOD!"
+      end
+    }
+    c.new.good!
+  end
+
+  assert_equal "GOOD!" do
+    c = Class.new
+    c.class_eval {
+      private
+      c.class_eval {
+        def good!
+          "GOOD!"
+        end
+      }
+    }
+    c.new.good!
+  end
+
+  assert_raise NoMethodError do
+    f = nil
+    c = Class.new {
+      private
+      f = -> {
+        def bad!
+          "BAD!"
+        end
+      }
+    }
+    f.call
+    c.new.bad!
+  end
+end
+
 assert('Module#module_function') do
   module M
     def modfunc; end