Reimplement support for inceptions

PROJECT_RULES.md

@@ -18,6 +18,9 @@ You should always pick the most specific option:
 * `dir`: Absolute path of a directory.
 * `abspath`: Absolute path of a file or directory.
 
+In some places we use `autoload_path` because it is even more concise. Zeitwerk
+only sets autoloads using absolute paths.
+
 Note that Zeitwerk does not deal with file or directory objects, only with paths. For brevity, we exploit this fact to adopt the convention `file`&`dir` instead of `filename`&`dirname` or somesuch.
 
 ## Paths

lib/zeitwerk/loader.rb

@@ -5,6 +5,7 @@
 
 module Zeitwerk
   class Loader
+    require_relative "loader/autoloads"
     require_relative "loader/helpers"
     require_relative "loader/callbacks"
     require_relative "loader/config"
@@ -21,14 +22,12 @@ class Loader
     MUTEX = Mutex.new
     private_constant :MUTEX
 
-    # Maps absolute paths for which an autoload has been set ---and not
-    # executed--- to their corresponding Zeitwerk::Cref object.
+    # Used by the loader to keep track of the autoloads it defines.
     #
-    #   "/Users/fxn/blog/app/models/user.rb"          => #<Zeitwerk::Cref:... @mod=Object, @cname=:User, ...>,
-    #   "/Users/fxn/blog/app/models/hotel/pricing.rb" => #<Zeitwerk::Cref:... @mod=Hotel, @cname=:Pricing, ...>,
-    #   ...
+    # To keep memory usage low, this collection grows and shrinks. Entries are
+    # added when autoloads are defined, and deleted when autoloads are executed.
     #
-    # @sig Hash[String, Zeitwerk::Cref]
+    # @sig Zeitwerk::Loader::Autoloads
     attr_reader :autoloads
     internal :autoloads
 
@@ -96,7 +95,7 @@ class Loader
     def initialize
       super
 
-      @autoloads       = {}
+      @autoloads       = Autoloads.new
       @autoloaded_dirs = []
       @to_unload       = {}
       @namespace_dirs  = Hash.new { |h, cpath| h[cpath] = [] }
@@ -151,19 +150,19 @@ def unload
         # is enough.
         unloaded_files = Set.new
 
-        autoloads.each do |abspath, cref|
+        autoloads.each do |cref, autoload_path|
           if cref.autoload?
             unload_autoload(cref)
           else
             # Could happen if loaded with require_relative. That is unsupported,
             # and the constant path would escape unloadable_cpath? This is just
             # defensive code to clean things up as much as we are able to.
             unload_cref(cref)
-            unloaded_files.add(abspath) if ruby?(abspath)
+            unloaded_files.add(autoload_path) if ruby?(autoload_path)
           end
         end
 
-        to_unload.each do |cref, abspath|
+        to_unload.each do |cref, loaded_feature|
           unless on_unload_callbacks.empty?
             begin
               value = cref.get
@@ -172,12 +171,12 @@ def unload
               # autoload failed to define the expected constant but the user
               # rescued the exception.
             else
-              run_on_unload_callbacks(cref.path, value, abspath)
+              run_on_unload_callbacks(cref.path, value, loaded_feature)
             end
           end
 
           unload_cref(cref)
-          unloaded_files.add(abspath) if ruby?(abspath)
+          unloaded_files.add(loaded_feature) if ruby?(loaded_feature)
         end
 
         unless unloaded_files.empty?
@@ -456,7 +455,7 @@ def all_dirs
 
     # @sig (Module, Symbol, String) -> void
     private def autoload_subdir(cref, subdir)
-      if autoload_path = autoload_path_set_by_me_for?(cref)
+      if autoload_path = autoloads.autoload_path_for(cref)
         if ruby?(autoload_path)
           # Scanning visited a Ruby file first, and now a directory for the same
           # constant has been found. This means we are dealing with an explicit
@@ -485,7 +484,8 @@ def all_dirs
 
     # @sig (Module, Symbol, String) -> void
     private def autoload_file(cref, file)
-      if autoload_path = cref.autoload? || Registry.inception?(cref)
+      # See https://bugs.ruby-lang.org/issues/21035.
+      if autoload_path = cref.autoload? || autoloads.autoload_path_for(cref)
         # First autoload for a Ruby file wins, just ignore subsequent ones.
         if ruby?(autoload_path)
           shadowed_files << file
@@ -518,33 +518,17 @@ def all_dirs
     end
 
     # @sig (Module, Symbol, String) -> void
