From 9efca53908c09b7f188183aec6c0a4a2df347316 Mon Sep 17 00:00:00 2001 From: Nicholas Seckar Date: Mon, 27 Mar 2006 05:13:46 +0000 Subject: Dependencies cleanup. Fixes #4221. git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4060 5ecf4fe2-1ee6-0310-87b1-e25e094e27de --- activesupport/CHANGELOG | 6 + .../lib/active_support/core_ext/module.rb | 4 +- .../core_ext/module/introspection.rb | 21 +++ .../lib/active_support/core_ext/module/loading.rb | 13 ++ activesupport/lib/active_support/dependencies.rb | 23 +-- .../module_folder/nested_class.rb | 2 + .../module_folder/nested_sibling.rb | 2 + activesupport/test/core_ext/module_test.rb | 16 ++ activesupport/test/dependencies_test.rb | 175 +++++++++++---------- 9 files changed, 162 insertions(+), 100 deletions(-) create mode 100644 activesupport/lib/active_support/core_ext/module/introspection.rb create mode 100644 activesupport/lib/active_support/core_ext/module/loading.rb create mode 100644 activesupport/test/autoloading_fixtures/module_folder/nested_class.rb create mode 100644 activesupport/test/autoloading_fixtures/module_folder/nested_sibling.rb diff --git a/activesupport/CHANGELOG b/activesupport/CHANGELOG index 87a8428d0f..b4eb8e0455 100644 --- a/activesupport/CHANGELOG +++ b/activesupport/CHANGELOG @@ -1,5 +1,11 @@ *SVN* +* When possible, avoid incorrectly obtaining constants from parent modules. Fixes #4221. [Nicholas Seckar] + +* Add more tests for dependencies; refactor existing cases. [Nicholas Seckar] + +* Move Module#parent and Module#as_load_path into core_ext. Add Module#parent. [Nicholas Seckar] + * Add CachingTools::HashCaching to simplify the creation of nested, autofilling hashes. [Nicholas Seckar] * Remove a hack intended to avoid unloading the same class twice, but which would not work anyways. [Nicholas Seckar] diff --git a/activesupport/lib/active_support/core_ext/module.rb b/activesupport/lib/active_support/core_ext/module.rb index bd6d62468a..e67cf940f6 100644 --- a/activesupport/lib/active_support/core_ext/module.rb +++ b/activesupport/lib/active_support/core_ext/module.rb @@ -1,3 +1,5 @@ require File.dirname(__FILE__) + '/module/inclusion' require File.dirname(__FILE__) + '/module/attribute_accessors' -require File.dirname(__FILE__) + '/module/delegation' \ No newline at end of file +require File.dirname(__FILE__) + '/module/delegation' +require File.dirname(__FILE__) + '/module/introspection' +require File.dirname(__FILE__) + '/module/loading' diff --git a/activesupport/lib/active_support/core_ext/module/introspection.rb b/activesupport/lib/active_support/core_ext/module/introspection.rb new file mode 100644 index 0000000000..0cd0d1ff2c --- /dev/null +++ b/activesupport/lib/active_support/core_ext/module/introspection.rb @@ -0,0 +1,21 @@ +class Module + # Return the module which contains this one; if this is a root module, such as + # +::MyModule+, then Object is returned. + def parent + parent_name = name.split('::')[0..-2] * '::' + parent_name.empty? ? Object : parent_name.constantize + end + + # Return all the parents of this module, ordered from nested outwards. The + # receiver is not contained within the result. + def parents + parents = [] + parts = name.split('::')[0..-2] + until parts.empty? + parents << (parts * '::').constantize + parts.pop + end + parents << Object unless parents.include? Object + parents + end +end diff --git a/activesupport/lib/active_support/core_ext/module/loading.rb b/activesupport/lib/active_support/core_ext/module/loading.rb new file mode 100644 index 0000000000..36c0c61405 --- /dev/null +++ b/activesupport/lib/active_support/core_ext/module/loading.rb @@ -0,0 +1,13 @@ +class Module + def as_load_path + if self == Object || self == Kernel + '' + elsif is_a? Class + parent == self ? '' : parent.as_load_path + else + name.split('::').collect do |word| + word.underscore + end * '/' + end + end +end \ No newline at end of file diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index b82c7e56f3..ffddda6f03 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -78,23 +78,6 @@ class Module #:nodoc: # Rename the original handler so we can chain it to the new one alias :rails_original_const_missing :const_missing - def parent - parent_name = name.split('::')[0..-2] * '::' - parent_name.empty? ? Object : parent_name.constantize - end - - def as_load_path - if self == Object || self == Kernel - '' - elsif is_a? Class - parent == self ? '' : parent.as_load_path - else - name.split('::').collect do |word| - word.underscore - end * '/' - end - end - # Use const_missing to autoload associations so we don't have to # require_association when using single-table inheritance. def const_missing(class_id) @@ -116,7 +99,11 @@ class Module #:nodoc: return mod end - if parent && parent != self + # Attempt to access the name from the parent, unless we don't have a valid + # parent, or the constant is already defined in the parent. If the latter + # is the case, then we are being queried via self::class_id, and we should + # avoid returning the constant from the parent if possible. + if parent && parent != self && ! parents.any? { |p| p.const_defined?(class_id) } suppress(NameError) do return parent.send(:const_missing, class_id) end diff --git a/activesupport/test/autoloading_fixtures/module_folder/nested_class.rb b/activesupport/test/autoloading_fixtures/module_folder/nested_class.rb new file mode 100644 index 0000000000..39ee0e50d5 --- /dev/null +++ b/activesupport/test/autoloading_fixtures/module_folder/nested_class.rb @@ -0,0 +1,2 @@ +class ModuleFolder::NestedClass +end \ No newline at end of file diff --git a/activesupport/test/autoloading_fixtures/module_folder/nested_sibling.rb b/activesupport/test/autoloading_fixtures/module_folder/nested_sibling.rb new file mode 100644 index 0000000000..80244b8bab --- /dev/null +++ b/activesupport/test/autoloading_fixtures/module_folder/nested_sibling.rb @@ -0,0 +1,2 @@ +class ModuleFolder::NestedSibling +end \ No newline at end of file diff --git a/activesupport/test/core_ext/module_test.rb b/activesupport/test/core_ext/module_test.rb index 464196a0ec..eb92fbc12c 100644 --- a/activesupport/test/core_ext/module_test.rb +++ b/activesupport/test/core_ext/module_test.rb @@ -82,4 +82,20 @@ class ModuleTest < Test::Unit::TestCase assert_raises(ArgumentError) { eval($nowhere) } assert_raises(ArgumentError) { eval($noplace) } end + + def test_parent + assert_equal Yz::Zy, Yz::Zy::Cd.parent + assert_equal Yz, Yz::Zy.parent + assert_equal Object, Yz.parent + end + + def test_parents + assert_equal [Yz::Zy, Yz, Object], Yz::Zy::Cd.parents + assert_equal [Yz, Object], Yz::Zy.parents + end + + def test_as_load_path + assert_equal 'yz/zy', Yz::Zy.as_load_path + assert_equal 'yz', Yz.as_load_path + end end \ No newline at end of file diff --git a/activesupport/test/dependencies_test.rb b/activesupport/test/dependencies_test.rb index af7c09068c..f5651eddcc 100644 --- a/activesupport/test/dependencies_test.rb +++ b/activesupport/test/dependencies_test.rb @@ -7,6 +7,17 @@ class DependenciesTest < Test::Unit::TestCase def teardown Dependencies.clear end + + def with_loading(from_dir = nil) + prior_path = $LOAD_PATH.clone + $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/#{from_dir}" if from_dir + old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load + yield + ensure + $LOAD_PATH.clear + $LOAD_PATH.concat prior_path + Dependencies.mechanism = old_mechanism + end def test_tracking_loaded_files require_dependency(File.dirname(__FILE__) + "/dependencies/service_one") @@ -29,76 +40,68 @@ class DependenciesTest < Test::Unit::TestCase end def test_dependency_which_raises_exception_isnt_added_to_loaded_set - old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load + with_loading do + filename = "#{File.dirname(__FILE__)}/dependencies/raises_exception" + $raises_exception_load_count = 0 - filename = "#{File.dirname(__FILE__)}/dependencies/raises_exception" - $raises_exception_load_count = 0 + 5.times do |count| + assert_raises(RuntimeError) { require_dependency filename } + assert_equal count + 1, $raises_exception_load_count - 5.times do |count| - assert_raises(RuntimeError) { require_dependency filename } - assert_equal count + 1, $raises_exception_load_count - - assert !Dependencies.loaded.include?(filename) - assert !Dependencies.history.include?(filename) + assert !Dependencies.loaded.include?(filename) + assert !Dependencies.history.include?(filename) + end end - ensure - Dependencies.mechanism = old_mechanism end def test_warnings_should_be_enabled_on_first_load - old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load - old_warnings, Dependencies.warnings_on_first_load = Dependencies.warnings_on_first_load, true + with_loading do + old_warnings, Dependencies.warnings_on_first_load = Dependencies.warnings_on_first_load, true - filename = "#{File.dirname(__FILE__)}/dependencies/check_warnings" - $check_warnings_load_count = 0 + filename = "#{File.dirname(__FILE__)}/dependencies/check_warnings" + $check_warnings_load_count = 0 - assert !Dependencies.loaded.include?(filename) - assert !Dependencies.history.include?(filename) + assert !Dependencies.loaded.include?(filename) + assert !Dependencies.history.include?(filename) - silence_warnings { require_dependency filename } - assert_equal 1, $check_warnings_load_count - assert_equal true, $checked_verbose, 'On first load warnings should be enabled.' + silence_warnings { require_dependency filename } + assert_equal 1, $check_warnings_load_count + assert_equal true, $checked_verbose, 'On first load warnings should be enabled.' - assert Dependencies.loaded.include?(filename) - Dependencies.clear - assert !Dependencies.loaded.include?(filename) - assert Dependencies.history.include?(filename) + assert Dependencies.loaded.include?(filename) + Dependencies.clear + assert !Dependencies.loaded.include?(filename) + assert Dependencies.history.include?(filename) - silence_warnings { require_dependency filename } - assert_equal 2, $check_warnings_load_count - assert_equal nil, $checked_verbose, 'After first load warnings should be left alone.' + silence_warnings { require_dependency filename } + assert_equal 2, $check_warnings_load_count + assert_equal nil, $checked_verbose, 'After first load warnings should be left alone.' - assert Dependencies.loaded.include?(filename) - Dependencies.clear - assert !Dependencies.loaded.include?(filename) - assert Dependencies.history.include?(filename) + assert Dependencies.loaded.include?(filename) + Dependencies.clear + assert !Dependencies.loaded.include?(filename) + assert Dependencies.history.include?(filename) - enable_warnings { require_dependency filename } - assert_equal 3, $check_warnings_load_count - assert_equal true, $checked_verbose, 'After first load warnings should be left alone.' + enable_warnings { require_dependency filename } + assert_equal 3, $check_warnings_load_count + assert_equal true, $checked_verbose, 'After first load warnings should be left alone.' - assert Dependencies.loaded.include?(filename) - ensure - Dependencies.mechanism = old_mechanism - Dependencies.warnings_on_first_load = old_warnings + assert Dependencies.loaded.include?(filename) + end end def test_mutual_dependencies_dont_infinite_loop - $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/dependencies" - old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load + with_loading 'dependencies' do + $mutual_dependencies_count = 0 + assert_nothing_raised { require_dependency 'mutual_one' } + assert_equal 2, $mutual_dependencies_count - $mutual_dependencies_count = 0 - assert_nothing_raised { require_dependency 'mutual_one' } - assert_equal 2, $mutual_dependencies_count + Dependencies.clear - Dependencies.clear - - $mutual_dependencies_count = 0 - assert_nothing_raised { require_dependency 'mutual_two' } - assert_equal 2, $mutual_dependencies_count - ensure - $LOAD_PATH.shift - Dependencies.mechanism = old_mechanism + $mutual_dependencies_count = 0 + assert_nothing_raised { require_dependency 'mutual_two' } + assert_equal 2, $mutual_dependencies_count + end end def test_as_load_path @@ -106,43 +109,53 @@ class DependenciesTest < Test::Unit::TestCase end def test_module_loading - begin - $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/autoloading_fixtures" - old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load - + with_loading 'autoloading_fixtures' do assert_kind_of Module, A assert_kind_of Class, A::B assert_kind_of Class, A::C::D assert_kind_of Class, A::C::E::F - ensure - $LOAD_PATH.shift - Dependencies.mechanism = old_mechanism end end - def test_non_existing_cost_raises_nameerrror - begin - $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/autoloading_fixtures" - old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load - assert_raises(NameError) do - DoesNotExist - end - - assert_raises(NameError) do - NoModule::DoesNotExist - end - - assert_raises(NameError) do - A::DoesNotExist - end - - assert_raises(NameError) do - A::B::DoesNotExist - end - ensure - $LOAD_PATH.shift - Dependencies.mechanism = old_mechanism + def test_non_existing_const_raises_name_error + with_loading 'autoloading_fixtures' do + assert_raises(NameError) { DoesNotExist } + assert_raises(NameError) { NoModule::DoesNotExist } + assert_raises(NameError) { A::DoesNotExist } + assert_raises(NameError) { A::B::DoesNotExist } end - end + + def test_directories_should_manifest_as_modules + with_loading 'autoloading_fixtures' do + assert_kind_of Module, ModuleFolder + Object.send :remove_const, :ModuleFolder + end + end + + def test_nested_class_access + with_loading 'autoloading_fixtures' do + assert_kind_of Class, ModuleFolder::NestedClass + Object.send :remove_const, :ModuleFolder + end + end + + def test_nested_class_can_access_sibling + with_loading 'autoloading_fixtures' do + sibling = ModuleFolder::NestedClass.class_eval "NestedSibling" + assert defined?(ModuleFolder::NestedSibling) + assert_equal ModuleFolder::NestedSibling, sibling + Object.send :remove_const, :ModuleFolder + end + end + + def failing_test_access_thru_and_upwards_fails + with_loading 'autoloading_fixtures' do + assert ! defined?(ModuleFolder) + assert_raises(NameError) { ModuleFolder::Object } + assert_raises(NameError) { ModuleFolder::NestedClass::Object } + Object.send :remove_const, :ModuleFolder + end + end + end -- cgit v1.2.3