Skip to content

Commit

Permalink
Raise Zeitwerk::SetupRequired if trying to load too early
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
fxn committed Nov 5, 2022
1 parent 53558e1 commit 866872a
Show file tree
Hide file tree
Showing 10 changed files with 101 additions and 18 deletions.
6 changes: 6 additions & 0 deletions lib/zeitwerk/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
23 changes: 19 additions & 4 deletions lib/zeitwerk/loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -216,6 +218,7 @@ def unload
# @sig () -> void
def reload
raise ReloadingDisabledError unless reloading_enabled?
raise Zeitwerk::SetupRequired unless @setup

unload
recompute_ignored_paths
Expand Down Expand Up @@ -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

Expand Down
5 changes: 5 additions & 0 deletions lib/zeitwerk/loader/eager_load.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions test/lib/zeitwerk/test_eager_load.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
12 changes: 10 additions & 2 deletions test/lib/zeitwerk/test_eager_load_dir.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
33 changes: 26 additions & 7 deletions test/lib/zeitwerk/test_eager_load_namespace.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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"],
Expand All @@ -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
6 changes: 2 additions & 4 deletions test/lib/zeitwerk/test_logging.rb
Original file line number Diff line number Diff line change
Expand Up @@ -181,17 +181,15 @@ 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
end
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
Expand Down
6 changes: 6 additions & 0 deletions test/lib/zeitwerk/test_reloading.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 6 additions & 0 deletions test/lib/zeitwerk/test_unload.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
7 changes: 6 additions & 1 deletion test/support/loader_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

2 comments on commit 866872a

@mgomes
Copy link

@mgomes mgomes commented on 866872a Nov 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is causing us some issues. Just curious, why not release a 2.7.x for this change?

I totally think it's something we've done incorrectly on our end, btw. hellostealth/stealth#346

@fxn
Copy link
Owner Author

@fxn fxn commented on 866872a Nov 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. This was thought as a backwards compatible improvement because if your application is loading correctly (lazy, eager, or reloading), then you are calling setup on time already.

It is very unlikely that you do not call setup and don't get an exception, and the application works at all. But in that edge case, you uncovered a bug.

Please sign in to comment.