-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Namespace management using control frame #13454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
9cb95e9
to
b0193f3
Compare
fd04acf
to
88e1fad
Compare
❌ Tests Failed✖️no tests failed ✔️62282 tests passed(2 flakes) |
139ed12
to
a5bbd18
Compare
a5bbd18
to
0ac3835
Compare
0ac3835
to
1e43ad1
Compare
…al contexts to fix inconsistent and wrong current namespace detections. This includes: * Moving load_path and related things from rb_vm_t to rb_namespace_t to simplify accessing those values via namespace (instead of accessing either vm or ns) * Initializing root_namespace earlier and consolidate builtin_namespace into root_namespace * Adding VM_FRAME_FLAG_NS_REQUIRE for checkpoints to detect a namespace to load/require files * Removing implicit refinements in the root namespace which was used to determine the namespace to be loaded (replaced by VM_FRAME_FLAG_NS_REQUIRE) * Removing namespaces from rb_proc_t because its namespace can be identified by lexical context * Starting to use ep[VM_ENV_DATA_INDEX_SPECVAL] to store the current namespace when the frame type is MAGIC_TOP or MAGIC_CLASS (block handlers don't exist in this case)
Calling rb_current_namespace() in rb_namespace_current() means to show the definition namespace of Namespace.current itself (it's the root always) but the users' expectation is to show the namespace of the place where the Namespace.current is called.
* checking all control frames (instead of filtering by VM_FRAME_RUBYFRAME_P) because VM_FRAME_FLAG_NS_REQUIRE is set on non-rubyframe * skip frames of CFUNC in the root namespace for Kernel#require (etc) to avoid detecting the root namespace of those frames wrongly
* The current namespace should be based on the Ruby-level location (file, line no in .rb) and we can get it by LEP(ep) basically (VM_ENV_FLAG_LOCAL flag is set) * But the control frame with VM_FRAME_MAGIC_CFUNC is also a LOCAL frame because it's a visible Ruby-level frame without block handlers * So, for the namespace detection, LEP(ep) is not enough and we need to skip CFUNC frames to fetch the caller of such frames
…/pop With this change, the argument code of Namespace#eval cannot refer local variables around the calling line, but it should not be able to refer these values. The code is evaluated in the receiver namespace, independently from the local context.
547c63f
to
5b8d4ab
Compare
35514e5
to
b47a40b
Compare
/* Never set black_handler for VM_FRAME_MAGIC_TOP or VM_FRAME_MAGIC_CLASS | ||
* and the specval is used for namespace (rb_namespace_t) in these case | ||
*/ | ||
return VM_BLOCK_HANDLER_NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the anchor in the discussion: how to handle SPECVAL
@@ -118,7 +141,15 @@ PUREFUNC(static inline VALUE VM_CF_BLOCK_HANDLER(const rb_control_frame_t * cons | |||
static inline VALUE | |||
VM_CF_BLOCK_HANDLER(const rb_control_frame_t * const cfp) | |||
{ | |||
const VALUE *ep = VM_CF_LEP(cfp); | |||
const VALUE *ep; | |||
if (VM_ENV_NAMESPACED_P(cfp->ep)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by the fact that VM_CF_BLOCK_HANDLER()
's implementation is different from GET_BLOCK_HANDLER()
's. GET_BLOCK_HANDLER()
checks VM_ENV_NAMESPACED_P
on GET_LEP()
while VM_CF_BLOCK_HANDLER()
checks cfp->ep
instead of using VM_CF_LEP(cfp)
, which is the one actually queried later.
What allows them to be different? How is it safe for VM_CF_BLOCK_HANDLER()
to not check the EP that has the specval to be queried?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if-clause is a kind of early returrn because if VM_ENV_NAMESPACED_P(ep)
is true, then VM_ENV_LOCAL_P(ep)
is also true and ep == VM_CF_LEP(ep)
(at least for now). VM_ENV_BLOCK_HANDLER(ep)
also checks VM_ENV_NAMESPACED_P(ep)
, so GET_BLOCK_HANDLER()
returns the same result anyway.
I agree that it's confusing. So, I'll try to update GET_BLOCK_HANDLER
definition to VM_CF_BLOCK_HANDLER(GET_CFP())
and check if it will work correctly.
This change is to update the namespace management. The main purpose is to fix bugs in corner cases to determine the current namespaces. I expect that this fix can solve bugs/problems to run RubyGems/Bundler with namespaces.
Core changes are:
VM_CF_LEP
orVM_EP_RUBY_LEP
) basicallycfp->ep[VM_ENV_DATA_INDEX_SPECVAL]
) to store the namespace of the frameVM_FRAME_MAGIC_TOP
orVM_FRAME_MAGIC_CLASS
vm->load_path
and other values, then start referring the root namespace (and itsload_path
and other fields) for simplicityVM_FRAME_FLAG_NS_REQUIRE
TODO:
top_self
under namespaces (#to_s
and#inspect
)rb_namespace_t
(especiallyloading_table_entry
)namespace_entry_memsize
vm_set_main_stack(ec, iseq); // TODO: not need to set the namespace?
Add tests about: