Merge pull request #43071 from rails/descendants-tracker

Remove autoloading logic from AS::DescendantsTracker

activesupport/lib/active_support/dependencies.rb

@@ -57,6 +57,11 @@ def eager_load?(path)
     # accessors of each engine.
     mattr_accessor :_eager_load_paths, default: Set.new
 
+    # If reloading is enabled, this private set holds autoloaded classes tracked
+    # by the descendants tracker. It is populated by an on_load callback in the
+    # main autoloader. Used to clear state.
+    mattr_accessor :_autoloaded_tracked_classes, default: Set.new
+
     # An array of qualified constant names that have been loaded. Adding a name
     # to this array will cause it to be unloaded the next time Dependencies are
     # cleared.

activesupport/lib/active_support/dependencies/zeitwerk_integration.rb

@@ -9,6 +9,7 @@ module ZeitwerkIntegration # :nodoc: all
       module Decorations
         def clear
           Dependencies.unload_interlock do
+            _autoloaded_tracked_classes.clear
             Rails.autoloaders.main.reload
           rescue Zeitwerk::ReloadingDisabledError
             raise "reloading is disabled because config.cache_classes is true"

activesupport/lib/active_support/descendants_tracker.rb

@@ -21,17 +21,20 @@ def descendants(klass)
         arr
       end
 
-      def clear
+      def clear(only: nil)
-        if defined? ActiveSupport::Dependencies
+        if only.nil?
-          @@direct_descendants.each do |klass, descendants|
+          @@direct_descendants.clear
-            if Dependencies.autoloaded?(klass)
+          return
-              @@direct_descendants.delete(klass)
+        end
-            else
+
-              descendants.reject! { |v| Dependencies.autoloaded?(v) }
+        @@direct_descendants.each do |klass, direct_descendants_of_klass|
+          if only.member?(klass)
+            @@direct_descendants.delete(klass)
+          else
+            direct_descendants_of_klass.reject! do |direct_descendant_of_class|
+              only.member?(direct_descendant_of_class)
             end
           end
-        else
-          @@direct_descendants.clear
         end
       end
 

activesupport/test/descendants_tracker_test.rb

@@ -1,33 +1,49 @@
 # frozen_string_literal: true
 
+require_relative "abstract_unit"
 require "set"
+require "active_support/descendants_tracker"
 
-module DescendantsTrackerTestCases
+class DescendantsTrackerTest < ActiveSupport::TestCase
-  class Parent
+  setup do
-    extend ActiveSupport::DescendantsTracker
+    @original_state = ActiveSupport::DescendantsTracker.class_eval("@@direct_descendants").dup
+    @original_state.each { |k, v| @original_state[k] = v.dup }
+
+    ActiveSupport::DescendantsTracker.clear
+    eval <<~RUBY
+      class Parent
+        extend ActiveSupport::DescendantsTracker
+      end
+
+      class Child1 < Parent
+      end
+
+      class Child2 < Parent
+      end
+
+      class Grandchild1 < Child1
+      end
+
+      class Grandchild2 < Child1
+      end
+    RUBY
   end
 
-  class Child1 < Parent
+  teardown do
+    ActiveSupport::DescendantsTracker.class_eval("@@direct_descendants").replace(@original_state)
+
+    %i(Parent Child1 Child2 Grandchild1 Grandchild2).each do |name|
+      DescendantsTrackerTest.send(:remove_const, name)
+    end
   end
 
-  class Child2 < Parent
+  test ".descendants" do
-  end
-
-  class Grandchild1 < Child1
-  end
-
-  class Grandchild2 < Child1
-  end
-
-  ALL = [Parent, Child1, Child2, Grandchild1, Grandchild2]
-
-  def test_descendants
     assert_equal_sets [Child1, Grandchild1, Grandchild2, Child2], Parent.descendants
     assert_equal_sets [Grandchild1, Grandchild2], Child1.descendants
     assert_equal_sets [], Child2.descendants
   end
 
