From c8ea5b8615aea926cb7d56ff632030e4da879563 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Sun, 11 Apr 2021 16:10:45 +0200 Subject: [PATCH 1/2] Use alias_method rather than define_method to handle non compilable names --- .../lib/active_model/attribute_methods.rb | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index d0910a384c..833446633a 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -406,10 +406,9 @@ module ActiveModel # using the given `extra` args. This falls back on `define_method` # and `send` if the given names cannot be compiled. def define_proxy_call(code_generator, name, target, *extra) - defn = if NAME_COMPILABLE_REGEXP.match?(name) - "def #{name}(*args)" - else - "define_method(:'#{name}') do |*args|" + mangled_name = name + unless NAME_COMPILABLE_REGEXP.match?(name) + mangled_name = "__temp__#{name.unpack1("h*")}" end extra = (extra.map!(&:inspect) << "*args").join(", ") @@ -421,10 +420,17 @@ module ActiveModel end code_generator << - defn << + "def #{mangled_name}(*args)" << body << "end" << - "ruby2_keywords(:'#{name}')" + "ruby2_keywords(:'#{mangled_name}')" + + if mangled_name != name + code_generator << + "alias_method(:'#{name}', :'#{mangled_name}')" << + "remove_method(:'#{mangled_name}')" + end + end class AttributeMethodMatcher #:nodoc: From eece0957650ac11ffafe5c177c405ff06bba6b68 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Sun, 11 Apr 2021 17:03:25 +0200 Subject: [PATCH 2/2] Allow to pass the method signature when defining attribute methods This saves some array allocations from avoiding `*args`, as well as makes the Method object `arity` and `parameters` correct. e.g. before this patch, ArgumentError would be confusing: ```ruby >> model.name_was(1) ArgumentError: wrong number of arguments (given 2, expected 1) ``` --- .../lib/active_model/attribute_methods.rb | 43 +++++++++++-------- activemodel/lib/active_model/attributes.rb | 2 +- activemodel/lib/active_model/dirty.rb | 9 ++-- activemodel/test/models/topic.rb | 2 +- .../attribute_methods/before_type_cast.rb | 4 +- .../active_record/attribute_methods/dirty.rb | 10 ++--- .../active_record/attribute_methods/query.rb | 2 +- .../active_record/attribute_methods/write.rb | 2 +- 8 files changed, 41 insertions(+), 33 deletions(-) diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index 833446633a..0027013197 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -67,6 +67,7 @@ module ActiveModel NAME_COMPILABLE_REGEXP = /\A[a-zA-Z_]\w*[!?=]?\z/ CALL_COMPILABLE_REGEXP = /\A[a-zA-Z_]\w*[!?]?\z/ + FORWARD_PARAMETERS = "*args" included do class_attribute :attribute_aliases, instance_writer: false, default: {} @@ -105,8 +106,8 @@ module ActiveModel # person.name # => "Bob" # person.clear_name # person.name # => nil - def attribute_method_prefix(*prefixes) - self.attribute_method_matchers += prefixes.map! { |prefix| AttributeMethodMatcher.new prefix: prefix } + def attribute_method_prefix(*prefixes, parameters: nil) + self.attribute_method_matchers += prefixes.map! { |prefix| AttributeMethodMatcher.new(prefix: prefix, parameters: parameters) } undefine_attribute_methods end @@ -140,8 +141,8 @@ module ActiveModel # person.name = 'Bob' # person.name # => "Bob" # person.name_short? # => true - def attribute_method_suffix(*suffixes) - self.attribute_method_matchers += suffixes.map! { |suffix| AttributeMethodMatcher.new suffix: suffix } + def attribute_method_suffix(*suffixes, parameters: nil) + self.attribute_method_matchers += suffixes.map! { |suffix| AttributeMethodMatcher.new(suffix: suffix, parameters: parameters) } undefine_attribute_methods end @@ -177,7 +178,7 @@ module ActiveModel # person.reset_name_to_default! # person.name # => 'Default Name' def attribute_method_affix(*affixes) - self.attribute_method_matchers += affixes.map! { |affix| AttributeMethodMatcher.new prefix: affix[:prefix], suffix: affix[:suffix] } + self.attribute_method_matchers += affixes.map! { |affix| AttributeMethodMatcher.new(**affix) } undefine_attribute_methods end @@ -211,7 +212,7 @@ module ActiveModel attribute_method_matchers.each do |matcher| matcher_new = matcher.method_name(new_name).to_s matcher_old = matcher.method_name(old_name).to_s - define_proxy_call owner, matcher_new, matcher_old + define_proxy_call(owner, matcher_new, matcher_old, matcher.parameters) end end end @@ -296,7 +297,7 @@ module ActiveModel if respond_to?(generate_method, true) send(generate_method, attr_name.to_s, owner: owner) else - define_proxy_call owner, method_name, matcher.target, attr_name.to_s + define_proxy_call(owner, method_name, matcher.target, matcher.parameters, attr_name.to_s) end end end @@ -405,41 +406,47 @@ module ActiveModel # Define a method `name` in `mod` that dispatches to `send` # using the given `extra` args. This falls back on `define_method` # and `send` if the given names cannot be compiled. - def define_proxy_call(code_generator, name, target, *extra) + def define_proxy_call(code_generator, name, target, parameters, *call_args) mangled_name = name unless NAME_COMPILABLE_REGEXP.match?(name) mangled_name = "__temp__#{name.unpack1("h*")}" end - extra = (extra.map!(&:inspect) << "*args").join(", ") + call_args.map!(&:inspect) + call_args << parameters if parameters body = if CALL_COMPILABLE_REGEXP.match?(target) - "self.#{target}(#{extra})" + "self.#{target}(#{call_args.join(", ")})" else - "send(:'#{target}', #{extra})" + call_args.unshift(":'#{target}'") + "send(#{call_args.join(", ")})" end code_generator << - "def #{mangled_name}(*args)" << + "def #{mangled_name}(#{parameters || ''})" << body << - "end" << - "ruby2_keywords(:'#{mangled_name}')" + "end" + + if parameters == FORWARD_PARAMETERS + code_generator << "ruby2_keywords(:'#{mangled_name}')" + end if mangled_name != name code_generator << "alias_method(:'#{name}', :'#{mangled_name}')" << "remove_method(:'#{mangled_name}')" end - end class AttributeMethodMatcher #:nodoc: - attr_reader :prefix, :suffix, :target + attr_reader :prefix, :suffix, :target, :parameters AttributeMethodMatch = Struct.new(:target, :attr_name) - def initialize(options = {}) - @prefix, @suffix = options.fetch(:prefix, ""), options.fetch(:suffix, "") + def initialize(prefix: "", suffix: "", parameters: nil) + @prefix = prefix + @suffix = suffix + @parameters = parameters.nil? ? FORWARD_PARAMETERS : parameters @regex = /^(?:#{Regexp.escape(@prefix)})(.*)(?:#{Regexp.escape(@suffix)})$/ @target = "#{@prefix}attribute#{@suffix}" @method_name = "#{prefix}%s#{suffix}" diff --git a/activemodel/lib/active_model/attributes.rb b/activemodel/lib/active_model/attributes.rb index 8d16614979..347fe7ac2c 100644 --- a/activemodel/lib/active_model/attributes.rb +++ b/activemodel/lib/active_model/attributes.rb @@ -9,7 +9,7 @@ module ActiveModel include ActiveModel::AttributeMethods included do - attribute_method_suffix "=" + attribute_method_suffix "=", parameters: "value" class_attribute :attribute_types, :_default_attributes, instance_accessor: false self.attribute_types = Hash.new(Type.default_value) self._default_attributes = AttributeSet.new({}) diff --git a/activemodel/lib/active_model/dirty.rb b/activemodel/lib/active_model/dirty.rb index a193a28e6a..76770e7b6e 100644 --- a/activemodel/lib/active_model/dirty.rb +++ b/activemodel/lib/active_model/dirty.rb @@ -123,10 +123,11 @@ module ActiveModel include ActiveModel::AttributeMethods included do - attribute_method_suffix "_changed?", "_change", "_will_change!", "_was" - attribute_method_suffix "_previously_changed?", "_previous_change", "_previously_was" - attribute_method_affix prefix: "restore_", suffix: "!" - attribute_method_affix prefix: "clear_", suffix: "_change" + attribute_method_suffix "_previously_changed?", "_changed?", parameters: "**options" + attribute_method_suffix "_change", "_will_change!", "_was", parameters: false + attribute_method_suffix "_previous_change", "_previously_was", parameters: false + attribute_method_affix prefix: "restore_", suffix: "!", parameters: false + attribute_method_affix prefix: "clear_", suffix: "_change", parameters: false end def initialize_dup(other) # :nodoc: diff --git a/activemodel/test/models/topic.rb b/activemodel/test/models/topic.rb index dc6ded553d..507e246b67 100644 --- a/activemodel/test/models/topic.rb +++ b/activemodel/test/models/topic.rb @@ -6,7 +6,7 @@ class Topic include ActiveModel::AttributeMethods include ActiveSupport::NumberHelper - attribute_method_suffix "_before_type_cast" + attribute_method_suffix "_before_type_cast", parameters: false define_attribute_method :price def self._validates_default_keys diff --git a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb index 33ca3d38b6..e4b3ce32cc 100644 --- a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb +++ b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb @@ -29,8 +29,8 @@ module ActiveRecord extend ActiveSupport::Concern included do - attribute_method_suffix "_before_type_cast", "_for_database" - attribute_method_suffix "_came_from_user?" + attribute_method_suffix "_before_type_cast", "_for_database", parameters: false + attribute_method_suffix "_came_from_user?", parameters: false end # Returns the value of the attribute identified by +attr_name+ before diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 44f222e534..a1d8b51ed6 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -17,13 +17,13 @@ module ActiveRecord class_attribute :partial_writes, instance_writer: false, default: true # Attribute methods for "changed in last call to save?" - attribute_method_affix(prefix: "saved_change_to_", suffix: "?") - attribute_method_prefix("saved_change_to_") - attribute_method_suffix("_before_last_save") + attribute_method_affix(prefix: "saved_change_to_", suffix: "?", parameters: "**options") + attribute_method_prefix("saved_change_to_", parameters: false) + attribute_method_suffix("_before_last_save", parameters: false) # Attribute methods for "will change if I call save?" - attribute_method_affix(prefix: "will_save_change_to_", suffix: "?") - attribute_method_suffix("_change_to_be_saved", "_in_database") + attribute_method_affix(prefix: "will_save_change_to_", suffix: "?", parameters: "**options") + attribute_method_suffix("_change_to_be_saved", "_in_database", parameters: false) end # reload the record and clears changed attributes. diff --git a/activerecord/lib/active_record/attribute_methods/query.rb b/activerecord/lib/active_record/attribute_methods/query.rb index 55c92e19b8..1430d8da75 100644 --- a/activerecord/lib/active_record/attribute_methods/query.rb +++ b/activerecord/lib/active_record/attribute_methods/query.rb @@ -6,7 +6,7 @@ module ActiveRecord extend ActiveSupport::Concern included do - attribute_method_suffix "?" + attribute_method_suffix "?", parameters: false end def query_attribute(attr_name) diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb index d78426604d..6757e637b6 100644 --- a/activerecord/lib/active_record/attribute_methods/write.rb +++ b/activerecord/lib/active_record/attribute_methods/write.rb @@ -6,7 +6,7 @@ module ActiveRecord extend ActiveSupport::Concern included do - attribute_method_suffix "=" + attribute_method_suffix "=", parameters: "value" end module ClassMethods # :nodoc: