Skip to content

Commit

Permalink
fixes circularity check in dependencies
Browse files Browse the repository at this point in the history
The check for circular loading should depend on a stack of files being
loaded at the moment, rather than the collection of loaded files.

This showed up indirectly in #16468, where a misspelled helper would
incorrectly result in a circularity error message.

References #16468
  • Loading branch information
fxn committed Oct 25, 2014
1 parent c4767e1 commit ae07806
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 1 deletion.
10 changes: 9 additions & 1 deletion activesupport/lib/active_support/dependencies.rb
Expand Up @@ -30,6 +30,10 @@ module Dependencies #:nodoc:
mattr_accessor :loaded
self.loaded = Set.new

# Stack of files being loaded.
mattr_accessor :loading
self.loading = []

# Should we load files or require them?
mattr_accessor :mechanism
self.mechanism = ENV['NO_RELOAD'] ? :require : :load
Expand Down Expand Up @@ -317,6 +321,7 @@ def depend_on(file_name, message = "No such file to load -- %s.rb")
def clear
log_call
loaded.clear
loading.clear
remove_unloadable_constants!
end

Expand All @@ -329,6 +334,7 @@ def require_or_load(file_name, const_path = nil)
# Record that we've seen this file *before* loading it to avoid an
# infinite loop with mutual dependencies.
loaded << expanded
loading << expanded

begin
if load?
Expand All @@ -351,6 +357,8 @@ def require_or_load(file_name, const_path = nil)
rescue Exception
loaded.delete expanded
raise
ensure
loading.pop
end

# Record history *after* loading so first load gets warnings.
Expand Down Expand Up @@ -475,7 +483,7 @@ def load_missing_constant(from_mod, const_name)
expanded = File.expand_path(file_path)
expanded.sub!(/\.rb\z/, '')

if loaded.include?(expanded)
if loading.include?(expanded)
raise "Circular dependency detected while autoloading constant #{qualified_name}"
else
require_or_load(expanded, qualified_name)
Expand Down
2 changes: 2 additions & 0 deletions activesupport/test/autoloading_fixtures/typo.rb
@@ -0,0 +1,2 @@
TypO = 1

25 changes: 25 additions & 0 deletions activesupport/test/dependencies_test.rb
Expand Up @@ -157,6 +157,31 @@ def test_circular_autoloading_detection
end
end

def test_ensures_the_expected_constant_is_defined
with_autoloading_fixtures do
e = assert_raise(LoadError) { Typo }
assert_match %r{Unable to autoload constant Typo, expected .*activesupport/test/autoloading_fixtures/typo.rb to define it}, e.message
end
end

def test_require_dependency_does_not_assume_any_particular_constant_is_defined
with_autoloading_fixtures do
require_dependency 'typo'
assert_equal 1, TypO
end
end

# Regression, see https://github.com/rails/rails/issues/16468.
def test_require_dependency_interaction_with_autoloading
with_autoloading_fixtures do
require_dependency 'typo'
assert_equal 1, TypO

e = assert_raise(LoadError) { Typo }
assert_match %r{Unable to autoload constant Typo, expected .*activesupport/test/autoloading_fixtures/typo.rb to define it}, e.message
end
end

def test_module_loading
with_autoloading_fixtures do
assert_kind_of Module, A
Expand Down
1 change: 1 addition & 0 deletions activesupport/test/dependencies_test_helpers.rb
Expand Up @@ -13,6 +13,7 @@ def with_loading(*from)
ActiveSupport::Dependencies.autoload_paths = prior_autoload_paths
ActiveSupport::Dependencies.mechanism = old_mechanism
ActiveSupport::Dependencies.explicitly_unloadable_constants = []
ActiveSupport::Dependencies.clear
end

def with_autoloading_fixtures(&block)
Expand Down

0 comments on commit ae07806

Please sign in to comment.