Replace TracePoint with const_added for explicit namespaces

.github/workflows/ci.yml

@@ -14,18 +14,9 @@ jobs:
           - "macos-latest"
           - "windows-latest"
         ruby-version:
-          - "2.5"
-          - "2.6"
-          - "2.7"
-          - "3.0"
-          - "3.1"
           - "3.2"
           - "3.3"
           - "head"
-        # GitHub is not able to set this combination up lately.
-        exclude:
-          - os: "macos-latest"
-            ruby-version: "2.5"
     runs-on: ${{ matrix.os }}
     steps:
       - uses: "actions/checkout@v4"

lib/zeitwerk.rb

@@ -11,10 +11,12 @@ module Zeitwerk
   require_relative "zeitwerk/inflector"
   require_relative "zeitwerk/gem_inflector"
   require_relative "zeitwerk/null_inflector"
-  require_relative "zeitwerk/kernel"
   require_relative "zeitwerk/error"
   require_relative "zeitwerk/version"
 
+  require_relative "zeitwerk/core_ext/kernel"
+  require_relative "zeitwerk/core_ext/module"
+
   # This is a dangerous method.
   #
   # @experimental

lib/zeitwerk/core_ext/module.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+module Zeitwerk::ExplicitNamespacesRegistry
+  def const_added(cname)
+    Zeitwerk::ExplicitNamespace.__on_const_added(self, cname)
+    super
+  end
+
+  Module.prepend(self)
+end

lib/zeitwerk/explicit_namespace.rb

@@ -1,93 +1,109 @@
 # frozen_string_literal: true
 
 module Zeitwerk
-  # Centralizes the logic for the trace point used to detect the creation of
-  # explicit namespaces, needed to descend into matching subdirectories right
-  # after the constant has been defined.
+  # Centralizes the logic needed to descend into matching subdirectories right
+  # after the constant for an explicit namespace has been defined.
   #
   # The implementation assumes an explicit namespace is managed by one loader.
   # Loaders that reopen namespaces owned by other projects are responsible for
   # loading their constant before setup. This is documented.
   module ExplicitNamespace # :nodoc: all
+    # Maps cpaths of explicit namespaces with their corresponding loader.
+    # Entries are added as the namespaces are found, and removed as they are
+    # autoloaded.
+    #
+    # @sig Hash[String => Zeitwerk::Loader]
+    @cpaths = {}
+
     class << self
       include RealModName
       extend Internal
 
-      # Maps constant paths that correspond to explicit namespaces according to
-      # the file system, to the loader responsible for them.
-      #
-      # @sig Hash[String, Zeitwerk::Loader]
-      attr_reader :cpaths
-      private :cpaths
-
-      # @sig Mutex
-      attr_reader :mutex
-      private :mutex
-
-      # @sig TracePoint
-      attr_reader :tracer
-      private :tracer
+      # @sig (Module, Symbol) -> void
+      internal def on_const_added(mod, cname)
+        if loader = loader_for(mod, cname)
+          namespace = mod.const_get(cname, false)
+          loader.on_namespace_loaded(namespace)
+        end
+      end
 
       # Asserts `cpath` corresponds to an explicit namespace for which `loader`
       # is responsible.
       #
       # @sig (String, Zeitwerk::Loader) -> void
       internal def register(cpath, loader)
-        mutex.synchronize do
-          cpaths[cpath] = loader
-          # We check enabled? because, looking at the C source code, enabling an
-          # enabled tracer does not seem to be a simple no-op.
-          tracer.enable unless tracer.enabled?
-        end
+        @cpaths[cpath] = loader
       end
 
       # @sig (Zeitwerk::Loader) -> void
       internal def unregister_loader(loader)
-        cpaths.delete_if { |_cpath, l| l == loader }
-        disable_tracer_if_unneeded
+        @cpaths.delete_if { _2.equal?(loader) }
       end
 
       # This is an internal method only used by the test suite.
       #
       # @sig (String) -> bool
       internal def registered?(cpath)
-        cpaths.key?(cpath)
+        @cpaths[cpath]
       end
 
+      # This is an internal method only used by the test suite.
+      #
       # @sig () -> void
-      private def disable_tracer_if_unneeded
-        mutex.synchronize do
-          tracer.disable if cpaths.empty?
-        end
+      internal def clear
+        @cpaths.clear
       end
 
