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,14 @@
+# frozen_string_literal: true
+
+module Zeitwerk::ExplicitNamespacesRegistry
+  def const_added(cname)
+    # Module#autoload triggers const_added too, but we are only interested in
+    # "actual" added constants.
+    unless autoload?(cname, false)
+      Zeitwerk::ExplicitNamespace.__on_const_added(self, cname)
+    end
+    super
+  end
+
+  Module.prepend(self)
+end

lib/zeitwerk/explicit_namespace.rb

@@ -1,8 +1,7 @@
 # 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
+  # Centralizes the logic needed to descend into matching subdirectories right
   # after the constant has been defined.
   #
   # The implementation assumes an explicit namespace is managed by one loader.
@@ -13,81 +12,49 @@ 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 (Module, Symbol) -> void
+      internal def on_const_added(mod, cname)
+        # This is common and fast if true.
+        return if @cpaths.empty?
 
-      # @sig Mutex
-      attr_reader :mutex
-      private :mutex
+        # We are only interested in class/module objects.
+        value = mod.const_get(cname, false)
+        return unless value.is_a?(Module)
 
-      # @sig TracePoint
-      attr_reader :tracer
-      private :tracer
+        if loader = @cpaths.delete(real_mod_name(value))
+          loader.on_namespace_loaded(value)
+        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 == 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
-      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.
-        #
-        # 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
+      internal def clear
+        @cpaths.clear
       end
     end
 
+    # @sig Hash[String, Zeitwerk::Loader]
     @cpaths = {}
-    @mutex  = Mutex.new
-
-    # 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))
   end
 end

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],

test/lib/zeitwerk/test_ruby_compatibility.rb

@@ -261,47 +261,6 @@ class C; end
     end
   end
 
-  # This is why we issue a namespace_dirs.delete call in the tracer block, to
-  # ignore events triggered by reopenings.
-  test "tracing :class calls you back on creation and on reopening" do
-    on_teardown do
-      @tracer.disable
-      remove_const :C, from: self.class
-      remove_const :M, from: self.class
-    end
-
-    traced = []
-    @tracer = TracePoint.trace(:class) do |tp|
-      traced << tp.self
-    end
-
-    2.times do
-      class C; end
-      module M; end
-    end
-
-    assert_equal [C, M, C, M], traced
-  end
-
-  # Computing hash codes is costly and we want the tracer to be as efficient as
-  # possible. The TP callback doesn't short-circuit anonymous classes/modules
-  # because Class.new/Module.new do not trigger the :class event. We leverage
-  # this property to save a nil? call.
-  #
-  # However, if that changes in future versions of Ruby, this test would tell us
-  # and we could revisit the callback implementation.
-  test "trace points on the :class events don't get called on Class.new and Module.new" do
-    on_teardown { @tracer.disable }
-
-    $tracer_for_anonymous_class_and_modules_called = false
-    @tracer = TracePoint.trace(:class) { $tracer_for_anonymous_class_and_modules_called = true }
-
-    Class.new
-    Module.new
-
-    assert !$tracer_for_anonymous_class_and_modules_called
-  end
-
   # If the user issues a require call with a Pathname object for a path that is
   # autoloadable, we are still able to intercept it because $LOADED_FEATURES
   # stores it as a string and loader_for is able to find its loader. During

test/lib/zeitwerk/test_unload.rb

@@ -131,14 +131,14 @@ def self.name
     ]
     with_files(files) do
       la = new_loader(dirs: "a")
-      assert Zeitwerk::ExplicitNamespace.send(:cpaths)["M"] == la
+      assert Zeitwerk::ExplicitNamespace.__registered?("M") == la
 
       lb = new_loader(dirs: "b")
-      assert Zeitwerk::ExplicitNamespace.send(:cpaths)["X"] == lb
+      assert Zeitwerk::ExplicitNamespace.__registered?("X") == lb
 
       la.unload
-      assert_nil Zeitwerk::ExplicitNamespace.send(:cpaths)["M"]
-      assert Zeitwerk::ExplicitNamespace.send(:cpaths)["X"] == lb
+      assert_nil Zeitwerk::ExplicitNamespace.__registered?("M")
+      assert Zeitwerk::ExplicitNamespace.__registered?("X") == lb
     end
   end
 

test/lib/zeitwerk/test_unregister.rb

@@ -26,13 +26,13 @@ class TestUnregister < LoaderTest
     assert !registry.gem_loaders_by_root_file.values.include?(loader1)
     assert !registry.autoloads.values.include?(loader1)
     assert !registry.inceptions.values.any? {|_, l| l == loader1}
-    assert !Zeitwerk::ExplicitNamespace.send(:cpaths).values.include?(loader1)
+    assert !Zeitwerk::ExplicitNamespace.__registered?("dummy1")
 
     assert registry.loaders.include?(loader2)
     assert registry.gem_loaders_by_root_file.values.include?(loader2)
     assert registry.autoloads.values.include?(loader2)
     assert registry.inceptions.values.any? {|_, l| l == loader2}
-    assert Zeitwerk::ExplicitNamespace.send(:cpaths).values.include?(loader2)
+    assert Zeitwerk::ExplicitNamespace.__registered?("dummy2") == loader2
   end
 
   test 'with_loader yields and unregisters' do

test/support/loader_test.rb

@@ -42,8 +42,7 @@ def reset_registry
   end
 
   def reset_explicit_namespace
-    Zeitwerk::ExplicitNamespace.send(:cpaths).clear
-    Zeitwerk::ExplicitNamespace.send(:tracer).disable
+    Zeitwerk::ExplicitNamespace.__clear
   end
 
   def teardown

zeitwerk.gemspec

@@ -25,5 +25,5 @@ Gem::Specification.new do |spec|
     "bug_tracker_uri" => "https://github.com/fxn/zeitwerk/issues"
   }
 
-  spec.required_ruby_version = ">= 2.5"
+  spec.required_ruby_version = ">= 3.2"
 end