Merge pull request rails#52722 from Shopify/class-attribute-redefine-once-take-2

class_attribute: reduce reliance on define_method

Author: byroot

activerecord/test/cases/test_case.rb

@@ -141,9 +141,6 @@ def with_has_many_inversing(model = ActiveRecord::Base)
       yield
     ensure
       model.has_many_inversing = old
-      if model != ActiveRecord::Base && !old
-        model.singleton_class.remove_method(:has_many_inversing) # reset the class_attribute
-      end
     end
 
     def with_automatic_scope_inversing(*reflections)

activesupport/lib/active_support.rb

@@ -62,6 +62,7 @@ module ActiveSupport
     autoload :Cache
     autoload :Callbacks
     autoload :Configurable
+    autoload :ClassAttribute
     autoload :Deprecation
     autoload :Delegation
     autoload :Digest

activesupport/lib/active_support/callbacks.rb

@@ -66,7 +66,7 @@ module Callbacks
 
     included do
       extend ActiveSupport::DescendantsTracker
-      class_attribute :__callbacks, instance_writer: false, default: {}
+      class_attribute :__callbacks, instance_writer: false, instance_predicate: false, default: {}
     end
 
     CALLBACK_FILTER_TYPES = [:before, :after, :around].freeze

activesupport/lib/active_support/class_attribute.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module ActiveSupport
+  module ClassAttribute # :nodoc:
+    class << self
+      def redefine(owner, name, value)
+        if owner.singleton_class?
+          owner.redefine_method(name) { value }
+          owner.send(:public, name)
+        end
+
+        owner.redefine_singleton_method(name) { value }
+        owner.singleton_class.send(:public, name)
+
+        owner.redefine_singleton_method("#{name}=") do |new_value|
+          if owner.equal?(self)
+            value = new_value
+          else
+            ::ActiveSupport::ClassAttribute.redefine(self, name, new_value)
+          end
+        end
+        owner.singleton_class.send(:public, "#{name}=")
+      end
+    end
+  end
+end

activesupport/lib/active_support/core_ext/class/attribute.rb

@@ -91,24 +91,16 @@ def class_attribute(*attrs, instance_accessor: true,
         raise TypeError, "#{name.inspect} is not a symbol nor a string"
       end
 
-      class_methods << <<~RUBY # In case the method exists and is not public
-        silence_redefinition_of_method def #{name}
-        end
-      RUBY
-
-      methods << <<~RUBY if instance_reader
-        silence_redefinition_of_method def #{name}
-          defined?(@#{name}) ? @#{name} : self.class.#{name}
-        end
-      RUBY
+      name = name.to_sym
+      ::ActiveSupport::ClassAttribute.redefine(self, name, default)
 
-      class_methods << <<~RUBY
-        silence_redefinition_of_method def #{name}=(value)
-          redefine_method(:#{name}) { value } if singleton_class?
-          redefine_singleton_method(:#{name}) { value }
-          value
-        end
-      RUBY
+      unless singleton_class?
+        methods << <<~RUBY if instance_reader
+          silence_redefinition_of_method def #{name}
+            defined?(@#{name}) ? @#{name} : self.class.#{name}
+          end
+        RUBY
+      end
 
       methods << <<~RUBY if instance_writer
         silence_redefinition_of_method(:#{name}=)
@@ -125,7 +117,5 @@ def class_attribute(*attrs, instance_accessor: true,
 
     location = caller_locations(1, 1).first
     class_eval(["class << self", *class_methods, "end", *methods].join(";").tr("\n", ";"), location.path, location.lineno)
-
-    attrs.each { |name| public_send("#{name}=", default) }
   end
 end

activesupport/test/core_ext/class/attribute_test.rb

@@ -8,6 +8,7 @@ def setup
     @klass = Class.new do
       class_attribute :setting
       class_attribute :timeout, default: 5
+      class_attribute :system # private kernel method
     end
 
     @sub = Class.new(@klass)
@@ -30,11 +31,14 @@ def setup
   test "overridable" do
     @sub.setting = 1
     assert_nil @klass.setting
+    assert_equal 1, @sub.setting
 
     @klass.setting = 2
+    assert_equal 2, @klass.setting
     assert_equal 1, @sub.setting
 
     assert_equal 1, Class.new(@sub).setting
+    assert_equal 2, Class.new(@klass).setting
   end
 
   test "predicate method" do
@@ -97,8 +101,34 @@ def setup
 
   test "works well with singleton classes" do
     object = @klass.new
+
     object.singleton_class.setting = "foo"
     assert_equal "foo", object.setting
+    assert_nil @klass.setting
+
+    object.singleton_class.setting = "bar"
+    assert_equal "bar", object.setting
+    assert_nil @klass.setting
+
+    @klass.setting = "plop"
+    assert_equal "bar", object.setting
+    assert_equal "plop", @klass.setting
+  end
+
+  test "when defined in a class's singleton" do
+    @klass = Class.new do
+      class << self
+        class_attribute :__callbacks, default: 1
+      end
+    end
+
+    assert_equal 1, @klass.__callbacks
+    assert_equal 1, @klass.singleton_class.__callbacks
+
+    # I honestly think this is a bug, but that's how it used to behave
+    @klass.__callbacks = 4
+    assert_equal 1, @klass.__callbacks
+    assert_equal 1, @klass.singleton_class.__callbacks
   end
 
   test "works well with module singleton classes" do
@@ -115,4 +145,16 @@ class << self
     val = @klass.public_send(:setting=, 1)
     assert_equal 1, val
   end
+
+  test "works when overriding private methods from an ancestor" do
+    assert_nil @klass.system
+    @klass.system = 1
+    assert_equal 1, @klass.system
+
+    instance = @klass.new
+    assert_equal 1, instance.system
+    assert_predicate @klass.new, :system?
+    instance.system = 2
+    assert_equal 2, instance.system
+  end
 end