From 866872acc245a008c05a1866898b7f3b66eeec00 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Sat, 5 Nov 2022 18:00:04 +0100 Subject: [PATCH] Raise Zeitwerk::SetupRequired if trying to load too early Nowadays you cannot load before calling setup, it silently does nothing in trivial cases like having no root directories configured, and raises in some random inner place otherwise. Better tell the user this is required in a controlled way, and with a helpful error message. --- lib/zeitwerk/error.rb | 6 ++++ lib/zeitwerk/loader.rb | 23 ++++++++++--- lib/zeitwerk/loader/eager_load.rb | 5 +++ test/lib/zeitwerk/test_eager_load.rb | 15 +++++++++ test/lib/zeitwerk/test_eager_load_dir.rb | 12 +++++-- .../lib/zeitwerk/test_eager_load_namespace.rb | 33 +++++++++++++++---- test/lib/zeitwerk/test_logging.rb | 6 ++-- test/lib/zeitwerk/test_reloading.rb | 6 ++++ test/lib/zeitwerk/test_unload.rb | 6 ++++ test/support/loader_test.rb | 7 +++- 10 files changed, 101 insertions(+), 18 deletions(-) diff --git a/lib/zeitwerk/error.rb b/lib/zeitwerk/error.rb index cfbbb896..d6d92853 100644 --- a/lib/zeitwerk/error.rb +++ b/lib/zeitwerk/error.rb @@ -12,4 +12,10 @@ def initialize class NameError < ::NameError end + + class SetupRequired < Error + def initialize + super("please, finish your configuration and call Zeitwerk::Loader#setup once all is ready") + end + end end diff --git a/lib/zeitwerk/loader.rb b/lib/zeitwerk/loader.rb index e63237cf..c557da2f 100644 --- a/lib/zeitwerk/loader.rb +++ b/lib/zeitwerk/loader.rb @@ -140,6 +140,8 @@ def setup # @sig () -> void def unload mutex.synchronize do + raise Zeitwerk::SetupRequired unless @setup + # We are going to keep track of the files that were required by our # autoloads to later remove them from $LOADED_FEATURES, thus making them # loadable by Kernel#require again. @@ -216,6 +218,7 @@ def unload # @sig () -> void def reload raise ReloadingDisabledError unless reloading_enabled? + raise Zeitwerk::SetupRequired unless @setup unload recompute_ignored_paths @@ -283,19 +286,31 @@ def for_gem(warn_on_extra_files: true) Registry.loader_for_gem(called_from, warn_on_extra_files: warn_on_extra_files) end - # Broadcasts `eager_load` to all loaders. + # Broadcasts `eager_load` to all loaders. Those that have not been setup + # are skipped. # # @sig () -> void def eager_load_all - Registry.loaders.each(&:eager_load) + Registry.loaders.each do |loader| + begin + loader.eager_load + rescue Zeitwerk::SetupRequired + # This is fine, we eager load what can be eager loaded. + end + end end - # Broadcasts `eager_load_namespace` to all loaders. + # Broadcasts `eager_load_namespace` to all loaders. Those that have not + # been setup are skipped. # # @sig (Module) -> void def eager_load_namespace(mod) Registry.loaders.each do |loader| - loader.eager_load_namespace(mod) + begin + loader.eager_load_namespace(mod) + rescue Zeitwerk::SetupRequired + # This is fine, we eager load what can be eager loaded. + end end end diff --git a/lib/zeitwerk/loader/eager_load.rb b/lib/zeitwerk/loader/eager_load.rb index e0be27fd..44998cd1 100644 --- a/lib/zeitwerk/loader/eager_load.rb +++ b/lib/zeitwerk/loader/eager_load.rb @@ -9,6 +9,7 @@ module Zeitwerk::Loader::EagerLoad def eager_load(force: false) mutex.synchronize do break if @eager_loaded + raise Zeitwerk::SetupRequired unless @setup log("eager load start") if logger @@ -29,6 +30,8 @@ def eager_load(force: false) # @sig (String | Pathname) -> void def eager_load_dir(path) + raise Zeitwerk::SetupRequired unless @setup + abspath = File.expand_path(path) raise Zeitwerk::Error.new("#{abspath} is not a directory") unless dir?(abspath) @@ -67,6 +70,8 @@ def eager_load_dir(path) # @sig (Module) -> void def eager_load_namespace(mod) + raise Zeitwerk::SetupRequired unless @setup + unless mod.is_a?(Module) raise Zeitwerk::Error, "#{mod.inspect} is not a class or module object" end diff --git a/test/lib/zeitwerk/test_eager_load.rb b/test/lib/zeitwerk/test_eager_load.rb index ec9fd6da..023b0690 100644 --- a/test/lib/zeitwerk/test_eager_load.rb +++ b/test/lib/zeitwerk/test_eager_load.rb @@ -88,6 +88,15 @@ class App1::Foo::Bar::Baz end end + test "skips loaders that are not ready" do + files = [["x.rb", "X = 1"]] + with_setup(files) do + new_loader(setup: false) # should be skipped + Zeitwerk::Loader.eager_load_all + assert required?(files) + end + end + test "eager loads gems" do on_teardown do remove_const :MyGem @@ -383,4 +392,10 @@ module DbAdapters::MysqlAdapter assert_equal ["X", "Y"], loaded end end + + test "raises if called before setup" do + assert_raises(Zeitwerk::SetupRequired) do + loader.eager_load + end + end end diff --git a/test/lib/zeitwerk/test_eager_load_dir.rb b/test/lib/zeitwerk/test_eager_load_dir.rb index 16257b80..7dfc3767 100644 --- a/test/lib/zeitwerk/test_eager_load_dir.rb +++ b/test/lib/zeitwerk/test_eager_load_dir.rb @@ -298,8 +298,10 @@ def loader.actual_eager_load_dir(*) end test "raises Zeitwerk::Error if the argument is not a directory" do - e = assert_raises(Zeitwerk::Error) { loader.eager_load_dir(__FILE__) } - assert_equal "#{__FILE__} is not a directory", e.message + with_setup([]) do + e = assert_raises(Zeitwerk::Error) { loader.eager_load_dir(__FILE__) } + assert_equal "#{__FILE__} is not a directory", e.message + end end test "raises if the argument is not managed by the loader" do @@ -308,4 +310,10 @@ def loader.actual_eager_load_dir(*) assert_equal "I do not manage #{__dir__}", e.message end end + + test "raises if called before setup" do + assert_raises(Zeitwerk::SetupRequired) do + loader.eager_load_dir(__dir__) + end + end end diff --git a/test/lib/zeitwerk/test_eager_load_namespace.rb b/test/lib/zeitwerk/test_eager_load_namespace.rb index 2e83ba2d..7a5b90fe 100644 --- a/test/lib/zeitwerk/test_eager_load_namespace.rb +++ b/test/lib/zeitwerk/test_eager_load_namespace.rb @@ -166,18 +166,22 @@ def loader.actual_eager_load_dir(*) end test "raises if the argument is not a class or module object" do - e = assert_raises(Zeitwerk::Error) do - loader.eager_load_namespace(self.class.name) + with_setup([]) do + e = assert_raises(Zeitwerk::Error) do + loader.eager_load_namespace(self.class.name) + end + assert_equal %Q("#{self.class.name}" is not a class or module object), e.message end - assert_equal %Q("#{self.class.name}" is not a class or module object), e.message end test "raises if the argument is not a class or module object, even if eager loaded" do - loader.eager_load - e = assert_raises(Zeitwerk::Error) do - loader.eager_load_namespace(self.class.name) + with_setup([]) do + loader.eager_load + e = assert_raises(Zeitwerk::Error) do + loader.eager_load_namespace(self.class.name) + end + assert_equal %Q("#{self.class.name}" is not a class or module object), e.message end - assert_equal %Q("#{self.class.name}" is not a class or module object), e.message end end @@ -389,6 +393,12 @@ module CN; end end end + test "raises if called before setup" do + assert_raises(Zeitwerk::SetupRequired) do + loader.eager_load_namespace(self.class) + end + end + test "the class method broadcasts the call to all registered loaders" do files = [ ["a/m/x.rb", "M::X = 1"], @@ -413,4 +423,13 @@ module CN; end assert !required?(files[4]) end end + + test "the class method skips loaders that are not ready" do + files = [["m/x.rb", "M::X = 1"]] + with_setup(files) do + new_loader(setup: false) # should be skipped + Zeitwerk::Loader.eager_load_namespace(M) + assert required?(files) + end + end end diff --git a/test/lib/zeitwerk/test_logging.rb b/test/lib/zeitwerk/test_logging.rb index eaba67fd..d28c02bf 100644 --- a/test/lib/zeitwerk/test_logging.rb +++ b/test/lib/zeitwerk/test_logging.rb @@ -181,8 +181,7 @@ def logger.debug(message) end test "logs when eager loading starts" do - files = [["x.rb", "X = true"]] - with_files(files) do + with_setup([]) do assert_logged(/eager load start/) do loader.eager_load end @@ -190,8 +189,7 @@ def logger.debug(message) end test "logs when eager loading ends" do - files = [["x.rb", "X = true"]] - with_files(files) do + with_setup([]) do assert_logged(/eager load end/) do loader.eager_load end diff --git a/test/lib/zeitwerk/test_reloading.rb b/test/lib/zeitwerk/test_reloading.rb index 8fc742d1..17ff8498 100644 --- a/test/lib/zeitwerk/test_reloading.rb +++ b/test/lib/zeitwerk/test_reloading.rb @@ -226,4 +226,10 @@ def silence_exceptions_in_threads assert X end end + + test "raises if called before setup" do + assert_raises(Zeitwerk::SetupRequired) do + loader.reload + end + end end diff --git a/test/lib/zeitwerk/test_unload.rb b/test/lib/zeitwerk/test_unload.rb index 24d48af6..d30366f8 100644 --- a/test/lib/zeitwerk/test_unload.rb +++ b/test/lib/zeitwerk/test_unload.rb @@ -179,4 +179,10 @@ def self.name assert !required?(files) end end + + test "raises if called before setup" do + assert_raises(Zeitwerk::SetupRequired) do + loader.unload + end + end end diff --git a/test/support/loader_test.rb b/test/support/loader_test.rb index 23a9183c..1fc8961e 100644 --- a/test/support/loader_test.rb +++ b/test/support/loader_test.rb @@ -26,7 +26,12 @@ def new_loader(dirs: [], namespace: Object, enable_reloading: true, setup: true) end def reset_constants - Zeitwerk::Registry.loaders.each(&:unload) + Zeitwerk::Registry.loaders.each do |loader| + begin + loader.unload + rescue Zeitwerk::SetupRequired + end + end end def reset_registry