-  def test_descendants_with_garbage_collected_classes
+  test ".descendants with garbage collected classes" do
     # The Ruby GC (and most other GCs for that matter) are not fully precise.
     # When GC is run, the whole stack is scanned to mark any object reference
     # in registers. But some of these references might simply be leftovers from
@@ -48,46 +64,35 @@ def test_descendants_with_garbage_collected_classes
     assert_equal_sets [Child1, Grandchild1, Grandchild2, Child2], Parent.descendants
   end
 
-  def test_direct_descendants
+  test ".direct_descendants" do
     assert_equal_sets [Child1, Child2], Parent.direct_descendants
     assert_equal_sets [Grandchild1, Grandchild2], Child1.direct_descendants
     assert_equal_sets [], Child2.direct_descendants
   end
 
-  def test_subclasses
+  test ".subclasses" do
     [Parent, Child1, Child2].each do |klass|
       assert_equal klass.direct_descendants, klass.subclasses
     end
   end
 
-  def test_clear
+  test ".clear deletes all state" do
-    mark_as_autoloaded(*ALL) do
+    ActiveSupport::DescendantsTracker.clear
-      ActiveSupport::DescendantsTracker.clear
+    assert_empty ActiveSupport::DescendantsTracker.class_eval("@@direct_descendants")
-      ALL.each do |k|
+  end
-        assert_empty ActiveSupport::DescendantsTracker.descendants(k)
+
-      end
+  test ".clear(only) deletes the given classes only" do
-    end
+    ActiveSupport::DescendantsTracker.clear(only: Set[Child2, Grandchild1])
+
+    assert_equal_sets [Child1, Grandchild2], Parent.descendants
+    assert_equal_sets [Grandchild2], Child1.descendants
+
+    assert_equal_sets [Child1], Parent.direct_descendants
+    assert_equal_sets [Grandchild2], Child1.direct_descendants
   end
 
   private
     def assert_equal_sets(expected, actual)
       assert_equal Set.new(expected), Set.new(actual)
     end
-
-    def mark_as_autoloaded(*klasses)
-      # If ActiveSupport::Dependencies is not loaded, forget about autoloading.
-      # This allows using AS::DescendantsTracker without AS::Dependencies.
-      if defined? ActiveSupport::Dependencies
-        old_autoloaded = ActiveSupport::Dependencies.autoloaded_constants.dup
-        ActiveSupport::Dependencies.autoloaded_constants = klasses.map(&:name)
-      end
-
-      old_descendants = ActiveSupport::DescendantsTracker.class_eval("@@direct_descendants").dup
-      old_descendants.each { |k, v| old_descendants[k] = v.dup }
-
-      yield
-    ensure
-      ActiveSupport::Dependencies.autoloaded_constants = old_autoloaded if defined? ActiveSupport::Dependencies
-      ActiveSupport::DescendantsTracker.class_eval("@@direct_descendants").replace(old_descendants)
-    end
 end

activesupport/test/descendants_tracker_with_autoloading_test.rb

@@ -1,36 +0,0 @@
-# frozen_string_literal: true
-
-require_relative "abstract_unit"
-require "active_support/descendants_tracker"
-require "active_support/dependencies"
-require_relative "descendants_tracker_test_cases"
-
-class DescendantsTrackerWithAutoloadingTest < ActiveSupport::TestCase
-  include DescendantsTrackerTestCases
-
-  def test_clear_with_autoloaded_parent_children_and_grandchildren
-    mark_as_autoloaded(*ALL) do
-      ActiveSupport::DescendantsTracker.clear
-      ALL.each do |k|
-        assert_empty ActiveSupport::DescendantsTracker.descendants(k)
-      end
-    end
-  end
-
-  def test_clear_with_autoloaded_children_and_grandchildren
-    mark_as_autoloaded Child1, Grandchild1, Grandchild2 do
-      ActiveSupport::DescendantsTracker.clear
-      assert_equal_sets [Child2], Parent.descendants
-      assert_equal_sets [], Child2.descendants
-    end
-  end
-
-  def test_clear_with_autoloaded_grandchildren
-    mark_as_autoloaded Grandchild1, Grandchild2 do
-      ActiveSupport::DescendantsTracker.clear
-      assert_equal_sets [Child1, Child2], Parent.descendants
-      assert_equal_sets [], Child1.descendants
-      assert_equal_sets [], Child2.descendants
-    end
-  end
-end

