From e62738c20e9ba0a4ab47a168e06b938685af76be Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Thu, 3 Oct 2024 21:58:33 +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 | 14 +++ lib/zeitwerk/explicit_namespace.rb | 75 ++++-------- 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 +- 11 files changed, 59 insertions(+), 217 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..ec887e38 --- /dev/null +++ b/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 diff --git a/lib/zeitwerk/explicit_namespace.rb b/lib/zeitwerk/explicit_namespace.rb index e25cb3f8..0b0708cd 100644 --- a/lib/zeitwerk/explicit_namespace.rb +++ b/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 Mutex - attr_reader :mutex - private :mutex - - # @sig TracePoint - attr_reader :tracer - private :tracer + # @sig (Module, Symbol) -> void + internal def on_const_added(mod, cname) + loader = @mutex.synchronize do + return if @cpaths.empty? + cpath = mod.equal?(Object) ? cname.name : "#{real_mod_name(mod)}::#{cname}" + @cpaths.delete(cpath) + end + loader&.on_namespace_loaded(mod.const_get(cname, false)) + 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? + @mutex.synchronize do + @cpaths[cpath] = loader end end # @sig (Zeitwerk::Loader) -> void internal def unregister_loader(loader) - cpaths.delete_if { |_cpath, l| l == loader } - disable_tracer_if_unneeded + @mutex.synchronize do + @cpaths.delete_if { _2.equal?(loader) } + end 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 @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)) + @mutex = Mutex.new end end 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