-    private def define_autoload(cref, abspath)
-      cref.autoload(abspath)
+    private def define_autoload(cref, autoload_path)
+      autoloads.define(cref, autoload_path)
+      Registry.register_autoload(self, autoload_path)
 
       if logger
-        if ruby?(abspath)
-          log("autoload set for #{cref}, to be loaded from #{abspath}")
+        if ruby?(autoload_path)
+          log("autoload set for #{cref}, to be loaded from #{autoload_path}")
         else
-          log("autoload set for #{cref}, to be autovivified from #{abspath}")
+          log("autoload set for #{cref}, to be autovivified from #{autoload_path}")
         end
       end
-
-      autoloads[abspath] = cref
-      Registry.register_autoload(self, abspath)
-
-      # See why in the documentation of Zeitwerk::Registry.inceptions.
-      unless cref.autoload?
-        Registry.register_inception(cref, abspath, self)
-      end
-    end
-
-    # @sig (Module, Symbol) -> String?
-    private def autoload_path_set_by_me_for?(cref)
-      if autoload_path = cref.autoload?
-        autoload_path if autoloads.key?(autoload_path)
-      else
-        Registry.inception?(cref, self)
-      end
     end
 
     # @sig (Zeitwerk::Cref) -> void

lib/zeitwerk/loader/autoloads.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+class Zeitwerk::Loader::Autoloads
+  # @sig () -> void
+  def initialize
+    # Maps crefs for which an autoload has been set, to their respective
+    # autoload paths. Entries look like this:
+    #
+    #   #<Zeitwerk::Cref:... @mod=Object, @cname=:User, ...> => ".../app/models/user.rb"
+    #   #<Zeitwerk::Cref:... @mod=Hotel, @cname=:Pricing, ...> => ".../app/models/hotel/pricing.rb"
+    #
+    # I would expect this collection to not be necessary, since it is basically
+    # doing what Module#autoload? already does. However, there is an edge case
+    # in which Module#autoload? returns nil for an autoload just set. See
+    #
+    #   https://bugs.ruby-lang.org/issues/21035
+    #
+    # @sig Hash[Zeitwerk::Cref, String]
+    @c2a = {}
+
+    # This is the inverse of `c2a`.
+    #
+    # @sig Hash[String, Zeitwerk::Cref]
+    @a2c = {}
+  end
+
+  # @sig (Zeitwerk::Cref, String) -> void
+  def define(cref, autoload_path)
+    cref.autoload(autoload_path)
+    @c2a[cref] = autoload_path
+    @a2c[autoload_path] = cref
+  end
+
+  # @sig () { () -> (Zeitwerk::Cref, String) } -> void
+  def each(&block)
+    @c2a.each(&block)
+  end
+
+  # @sig (Zeitwerk::Cref) -> String?
+  def autoload_path_for(cref)
+    @c2a[cref]
+  end
+
+  # @sig (String) -> Zeitwerk::Cref?
+  def cref_for(autoload_path)
+    @a2c[autoload_path]
+  end
+
+  # @sig (String) -> Zeitwerk::Cref?
+  def delete(abspath)
+    cref = @a2c.delete(abspath)
+    @c2a.delete(cref)
+    cref
+  end
+
+  # @sig () -> void
+  def clear
+    @c2a.clear
+    @a2c.clear
+  end
+
+  # @sig () -> bool
+  def empty?
+    @c2a.empty? && @a2c.empty?
+  end
+end

lib/zeitwerk/loader/eager_load.rb

@@ -169,7 +169,7 @@ def load_file(path)
         next if honour_exclusions && eager_load_exclusions.member?(abspath)
 
         if ftype == :file
-          if (cref = autoloads[abspath])
+          if cref = autoloads.cref_for(abspath)
             cref.get
           end
         else

lib/zeitwerk/registry.rb

@@ -25,45 +25,6 @@ class << self
       # @sig Hash[String, Zeitwerk::Loader]
       attr_reader :autoloads
 
-      # This hash table addresses an edge case in which an autoload is ignored.
-      #
-      # For example, let's suppose we want to autoload in a gem like this:
-      #
-      #   # lib/my_gem.rb
-      #   loader = Zeitwerk::Loader.new
-      #   loader.push_dir(__dir__)
-      #   loader.setup
-      #
-      #   module MyGem
-      #   end
-      #
-      # if you require "my_gem", as Bundler would do, this happens while setting
-      # up autoloads:
-      #
-      #   1. Object.autoload?(:MyGem) returns `nil` because the autoload for
-      #      the constant is issued by Zeitwerk while the same file is being
-      #      required.
-      #   2. The constant `MyGem` is undefined while setup runs.
-      #
-      # Therefore, a directory `lib/my_gem` would autovivify a module according to
-      # the existing information. But that would be wrong.
-      #
-      # To overcome this fundamental limitation, we keep track of the constant
-      # paths that are in this situation ---in the example above, "MyGem"--- and
-      # take this collection into account for the autovivification logic.
-      #
-      # Note that you cannot generally address this by moving the setup code
-      # below the constant definition, because we want libraries to be able to
-      # use managed constants in the module body:
-      #
-      #   module MyGem
-      #     include MyConcern
-      #   end
-      #
-      # @private
-      # @sig Hash[Zeitwerk::Cref, String, Zeitwerk::Loader]]
-      attr_reader :inceptions
-
       # Registers a loader.
       #
       # @private