activesupport/test/descendants_tracker_without_autoloading_test.rb

@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-require_relative "abstract_unit"
-require "active_support/descendants_tracker"
-require_relative "descendants_tracker_test_cases"
-
-class DescendantsTrackerWithoutAutoloadingTest < ActiveSupport::TestCase
-  include DescendantsTrackerTestCases
-
-  # Regression test for #8422. https://github.com/rails/rails/issues/8442
-  def test_clear_without_autoloaded_singleton_parent
-    mark_as_autoloaded do
-      parent_instance = Parent.new
-      parent_instance.singleton_class.descendants
-      ActiveSupport::DescendantsTracker.clear
-      assert_not ActiveSupport::DescendantsTracker.class_variable_get(:@@direct_descendants).key?(parent_instance.singleton_class)
-    end
-  end
-end

railties/lib/rails/application/finisher.rb

@@ -2,6 +2,8 @@
 
 require "active_support/core_ext/string/inflections"
 require "active_support/core_ext/array/conversions"
+require "active_support/descendants_tracker"
+require "active_support/dependencies"
 require "zeitwerk"
 
 module Rails
@@ -27,6 +29,13 @@ module Finisher
 
         unless config.cache_classes
           autoloader.enable_reloading
+
+          autoloader.on_load do |_cpath, value, _abspath|
+            if value.is_a?(Class) && value.singleton_class < ActiveSupport::DescendantsTracker
+              ActiveSupport::Dependencies._autoloaded_tracked_classes << value
+            end
+          end
+
           autoloader.on_unload do |_cpath, value, _abspath|
             value.before_remove_const if value.respond_to?(:before_remove_const)
           end
@@ -170,7 +179,9 @@ def self.complete(_state)
       # added in the hook are taken into account.
       initializer :set_clear_dependencies_hook, group: :all do |app|
         callback = lambda do
-          ActiveSupport::DescendantsTracker.clear
+          ActiveSupport::DescendantsTracker.clear(
+            only: ActiveSupport::Dependencies._autoloaded_tracked_classes
+          )
           ActiveSupport::Dependencies.clear
         end
 

railties/test/application/zeitwerk_integration_test.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "set"
 require "isolation/abstract_unit"
 require "active_support/dependencies/zeitwerk_integration"
 
@@ -355,6 +356,36 @@ def Foo.before_remove_const
     assert $before_remove_const_invoked
   end
 
+  test "reloading clears autoloaded tracked classes" do
+    eval <<~RUBY
+      class Parent
+        extend ActiveSupport::DescendantsTracker
+      end
+    RUBY
+
+    app_file "app/models/child.rb", <<~RUBY
+      class Child < #{self.class.name}::Parent
+      end
+    RUBY
+
+    app_file "app/models/grandchild.rb", <<~RUBY
+      class Grandchild < Child
+      end
+    RUBY
+
+    boot
+    assert Grandchild
+
+    # Preconditions, we add some redundancy about descendants tracking.
+    assert_equal Set[Child, Grandchild], ActiveSupport::Dependencies._autoloaded_tracked_classes
+    assert_equal [Child, Grandchild], Parent.descendants
+
+    Rails.application.reloader.reload!
+
+    assert_empty ActiveSupport::Dependencies._autoloaded_tracked_classes
+    assert_equal [], Parent.descendants
+  end
+
   test "autoloaders.logger=" do
     boot