diff options
author | Matthew Draper <matthew@trebex.net> | 2017-05-26 13:12:21 +0930 |
---|---|---|
committer | Matthew Draper <matthew@trebex.net> | 2017-09-01 14:27:13 +0930 |
commit | 2e6658ae510e17e9e6e98ebd784066752ea6027c (patch) | |
tree | a5424b329e24f37cab0b6e4698b39a0ac592573f /activesupport | |
parent | 2cd8ac1b68ba04ca12a816c8113271017b3c43c4 (diff) | |
download | rails-2e6658ae510e17e9e6e98ebd784066752ea6027c.tar.gz rails-2e6658ae510e17e9e6e98ebd784066752ea6027c.tar.bz2 rails-2e6658ae510e17e9e6e98ebd784066752ea6027c.zip |
Clarify intentions around method redefinitions
Don't use remove_method or remove_possible_method just before a new
definition: at best the purpose is unclear, and at worst it creates a
race condition.
Instead, prefer redefine_method when practical, and
silence_redefinition_of_method otherwise.
Diffstat (limited to 'activesupport')
9 files changed, 69 insertions, 54 deletions
diff --git a/activesupport/lib/active_support/core_ext/class/attribute.rb b/activesupport/lib/active_support/core_ext/class/attribute.rb index e5a52db36a..3453fc4ac6 100644 --- a/activesupport/lib/active_support/core_ext/class/attribute.rb +++ b/activesupport/lib/active_support/core_ext/class/attribute.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require_relative "../kernel/singleton_class" -require_relative "../module/remove_method" +require_relative "../module/redefine_method" require_relative "../array/extract_options" class Class @@ -92,25 +92,23 @@ class Class default_value = options.fetch(:default, nil) attrs.each do |name| - remove_possible_singleton_method(name) + singleton_class.silence_redefinition_of_method(name) define_singleton_method(name) { nil } - remove_possible_singleton_method("#{name}?") + singleton_class.silence_redefinition_of_method("#{name}?") define_singleton_method("#{name}?") { !!public_send(name) } if instance_predicate ivar = "@#{name}" - remove_possible_singleton_method("#{name}=") + singleton_class.silence_redefinition_of_method("#{name}=") define_singleton_method("#{name}=") do |val| singleton_class.class_eval do - remove_possible_method(name) - define_method(name) { val } + redefine_method(name) { val } end if singleton_class? class_eval do - remove_possible_method(name) - define_method(name) do + redefine_method(name) do if instance_variable_defined? ivar instance_variable_get ivar else @@ -123,8 +121,7 @@ class Class end if instance_reader - remove_possible_method name - define_method(name) do + redefine_method(name) do if instance_variable_defined?(ivar) instance_variable_get ivar else @@ -132,13 +129,13 @@ class Class end end - remove_possible_method "#{name}?" - define_method("#{name}?") { !!public_send(name) } if instance_predicate + redefine_method("#{name}?") { !!public_send(name) } if instance_predicate end if instance_writer - remove_possible_method "#{name}=" - attr_writer name + redefine_method("#{name}=") do |val| + instance_variable_set ivar, val + end end unless default_value.nil? diff --git a/activesupport/lib/active_support/core_ext/date/conversions.rb b/activesupport/lib/active_support/core_ext/date/conversions.rb index 000d6f61ca..88fb572725 100644 --- a/activesupport/lib/active_support/core_ext/date/conversions.rb +++ b/activesupport/lib/active_support/core_ext/date/conversions.rb @@ -3,7 +3,7 @@ require "date" require_relative "../../inflector/methods" require_relative "zones" -require_relative "../module/remove_method" +require_relative "../module/redefine_method" class Date DATE_FORMATS = { @@ -19,14 +19,6 @@ class Date iso8601: lambda { |date| date.iso8601 } } - # Ruby 1.9 has Date#to_time which converts to localtime only. - remove_method :to_time - - # Ruby 1.9 has Date#xmlschema which converts to a string without the time - # component. This removal may generate an issue on FreeBSD, that's why we - # need to use remove_possible_method here - remove_possible_method :xmlschema - # Convert to a formatted string. See DATE_FORMATS for predefined formats. # # This method is aliased to <tt>to_s</tt>. @@ -72,6 +64,8 @@ class Date alias_method :default_inspect, :inspect alias_method :inspect, :readable_inspect + silence_redefinition_of_method :to_time + # Converts a Date instance to a Time, where the time is set to the beginning of the day. # The timezone can be either :local or :utc (default :local). # @@ -89,6 +83,8 @@ class Date ::Time.send(form, year, month, day) end + silence_redefinition_of_method :xmlschema + # Returns a string which represents the time in used time zone as DateTime # defined by XML Schema: # diff --git a/activesupport/lib/active_support/core_ext/date_time/compatibility.rb b/activesupport/lib/active_support/core_ext/date_time/compatibility.rb index 71da7be8ca..bb9714545e 100644 --- a/activesupport/lib/active_support/core_ext/date_time/compatibility.rb +++ b/activesupport/lib/active_support/core_ext/date_time/compatibility.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true require_relative "../date_and_time/compatibility" -require_relative "../module/remove_method" +require_relative "../module/redefine_method" class DateTime include DateAndTime::Compatibility - remove_possible_method :to_time + silence_redefinition_of_method :to_time # Either return an instance of `Time` with the same UTC offset # as +self+ or an instance of `Time` representing the same time diff --git a/activesupport/lib/active_support/core_ext/module.rb b/activesupport/lib/active_support/core_ext/module.rb index da8f572c3b..8445643730 100644 --- a/activesupport/lib/active_support/core_ext/module.rb +++ b/activesupport/lib/active_support/core_ext/module.rb @@ -10,4 +10,5 @@ require_relative "module/attr_internal" require_relative "module/concerning" require_relative "module/delegation" require_relative "module/deprecation" +require_relative "module/redefine_method" require_relative "module/remove_method" diff --git a/activesupport/lib/active_support/core_ext/module/redefine_method.rb b/activesupport/lib/active_support/core_ext/module/redefine_method.rb new file mode 100644 index 0000000000..5bd8e6e973 --- /dev/null +++ b/activesupport/lib/active_support/core_ext/module/redefine_method.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +class Module + # Marks the named method as intended to be redefined, if it exists. + # Suppresses the Ruby method redefinition warning. Prefer + # #redefine_method where possible. + def silence_redefinition_of_method(method) + if method_defined?(method) || private_method_defined?(method) + # This suppresses the "method redefined" warning; the self-alias + # looks odd, but means we don't need to generate a unique name + alias_method method, method + end + end + + # Replaces the existing method definition, if there is one, with the passed + # block as its body. + def redefine_method(method, &block) + visibility = method_visibility(method) + silence_redefinition_of_method(method) + define_method(method, &block) + send(visibility, method) + end + + # Replaces the existing singleton method definition, if there is one, with + # the passed block as its body. + def redefine_singleton_method(method, &block) + singleton_class.redefine_method(method, &block) + end + + def method_visibility(method) # :nodoc: + case + when private_method_defined?(method) + :private + when protected_method_defined?(method) + :protected + else + :public + end + end +end diff --git a/activesupport/lib/active_support/core_ext/module/remove_method.rb b/activesupport/lib/active_support/core_ext/module/remove_method.rb index bf0d686e16..704f144bdd 100644 --- a/activesupport/lib/active_support/core_ext/module/remove_method.rb +++ b/activesupport/lib/active_support/core_ext/module/remove_method.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative "redefine_method" + class Module # Removes the named method, if it exists. def remove_possible_method(method) @@ -10,28 +12,6 @@ class Module # Removes the named singleton method, if it exists. def remove_possible_singleton_method(method) - singleton_class.instance_eval do - remove_possible_method(method) - end - end - - # Replaces the existing method definition, if there is one, with the passed - # block as its body. - def redefine_method(method, &block) - visibility = method_visibility(method) - remove_possible_method(method) - define_method(method, &block) - send(visibility, method) - end - - def method_visibility(method) # :nodoc: - case - when private_method_defined?(method) - :private - when protected_method_defined?(method) - :protected - else - :public - end + singleton_class.remove_possible_method(method) end end diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb index adcd2b1dca..600b41da10 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -2,6 +2,7 @@ require "erb" require_relative "../kernel/singleton_class" +require_relative "../module/redefine_method" require_relative "../../multibyte/unicode" class ERB @@ -23,13 +24,12 @@ class ERB unwrapped_html_escape(s).html_safe end - # Aliasing twice issues a warning "discarding old...". Remove first to avoid it. - remove_method(:h) + silence_redefinition_of_method :h alias h html_escape module_function :h - singleton_class.send(:remove_method, :html_escape) + singleton_class.silence_redefinition_of_method :html_escape module_function :html_escape # HTML escapes strings but doesn't wrap them with an ActiveSupport::SafeBuffer. diff --git a/activesupport/lib/active_support/core_ext/time/compatibility.rb b/activesupport/lib/active_support/core_ext/time/compatibility.rb index 93840e93ca..147ae0f802 100644 --- a/activesupport/lib/active_support/core_ext/time/compatibility.rb +++ b/activesupport/lib/active_support/core_ext/time/compatibility.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true require_relative "../date_and_time/compatibility" -require_relative "../module/remove_method" +require_relative "../module/redefine_method" class Time include DateAndTime::Compatibility - remove_possible_method :to_time + silence_redefinition_of_method :to_time # Either return +self+ or the time in the local system timezone depending # on the setting of +ActiveSupport.to_time_preserves_timezone+. diff --git a/activesupport/lib/active_support/core_ext/uri.rb b/activesupport/lib/active_support/core_ext/uri.rb index 60fc7f084f..c93c0b5c2d 100644 --- a/activesupport/lib/active_support/core_ext/uri.rb +++ b/activesupport/lib/active_support/core_ext/uri.rb @@ -5,8 +5,9 @@ str = "\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E" # Ni-ho-nn-go in UTF-8, means Japan parser = URI::Parser.new unless str == parser.unescape(parser.escape(str)) + require "active_support/core_ext/module/redefine_method" URI::Parser.class_eval do - remove_method :unescape + silence_redefinition_of_method :unescape def unescape(str, escaped = /%[a-fA-F\d]{2}/) # TODO: Are we actually sure that ASCII == UTF-8? # YK: My initial experiments say yes, but let's be sure please |