From 3fd5e1c2506dca397f819668849e8d528b5873bb Mon Sep 17 00:00:00 2001 From: dearblue Date: Wed, 9 Apr 2025 22:20:33 +0900 Subject: [PATCH] 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 | 3 +- include/mruby/internal.h | 6 +++- include/mruby/proc.h | 6 +++- include/mruby/value.h | 8 +++++ mrbgems/mruby-eval/src/eval.c | 2 +- mrbgems/mruby-eval/test/eval.rb | 23 +++++++++++++ mrbgems/mruby-method/src/method.c | 2 +- mrbgems/mruby-struct/test/struct.rb | 21 ++++++++++++ src/class.c | 50 +++++++++++++++++------------ src/proc.c | 1 + src/vm.c | 36 ++++++++++++++++----- test/t/module.rb | 45 ++++++++++++++++++++++++++ 12 files changed, 169 insertions(+), 34 deletions(-) diff --git a/include/mruby.h b/include/mruby.h index fe29b1e7d..31bd6e949 100644 --- a/include/mruby.h +++ b/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; diff --git a/include/mruby/internal.h b/include/mruby/internal.h index b8387b303..1adc527ba 100644 --- a/include/mruby/internal.h +++ b/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); diff --git a/include/mruby/proc.h b/include/mruby/proc.h index f35d64df9..ee4224d47 100644 --- a/include/mruby/proc.h +++ b/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. diff --git a/include/mruby/value.h b/include/mruby/value.h index 057054463..d915132ed 100644 --- a/include/mruby/value.h +++ b/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**); diff --git a/mrbgems/mruby-eval/src/eval.c b/mrbgems/mruby-eval/src/eval.c index ff471b10f..9fc243875 100644 --- a/mrbgems/mruby-eval/src/eval.c +++ b/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 diff --git a/mrbgems/mruby-eval/test/eval.rb b/mrbgems/mruby-eval/test/eval.rb index a9b0f4e9c..79b304787 100644 --- a/mrbgems/mruby-eval/test/eval.rb +++ b/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 diff --git a/mrbgems/mruby-method/src/method.c b/mrbgems/mruby-method/src/method.c index e85f4dab3..041804bf1 100644 --- a/mrbgems/mruby-method/src/method.c +++ b/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 diff --git a/mrbgems/mruby-struct/test/struct.rb b/mrbgems/mruby-struct/test/struct.rb index c29d5a0ba..0c6efacd1 100644 --- a/mrbgems/mruby-struct/test/struct.rb +++ b/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 diff --git a/src/class.c b/src/class.c index cf03a0197..cb04c8bf4 100644 --- a/src/class.c +++ b/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; diff --git a/src/proc.c b/src/proc.c index 8efc305c5..e3d06408c 100644 --- a/src/proc.c +++ b/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; } diff --git a/src/vm.c b/src/vm.c index c7e4cc8f2..974a71857 100644 --- a/src/vm.c +++ b/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 diff --git a/test/t/module.rb b/test/t/module.rb index 1cfef87a3..a4d734819 100644 --- a/test/t/module.rb +++ b/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