Dependencies cleanup. Fixes #4221.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4060 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

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]

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'
+require File.dirname(__FILE__) + '/module/delegation'
+require File.dirname(__FILE__) + '/module/introspection'
+require File.dirname(__FILE__) + '/module/loading'

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

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

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

activesupport/test/autoloading_fixtures/module_folder/nested_class.rb

@@ -0,0 +1,2 @@
+class ModuleFolder::NestedClass
+end

activesupport/test/autoloading_fixtures/module_folder/nested_sibling.rb

@@ -0,0 +1,2 @@
+class ModuleFolder::NestedSibling
+end

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

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