Fix a bug where requires inside of autoloads were being added to the autoloaded_constants list, causing mayhem. [#5165 state:resolved]

5 files changed, 73 insertions, 5 deletions

diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb
index 9a6da38b1c..2b80bd214f 100644
--- a/activesupport/lib/active_support/dependencies.rb
+++ b/activesupport/lib/active_support/dependencies.rb
@@ -72,10 +72,6 @@ module ActiveSupport #:nodoc:
         methods.each { |m| class_eval "def #{m}(*) lock { super } end", __FILE__, __LINE__ }
       end
 
-      def get(key)
-        (val = assoc(key)) ? val[1] : []
-      end
-
       locked :concat, :each, :delete_if, :<<
 
       def new_constants_for(frames)
@@ -85,7 +81,18 @@ module ActiveSupport #:nodoc:
           next unless mod.is_a?(Module)
 
           new_constants = mod.local_constant_names - prior_constants
-          get(mod_name).concat(new_constants)
+
+          # If we are checking for constants under, say, :Object, nested under something
+          # else that is checking for constants also under :Object, make sure the
+          # parent knows that we have found, and taken care of, the constant.
+          #
+          # In particular, this means that since Kernel.require discards the constants
+          # it finds, parents will be notified that about those constants, and not
+          # consider them "new". As a result, they will not be added to the
+          # autoloaded_constants list.
+          each do |key, value|
+            value.concat(new_constants) if key == mod_name
+          end
 
           new_constants.each do |suffix|
             constants << ([mod_name, suffix] - ["Object"]).join("::")
diff --git a/activesupport/test/autoloading_fixtures/load_path/loaded_constant.rb b/activesupport/test/autoloading_fixtures/load_path/loaded_constant.rb
new file mode 100644
index 0000000000..e3d1218c96
--- /dev/null
+++ b/activesupport/test/autoloading_fixtures/load_path/loaded_constant.rb
@@ -0,0 +1,3 @@
+module LoadedConstant
+end
+
diff --git a/activesupport/test/autoloading_fixtures/loads_constant.rb b/activesupport/test/autoloading_fixtures/loads_constant.rb
new file mode 100644
index 0000000000..b5b80c46da
--- /dev/null
+++ b/activesupport/test/autoloading_fixtures/loads_constant.rb
@@ -0,0 +1,4 @@
+module LoadsConstant
+end
+
+RequiresConstant
diff --git a/activesupport/test/autoloading_fixtures/requires_constant.rb b/activesupport/test/autoloading_fixtures/requires_constant.rb
new file mode 100644
index 0000000000..14804a0de0
--- /dev/null
+++ b/activesupport/test/autoloading_fixtures/requires_constant.rb
@@ -0,0 +1,5 @@
+require "loaded_constant"
+
+module RequiresConstant
+end
+
diff --git a/activesupport/test/dependencies_test.rb b/activesupport/test/dependencies_test.rb
index d7bde185bd..ec5116bff4 100644
--- a/activesupport/test/dependencies_test.rb
+++ b/activesupport/test/dependencies_test.rb
@@ -213,6 +213,48 @@ class DependenciesTest < Test::Unit::TestCase
     end
   end
 
+  def test_doesnt_break_normal_require
+    path = File.expand_path("../autoloading_fixtures/load_path", __FILE__)
+    original_path = $:.dup
+    original_features = $".dup
+    $:.push(path)
+
+    with_autoloading_fixtures do
+      RequiresConstant
+      assert defined?(RequiresConstant)
+      assert defined?(LoadedConstant)
+      ActiveSupport::Dependencies.clear
+      RequiresConstant
+      assert defined?(RequiresConstant)
+      assert defined?(LoadedConstant)
+    end
+  ensure
+    remove_constants(:RequiresConstant, :LoadedConstant, :LoadsConstant)
+    $".replace(original_features)
+    $:.replace(original_path)
+  end
+
+  def test_doesnt_break_normal_require_nested
+    path = File.expand_path("../autoloading_fixtures/load_path", __FILE__)
+    original_path = $:.dup
+    original_features = $".dup
+    $:.push(path)
+
+    with_autoloading_fixtures do
+      LoadsConstant
+      assert defined?(LoadsConstant)
+      assert defined?(LoadedConstant)
+      ActiveSupport::Dependencies.clear
+      LoadsConstant
+      assert defined?(LoadsConstant)
+      assert defined?(LoadedConstant)
+    end
+  ensure
+    remove_constants(:RequiresConstant, :LoadedConstant, :LoadsConstant)
+    $".replace(original_features)
+    $:.replace(original_path)
+  end
+
   def failing_test_access_thru_and_upwards_fails
     with_autoloading_fixtures do
       assert ! defined?(ModuleFolder)
@@ -797,4 +839,11 @@ class DependenciesTest < Test::Unit::TestCase
   ensure
     ActiveSupport::Dependencies.hook!
   end
+
+private
+  def remove_constants(*constants)
+    constants.each do |constant|
+      Object.send(:remove_const, constant) if Object.const_defined?(constant)
+    end
+  end
 end