From 284a6fb28c81d21f98d2e087e7fda2460641094c Mon Sep 17 00:00:00 2001
From: Xavier Noria <fxn@hashref.com>
Date: Sat, 5 Oct 2024 13:01:28 +0200
Subject: [PATCH] Replace TracePoint with const_added for explicit namespaces

---
 .github/workflows/ci.yml                     |   9 --
 lib/zeitwerk.rb                              |   4 +-
 lib/zeitwerk/core_ext/module.rb              |  10 ++
 lib/zeitwerk/explicit_namespace.rb           | 124 +++++++++++--------
 lib/zeitwerk/loader.rb                       |  14 ++-
 lib/zeitwerk/loader/callbacks.rb             |   2 +-
 test/lib/zeitwerk/test_explicit_namespace.rb | 114 ++---------------
 test/lib/zeitwerk/test_ruby_compatibility.rb |  41 ------
 test/lib/zeitwerk/test_unload.rb             |   8 +-
 test/lib/zeitwerk/test_unregister.rb         |   4 +-
 test/support/loader_test.rb                  |   3 +-
 zeitwerk.gemspec                             |   2 +-
 12 files changed, 113 insertions(+), 222 deletions(-)
 create mode 100644 lib/zeitwerk/core_ext/module.rb

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 1a613601..8c745cb8 100644
--- a/.github/workflows/ci.yml
+++ b/.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"
diff --git a/lib/zeitwerk.rb b/lib/zeitwerk.rb
index 3b81608b..91f1672c 100644
--- a/lib/zeitwerk.rb
+++ b/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
diff --git a/lib/zeitwerk/core_ext/module.rb b/lib/zeitwerk/core_ext/module.rb
new file mode 100644
index 00000000..c989485e
--- /dev/null
+++ b/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
diff --git a/lib/zeitwerk/explicit_namespace.rb b/lib/zeitwerk/explicit_namespace.rb
index e25cb3f8..35a84144 100644
--- a/lib/zeitwerk/explicit_namespace.rb
+++ b/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
diff --git a/lib/zeitwerk/loader.rb b/lib/zeitwerk/loader.rb
index c5208326..56780a24 100644
--- a/lib/zeitwerk/loader.rb
+++ b/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
diff --git a/lib/zeitwerk/loader/callbacks.rb b/lib/zeitwerk/loader/callbacks.rb
index bb391c5b..62d52a2b 100644
--- a/lib/zeitwerk/loader/callbacks.rb
+++ b/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
diff --git a/test/lib/zeitwerk/test_explicit_namespace.rb b/test/lib/zeitwerk/test_explicit_namespace.rb
index 7a74b719..0a69d600 100644
--- a/test/lib/zeitwerk/test_explicit_namespace.rb
+++ b/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],
diff --git a/test/lib/zeitwerk/test_ruby_compatibility.rb b/test/lib/zeitwerk/test_ruby_compatibility.rb
index e68eea7b..faf09a62 100644
--- a/test/lib/zeitwerk/test_ruby_compatibility.rb
+++ b/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
diff --git a/test/lib/zeitwerk/test_unload.rb b/test/lib/zeitwerk/test_unload.rb
index 6859f4aa..5550faf1 100644
--- a/test/lib/zeitwerk/test_unload.rb
+++ b/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
 
diff --git a/test/lib/zeitwerk/test_unregister.rb b/test/lib/zeitwerk/test_unregister.rb
index 76a14430..738142e7 100644
--- a/test/lib/zeitwerk/test_unregister.rb
+++ b/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
diff --git a/test/support/loader_test.rb b/test/support/loader_test.rb
index 4db9ae7b..e0c98156 100644
--- a/test/support/loader_test.rb
+++ b/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
diff --git a/zeitwerk.gemspec b/zeitwerk.gemspec
index 5d6b3c75..2cfe29da 100644
--- a/zeitwerk.gemspec
+++ b/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