-      # @sig (TracePoint) -> void
-      private def tracepoint_class_callback(event)
-        # If the class is a singleton class, we won't do anything with it so we
-        # can bail out immediately. This is several orders of magnitude faster
-        # than accessing its name.
-        return if event.self.singleton_class?
-
-        # It might be tempting to return if name.nil?, to avoid the computation
-        # of a hash code and delete call. But Ruby does not trigger the :class
-        # event on Class.new or Module.new, so that would incur in an extra call
-        # for nothing.
+      # Returns the loader registerd for cpath, if any. This method deletes
+      # cpath from @cpath if present.
+      #
+      # @sig (String) -> Zeitwerk::Loader?
+      private def loader_for(mod, cname)
+        # @cpaths.empty? is cheap and, depending on the code base, often true.
         #
-        # On the other hand, if we were called, cpaths is not empty. Otherwise
-        # the tracer is disabled. So we do need to go ahead with the hash code
-        # computation and delete call.
-        if loader = cpaths.delete(real_mod_name(event.self))
-          loader.on_namespace_loaded(event.self)
-          disable_tracer_if_unneeded
-        end
+        # Note that due to the way Zeitwerk works, namespaces are registered
+        # necessarily before their constant is defined, so the race codintion
+        # due to the gap from here to the delete call down below would not
+        # introduce a logic flaw.
+        #
+        # On one hand, if @cpaths is empty and a new entry is created after this
+        # check, it won't be for mod::cname anyway.
+        #
+        # On the other hand, if @cpaths is not empty, what happens with @cpaths
+        # during the gap is fine, because delete has the last word anyway.
+        return if @cpaths.empty?
+
+        # Module#const_added is triggered when an autoload is defined too. This
+        # callback is only for constants that are defined for real. In the case
+        # of inceptions we get a false nil, but this is covered in the loader by
+        # doing things in a certain order.
+        return if mod.autoload?(cname, false)
+
+        # I benchmarked this against using pairs [mod, cname] as keys, and
+        # strings won.
+        cpath = mod.equal?(Object) ? cname.name : "#{real_mod_name(mod)}::#{cname}"
+        @cpaths.delete(cpath)
       end
-    end
 
-    @cpaths = {}
-    @mutex  = Mutex.new
+      module Synchronized
+        extend Internal
+
+        MUTEX = Mutex.new
+
+        internal def register(...)
+          MUTEX.synchronize { super }
+        end
+
+        internal def unregister_loader(...)
+          MUTEX.synchronize { super }
+        end
 
-    # We go through a method instead of defining a block mainly to have a better
-    # label when profiling.
-    @tracer = TracePoint.new(:class, &method(:tracepoint_class_callback))
+        private def loader_for(...)
+          MUTEX.synchronize { super }
+        end
+      end
+
+      prepend Synchronized unless RUBY_ENGINE == "ruby"
+    end
   end
 end

lib/zeitwerk/loader.rb

@@ -469,7 +469,7 @@ def all_dirs
           # Registering is idempotent, and we have to keep the autoload pointing
           # to the file. This may run again if more directories are found later
           # on, no big deal.
-          register_explicit_namespace(cref.path)
+          register_explicit_namespace(cref)
         end
         # If the existing autoload points to a file, it has to be preserved, if
         # not, it is fine as it is. In either case, we do not need to override.
@@ -515,8 +515,12 @@ def all_dirs
 
       log("earlier autoload for #{cref.path} discarded, it is actually an explicit namespace defined in #{file}") if logger
 
+      # In the case of inception, order matters: When Module#const_added is
+      # triggered by the autoload, the autoload? predicate in the callback
+      # returns nil for inceptions, so execution continures. If Hash#delete
+      # later returns a loader, it is going to const_get an enter circularity.
       define_autoload(cref, file)
-      register_explicit_namespace(cref.path)
+      register_explicit_namespace(cref)
     end
 
     # @sig (Module, Symbol, String) -> void
@@ -549,9 +553,9 @@ def all_dirs
       end
     end
 
-    # @sig (String) -> void
-    private def register_explicit_namespace(cpath)
-      ExplicitNamespace.__register(cpath, self)
+    # @sig (Zeitwerk::Cref) -> void
+    private def register_explicit_namespace(cref)
+      ExplicitNamespace.__register(cref.path, self)
     end
 
     # @sig (String) -> void

