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.

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