From cd0fbaea01e684806a8f8b36ffbaedfc42873b6a Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Sat, 4 Jan 2020 17:47:10 -0800 Subject: [PATCH 1/3] Define fake_zones on AS::TimeZone, not singleton Previously this was incorrectly defining this attribute on the singleton class, which would end up actually using the class variable from Module. --- actionview/test/template/form_options_helper_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionview/test/template/form_options_helper_test.rb b/actionview/test/template/form_options_helper_test.rb index bfe9ac8d41..dc6990c4b7 100644 --- a/actionview/test/template/form_options_helper_test.rb +++ b/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 From 22115751f3e5c22e00437b6e085c58311181afc8 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 2 Jan 2020 12:19:30 -0800 Subject: [PATCH 2/3] Define module attr methods at caller location Before: [1] pry(main)> $ ActiveJob::Base.default_priority From: lib/active_support/core_ext/module/attribute_accessors.rb @ line 64: Owner: ActiveJob::QueuePriority::ClassMethods Visibility: public Number of lines: 3 def #{sym} @@#{sym} end After: [1] pry(main)> $ ActiveJob::Base.default_priority From: /home/jhawthorn/src/rails/activejob/lib/active_job/queue_priority.rb @ line 9: Owner: ActiveJob::QueuePriority::ClassMethods Visibility: public Number of lines: 1 mattr_accessor :default_priority --- .../core_ext/module/attribute_accessors.rb | 51 ++++++++----------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb b/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb index cc1926e022..f9cf38dd50 100644 --- a/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb +++ b/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb @@ -48,28 +48,23 @@ 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) + 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 +110,23 @@ 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) + 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 +195,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 From b5b1b02087cac08d43a3174cfb8c0909ec6bb6ea Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Sat, 4 Jan 2020 17:52:15 -0800 Subject: [PATCH 3/3] Error if mattr_accessor is called on singleton --- .../core_ext/module/attribute_accessors.rb | 3 +++ .../test/core_ext/module/attribute_accessor_test.rb | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb b/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb index f9cf38dd50..1db905ff65 100644 --- a/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb +++ b/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb @@ -49,11 +49,13 @@ class Module # # Person.new.hair_colors # => [:brown, :black, :blonde, :red] 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) + definition << "def self.#{sym}; @@#{sym}; end" if instance_reader && instance_accessor @@ -111,6 +113,7 @@ class Module # # Person.class_variable_get("@@hair_colors") # => [:brown, :black, :blonde, :red] 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 = [] diff --git a/activesupport/test/core_ext/module/attribute_accessor_test.rb b/activesupport/test/core_ext/module/attribute_accessor_test.rb index 7ac76251df..a6af1ea11d 100644 --- a/activesupport/test/core_ext/module/attribute_accessor_test.rb +++ b/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