lib/zeitwerk/loader/callbacks.rb

@@ -73,7 +73,7 @@ module Zeitwerk::Loader::Callbacks
   end
 
   # Invoked when a class or module is created or reopened, either from the
-  # tracer or from module autovivification. If the namespace has matching
+  # const_added or from module autovivification. If the namespace has matching
   # subdirectories, we descend into them now.
   #
   # @private

test/lib/zeitwerk/test_explicit_namespace.rb

@@ -53,6 +53,18 @@ module Namespace; end
     end
   end
 
+  test "explicit namespaces defined with an explicit constant assignment are loaded correctly" do
+    files = [
+      ["hotel.rb", "Hotel = Class.new; Hotel::X = 1"],
+      ["hotel/pricing.rb", "class Hotel::Pricing; end"]
+    ]
+    with_setup(files) do
+      assert_kind_of Class, Hotel
+      assert Hotel::X
+      assert Hotel::Pricing
+    end
+  end
+
   test "explicit namespaces are loaded correctly even if #name is overridden" do
     files = [
       ["hotel.rb", <<~RUBY],
@@ -138,108 +150,6 @@ def self.name
     end
   end
 
-  # As of this writing, a tracer on the :class event does not seem to have any
-  # performance penalty in an ordinary code base. But I prefer to precisely
-  # control that we use a tracer only if needed in case this issue
-  #
-  #     https://bugs.ruby-lang.org/issues/14104
-  #
-  # goes forward.
-  def tracer
-    Zeitwerk::ExplicitNamespace.send(:tracer)
-  end
-
-  test "the tracer starts disabled" do
-    assert !tracer.enabled?
-  end
-
-  test "simple autoloading does not enable the tracer" do
-    files = [["x.rb", "X = true"]]
-    with_setup(files) do
-      assert !tracer.enabled?
-      assert X
-      assert !tracer.enabled?
-    end
-  end
-
-  test "autovivification does not enable the tracer, one directory" do
-    files = [["foo/bar.rb", "module Foo::Bar; end"]]
-    with_setup(files) do
-      assert !tracer.enabled?
-      assert Foo::Bar
-      assert !tracer.enabled?
-    end
-  end
-
-  test "autovivification does not enable the tracer, two directories" do
-    files = [
-      ["rd1/foo/bar.rb", "module Foo::Bar; end"],
-      ["rd2/foo/baz.rb", "module Foo::Baz; end"],
-    ]
-    with_setup(files) do
-      assert !tracer.enabled?
-      assert Foo::Bar
-      assert !tracer.enabled?
-    end
-  end
-
-  test "explicit namespaces enable the tracer until loaded" do
-    files = [
-      ["hotel.rb", "class Hotel; end"],
-      ["hotel/pricing.rb", "class Hotel::Pricing; end"]
-    ]
-    with_setup(files) do
-      assert tracer.enabled?
-      assert Hotel
-      assert !tracer.enabled?
-      assert Hotel::Pricing
-      assert !tracer.enabled?
-    end
-  end
-
-  test "the tracer is enabled until everything is loaded" do
-    files = [
-      ["a/m.rb", "module M; end"], ["a/m/n.rb", "M::N = true"],
-      ["b/x.rb", "module X; end"], ["b/x/y.rb", "X::Y = true"],
-    ]
-    with_files(files) do
-      new_loader(dirs: "a")
-      assert tracer.enabled?
-
-      new_loader(dirs: "b")
-      assert tracer.enabled?
-
-      assert M
-      assert tracer.enabled?
-
-      assert X
-      assert !tracer.enabled?
-    end
-  end
-
-  # This is a regression test.
-  test "the tracer handles singleton classes" do
-    files = [
-      ["hotel.rb", <<-EOS],
-        class Hotel
-          class << self
-            def x
-              1
-            end
-          end
-        end
-      EOS
-      ["hotel/pricing.rb", "class Hotel::Pricing; end"],
-      ["car.rb", "class Car; end"],
-      ["car/pricing.rb", "class Car::Pricing; end"],
-    ]
-    with_setup(files) do
-      assert tracer.enabled?
-      assert_equal 1, Hotel.x
-      assert tracer.enabled?
-    end
-  end
-
   test "non-hashable explicit namespaces are supported" do
     files = [
       ["m.rb", <<~EOS],