@@ -78,7 +39,6 @@ def unregister_loader(loader)
         loaders.delete(loader)
         gem_loaders_by_root_file.delete_if { |_, l| l == loader }
         autoloads.delete_if { |_, l| l == loader }
-        inceptions.delete_if { |_, (_, l)| l == loader }
       end
 
       # This method returns always a loader, the same instance for the same root
@@ -102,23 +62,6 @@ def unregister_autoload(abspath)
         autoloads.delete(abspath)
       end
 
-      # @private
-      # @sig (Zeitwerk::Cref, String, Zeitwerk::Loader) -> void
-      def register_inception(cref, abspath, loader)
-        inceptions[cref] = [abspath, loader]
-      end
-
-      # @private
-      # @sig (String) -> String?
-      def inception?(cref, registered_by_loader=nil)
-        if pair = inceptions[cref]
-          abspath, loader = pair
-          if registered_by_loader.nil? || registered_by_loader.equal?(loader)
-            abspath
-          end
-        end
-      end
-
       # @private
       # @sig (String) -> Zeitwerk::Loader?
       def loader_for(path)
@@ -129,13 +72,11 @@ def loader_for(path)
       # @sig (Zeitwerk::Loader) -> void
       def on_unload(loader)
         autoloads.delete_if { |_, object| object == loader }
-        inceptions.delete_if { |_, (_, object)| object == loader }
       end
     end
 
     @loaders                  = []
     @gem_loaders_by_root_file = {}
     @autoloads                = {}
-    @inceptions               = {}
   end
 end

test/lib/zeitwerk/test_autoloads.rb

@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class TestAutoloads < Minitest::Test
+  def setup
+    @autoloads      = Zeitwerk::Loader::Autoloads.new
+    @autoload_path  = "/m.rb"
+    @autoload_path2 = "/n.rb"
+    @cref           = Zeitwerk::Cref.new(Object, :M)
+    @cref2          = Zeitwerk::Cref.new(Object, :N)
+  end
+
+  test "define defines an autoload" do
+    on_teardown { remove_const :M }
+
+    @autoloads.define(@cref, @autoload_path)
+    assert_equal @autoload_path, @cref.autoload?
+  end
+
+  test "autoload_path_for returns the configured autoload path" do
+    on_teardown { remove_const :M }
+
+    @autoloads.define(@cref, @autoload_path)
+    assert_equal @autoload_path, @autoloads.autoload_path_for(@cref)
+    assert_nil @autoloads.autoload_path_for(@cref2)
+  end
+
+  test "cref_for returns the configured cref" do
+    on_teardown { remove_const :M }
+
+    @autoloads.define(@cref, @autoload_path)
+    assert_equal @cref, @autoloads.cref_for(@autoload_path)
+    assert_nil @autoloads.cref_for(@autoload_path2)
+  end
+
+  test "each iterates over the autoloads" do
+    on_teardown { remove_const :M }
+
+    @autoloads.define(@cref, @autoload_path)
+    @autoloads.each do |cref, autoload_path|
+      assert_equal @autoload_path, autoload_path
+      assert_equal Object, cref.mod
+      assert_equal :M, cref.cname
+    end
+  end
+
+  test "delete maintains the two internal collections" do
+    on_teardown { remove_const :M }
+
+    @autoloads.define(@cref, @autoload_path)
+    @autoloads.delete(@autoload_path)
+    assert @autoloads.empty?
+  end
+
+  test "a new instance is empty" do
+    assert @autoloads.empty?
+  end
+
+  test "an instance with definitions is not empty" do
+    on_teardown { remove_const :M }
+
+    @autoloads.define(@cref, @autoload_path)
+    assert !@autoloads.empty?
+  end
+
+  test "a cleared instance is empty" do
+    on_teardown { remove_const :M }
+
+    @autoloads.define(@cref, @autoload_path)
+    @autoloads.clear
+    assert @autoloads.empty?
+  end
+end