Skip to content

Conversation

tagomoris
Copy link
Contributor

@tagomoris tagomoris commented May 28, 2025

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:

  • Determine the current namespace by the local context (VM_CF_LEP or VM_EP_RUBY_LEP) basically
  • Stop using stacks for the namespace management
  • Start using control frames (especially cfp->ep[VM_ENV_DATA_INDEX_SPECVAL]) to store the namespace of the frame
    • This is valid only when the frame type is VM_FRAME_MAGIC_TOP or VM_FRAME_MAGIC_CLASS
    • Those frame types don't have block handlers (block handlers are stored in specval for other frame types)
  • Remove vm->load_path and other values, then start referring the root namespace (and its load_path and other fields) for simplicity
  • Stop using refinements to determine the loading namespace by setting VM_FRAME_FLAG_NS_REQUIRE

TODO:

  • Delete the unused code about the current namespace management in namespace.c
  • Define singleton methods on top_self under namespaces (#to_s and #inspect)
  • Implement freeing rb_namespace_t (especially loading_table_entry)
  • Update namespace_entry_memsize
  • Implement global variable handling based on root/main namespaces
  • Check vm_set_main_stack(ec, iseq); // TODO: not need to set the namespace?

Add tests about:

  • Basic namespace behavior
  • Current namespace management under CFUNC calls
  • Global variables in main/user namespaces

@tagomoris tagomoris force-pushed the namespace-using-control-frame branch from 9cb95e9 to b0193f3 Compare June 1, 2025 03:19
@tagomoris tagomoris force-pushed the namespace-using-control-frame branch from fd04acf to 88e1fad Compare June 14, 2025 04:52
Copy link

launchable-app bot commented Jun 15, 2025

Tests Failed

✖️no tests failed ✔️62282 tests passed(2 flakes)

@tagomoris tagomoris force-pushed the namespace-using-control-frame branch from 139ed12 to a5bbd18 Compare June 15, 2025 10:04
@tagomoris tagomoris force-pushed the namespace-using-control-frame branch from a5bbd18 to 0ac3835 Compare June 17, 2025 14:02
@tagomoris tagomoris force-pushed the namespace-using-control-frame branch from 0ac3835 to 1e43ad1 Compare June 29, 2025 00:48
Satoshi Tagomori added 10 commits August 10, 2025 14:48
…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.
@tagomoris tagomoris force-pushed the namespace-using-control-frame branch from 547c63f to 5b8d4ab Compare August 11, 2025 04:47
@tagomoris tagomoris force-pushed the namespace-using-control-frame branch from 35514e5 to b47a40b Compare August 11, 2025 08:51
/* 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;
Copy link
Contributor Author

@tagomoris tagomoris Aug 12, 2025

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)) {
Copy link
Member

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?

Copy link
Contributor Author

@tagomoris tagomoris Aug 23, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants