Merge pull request #38144 from jhawthorn/mattr_location

Define mattr_* methods at caller's location

actionview/test/template/form_options_helper_test.rb

@@ -57,8 +57,8 @@ class FormOptionsHelperTest < ActionView::TestCase
     end
 
     def self.prepended(base)
+      base.mattr_accessor(:fake_zones)
       class << base
-        mattr_accessor(:fake_zones)
         prepend ClassMethods
       end
     end

activesupport/lib/active_support/core_ext/module/attribute_accessors.rb

@@ -48,28 +48,25 @@ class Module
   #   end
   #
   #   Person.new.hair_colors # => [:brown, :black, :blonde, :red]
-  def mattr_reader(*syms, instance_reader: true, instance_accessor: true, default: nil)
+  def mattr_reader(*syms, instance_reader: true, instance_accessor: true, default: nil, location: nil)
+    raise TypeError, "module attributes should be defined directly on class, not singleton" if singleton_class?
+    location ||= caller_locations(1, 1).first
+
+    definition = []
     syms.each do |sym|
       raise NameError.new("invalid attribute name: #{sym}") unless /\A[_A-Za-z]\w*\z/.match?(sym)
-      class_eval(<<-EOS, __FILE__, __LINE__ + 1)
-        @@#{sym} = nil unless defined? @@#{sym}
 
-        def self.#{sym}
-          @@#{sym}
-        end
-      EOS
+      definition << "def self.#{sym}; @@#{sym}; end"
 
       if instance_reader && instance_accessor
-        class_eval(<<-EOS, __FILE__, __LINE__ + 1)
-          def #{sym}
-            @@#{sym}
-          end
-        EOS
+        definition << "def #{sym}; @@#{sym}; end"
       end
 
       sym_default_value = (block_given? && default.nil?) ? yield : default
-      class_variable_set("@@#{sym}", sym_default_value) unless sym_default_value.nil?
+      class_variable_set("@@#{sym}", sym_default_value) unless sym_default_value.nil? && class_variable_defined?("@@#{sym}")
     end
+
+    module_eval(definition.join(";"), location.path, location.lineno)
   end
   alias :cattr_reader :mattr_reader
 
@@ -115,28 +112,24 @@ class Module
   #   end
   #
   #   Person.class_variable_get("@@hair_colors") # => [:brown, :black, :blonde, :red]
-  def mattr_writer(*syms, instance_writer: true, instance_accessor: true, default: nil)
+  def mattr_writer(*syms, instance_writer: true, instance_accessor: true, default: nil, location: nil)
+    raise TypeError, "module attributes should be defined directly on class, not singleton" if singleton_class?
+    location ||= caller_locations(1, 1).first
+
+    definition = []
     syms.each do |sym|
       raise NameError.new("invalid attribute name: #{sym}") unless /\A[_A-Za-z]\w*\z/.match?(sym)
-      class_eval(<<-EOS, __FILE__, __LINE__ + 1)
-        @@#{sym} = nil unless defined? @@#{sym}
-
-        def self.#{sym}=(obj)
-          @@#{sym} = obj
-        end
-      EOS
+      definition << "def self.#{sym}=(val); @@#{sym} = val; end"
 
       if instance_writer && instance_accessor
-        class_eval(<<-EOS, __FILE__, __LINE__ + 1)
-          def #{sym}=(obj)
-            @@#{sym} = obj
-          end
-        EOS
+        definition << "def #{sym}=(val); @@#{sym} = val; end"
       end
 
       sym_default_value = (block_given? && default.nil?) ? yield : default
-      send("#{sym}=", sym_default_value) unless sym_default_value.nil?
+      class_variable_set("@@#{sym}", sym_default_value) unless sym_default_value.nil? && class_variable_defined?("@@#{sym}")
     end
+
+    module_eval(definition.join(";"), location.path, location.lineno)
   end
   alias :cattr_writer :mattr_writer
 
@@ -205,8 +198,9 @@ class Module
   #
   #   Person.class_variable_get("@@hair_colors") # => [:brown, :black, :blonde, :red]
   def mattr_accessor(*syms, instance_reader: true, instance_writer: true, instance_accessor: true, default: nil, &blk)
-    mattr_reader(*syms, instance_reader: instance_reader, instance_accessor: instance_accessor, default: default, &blk)
-    mattr_writer(*syms, instance_writer: instance_writer, instance_accessor: instance_accessor, default: default)
+    location = caller_locations(1, 1).first
+    mattr_reader(*syms, instance_reader: instance_reader, instance_accessor: instance_accessor, default: default, location: location, &blk)
+    mattr_writer(*syms, instance_writer: instance_writer, instance_accessor: instance_accessor, default: default, location: location)
   end
   alias :cattr_accessor :mattr_accessor
 end

activesupport/test/core_ext/module/attribute_accessor_test.rb

@@ -134,4 +134,17 @@ class ModuleAttributeAccessorTest < ActiveSupport::TestCase
     assert_equal 1, @module.defn1
     assert_equal 2, @module.defn2
   end
+
+  def test_declaring_attributes_on_singleton_errors
+    klass = Class.new
+
+    ex = assert_raises TypeError do
+      class << klass
+        mattr_accessor :my_attr
+      end
+    end
+    assert_equal "module attributes should be defined directly on class, not singleton", ex.message
+
+    assert_not_includes Module.class_variables, :@@my_attr
+  end
 end