From 177ab23371d69bc960b9d99fa5f4143f125d26b1 Mon Sep 17 00:00:00 2001
From: Xavier Noria <fxn@hashref.com>
Date: Sun, 16 Feb 2025 00:39:57 +0100
Subject: [PATCH] Docs improvements

---
 lib/zeitwerk/cref/map.rb       | 49 +++++++++++++++++++++++-----------
 lib/zeitwerk/internal.rb       |  1 +
 lib/zeitwerk/null_inflector.rb |  1 +
 3 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/lib/zeitwerk/cref/map.rb b/lib/zeitwerk/cref/map.rb
index bac263ee..dcd35a04 100644
--- a/lib/zeitwerk/cref/map.rb
+++ b/lib/zeitwerk/cref/map.rb
@@ -2,33 +2,40 @@
 
 # This class emulates a hash table whose keys are of type Zeitwerk::Cref.
 #
-# It is a synchronized hash of hashes. The first one, stored in `@map`, is keyed
-# on class and module object IDs. Then, each one of them stores a hash table
-# keyed on constant names, where we finally store the values.
+# It is a synchronized 2-level hash. The keys of the top one, stored in `@map`,
+# are class and module objects, but their hash code is forced to be their object
+# IDs (see why below). Then, each one of them stores a hash table keyed on
+# constant names as symbols. We finally store the values in those.
 #
 # For example, if we store values 0, 1, and 2 for the crefs that would
 # correspond to `M::X`, `M::Y`, and `N::Z`, the map will look like this:
 #
 #   { M => { X: 0, :Y => 1 }, N => { Z: 2 } }
 #
-# Why not use simple hash tables of type Hash[Module, Symbol]? Because class and
-# module objects are not guaranteed to be hashable, the `hash` method may have
-# been overridden:
+# This structure is internal, so only the needed interface is implemented.
+#
+# Why not use tables that map pairs [Module, Symbol] to their values? Because
+# class and module objects are not guaranteed to be hashable, the `hash` method
+# may have been overridden:
 #
 #   /~https://github.com/fxn/zeitwerk/issues/188
 #
-# Another option would be to make crefs hashable. I tried with hash code
+# We can also use a 1-level hash whose keys are the corresponding class and
+# module names. In the example above it would be:
+#
+#   { "M::X" => 0, "M::Y" => 1, "N::Z" => 2 }
 #
-#  real_mod_hash(mod) ^ cname.hash
+# The gem used this approach for several years.
+#
+# Another option would be to make crefs hashable. I tried with hash code
 #
-# and the matching eql?, but that was about 1.8x slower than a hash keyed by
-# class and module names.
+#   real_mod_hash(mod) ^ cname.hash
 #
-# The gem used hashes keyed by class and module names, but that felt like an
-# unnecessary dependency on said names, our natural objects are the crefs.
+# and the matching eql?, but that was about 1.8x slower.
 #
-# On the other hand, an unsynchronized hash based on constant paths is 1.6x
-# slower than this map.
+# Finally, I came with this solution which is 1.6x faster than the previous one
+# based on class and module names, even being synchronized. Also, client code
+# feels natural, since crefs are central objects in Zeitwerk's implementation.
 class Zeitwerk::Cref::Map # :nodoc: all
   def initialize
     @map = {}
@@ -36,6 +43,7 @@ def initialize
     @mutex = Mutex.new
   end
 
+  # @sig (Zeitwerk::Cref, Object) -> Object
   def []=(cref, value)
     @mutex.synchronize do
       cnames = (@map[cref.mod] ||= {})
@@ -43,18 +51,22 @@ def []=(cref, value)
     end
   end
 
+  # @sig (Zeitwerk::Cref) -> Object?
   def [](cref)
     @mutex.synchronize do
       @map[cref.mod]&.[](cref.cname)
     end
   end
 
+  # @sig (Zeitwerk::Cref) -> Object?
   def delete(cref)
     delete_mod_cname(cref.mod, cref.cname)
   end
 
   # Ad-hoc for loader_for, called from const_added. That is a hot path, I prefer
   # to not create a cref in every call, since that is global.
+  #
+  # @sig (Module, Symbol) -> Object?
   def delete_mod_cname(mod, cname)
     @mutex.synchronize do
       if cnames = @map[mod]
@@ -65,6 +77,7 @@ def delete_mod_cname(mod, cname)
     end
   end
 
+  # @sig (Object) -> void
   def delete_by_value(value)
     @mutex.synchronize do
       @map.delete_if do |mod, cnames|
@@ -74,7 +87,9 @@ def delete_by_value(value)
     end
   end
 
-  # Order is undefined.
+  # Order of yielded crefs is undefined.
+  #
+  # @sig () { () -> Zeitwerk::Cref } -> void
   def each_key
     @mutex.synchronize do
       @map.each do |mod, cnames|
@@ -85,13 +100,15 @@ def each_key
     end
   end
 
+  # @sig () -> void
   def clear
     @mutex.synchronize do
       @map.clear
     end
   end
 
-  def empty?
+  # @sig () -> bool
+  def empty? # for tests
     @mutex.synchronize do
       @map.empty?
     end
diff --git a/lib/zeitwerk/internal.rb b/lib/zeitwerk/internal.rb
index 7984281b..dc95f698 100644
--- a/lib/zeitwerk/internal.rb
+++ b/lib/zeitwerk/internal.rb
@@ -2,6 +2,7 @@
 
 # This is a private module.
 module Zeitwerk::Internal
+  # @sig (Symbol) -> void
   def internal(method_name)
     private method_name
 
diff --git a/lib/zeitwerk/null_inflector.rb b/lib/zeitwerk/null_inflector.rb
index 195223b8..a984c692 100644
--- a/lib/zeitwerk/null_inflector.rb
+++ b/lib/zeitwerk/null_inflector.rb
@@ -1,4 +1,5 @@
 class Zeitwerk::NullInflector
+  # @sig (String, String) -> String
   def camelize(basename, _abspath)
     basename
   end