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.

actioncable/lib/action_cable/remote_connections.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "active_support/core_ext/module/redefine_method"
+
 module ActionCable
   # If you need to disconnect a given connection, you can go through the
   # RemoteConnections. You can find the connections you're looking for by

actionpack/lib/action_controller/metal/renderers.rb

@@ -85,7 +85,7 @@ module ActionController
     def self.remove(key)
       RENDERERS.delete(key.to_sym)
       method_name = _render_with_renderer_method_name(key)
-      remove_method(method_name) if method_defined?(method_name)
+      remove_possible_method(method_name)
     end
 
     def self._render_with_renderer_method_name(key)

actionpack/lib/action_controller/test_case.rb

@@ -4,6 +4,7 @@ require "rack/session/abstract/id"
 require "active_support/core_ext/hash/conversions"
 require "active_support/core_ext/object/to_query"
 require "active_support/core_ext/module/anonymous"
+require "active_support/core_ext/module/redefine_method"
 require "active_support/core_ext/hash/keys"
 require "active_support/testing/constant_lookup"
 require_relative "template_assertions"
@@ -19,7 +20,7 @@ module ActionController
     # the database on the main thread, so they could open a txn, then the
     # controller thread will open a new connection and try to access data
     # that's only visible to the main thread's txn. This is the problem in #23483.
-    remove_method :new_controller_thread
+    silence_redefinition_of_method :new_controller_thread
     def new_controller_thread # :nodoc:
       yield
     end

actionpack/lib/action_dispatch/routing/route_set.rb

@@ -3,6 +3,7 @@
 require_relative "../journey"
 require "active_support/core_ext/object/to_query"
 require "active_support/core_ext/hash/slice"
+require "active_support/core_ext/module/redefine_method"
 require "active_support/core_ext/module/remove_method"
 require "active_support/core_ext/array/extract_options"
 require "action_controller/metal/exceptions"
@@ -546,7 +547,7 @@ module ActionDispatch
 
           # plus a singleton class method called _routes ...
           included do
-            singleton_class.send(:redefine_method, :_routes) { routes }
+            redefine_singleton_method(:_routes) { routes }
           end
 
           # And an instance method _routes. Note that

actionview/lib/action_view/layouts.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 require_relative "rendering"
-require "active_support/core_ext/module/remove_method"
+require "active_support/core_ext/module/redefine_method"
 
 module ActionView
   # Layouts reverse the common pattern of including shared headers and footers in many templates to isolate changes in
@@ -279,7 +279,7 @@ module ActionView
       # If a layout is not explicitly mentioned then look for a layout with the controller's name.
       # if nothing is found then try same procedure to find super class's layout.
       def _write_layout_method # :nodoc:
-        remove_possible_method(:_layout)
+        silence_redefinition_of_method(:_layout)
 
         prefixes = /\blayouts/.match?(_implied_layout_name) ? [] : ["layouts"]
         default_behavior = "lookup_context.find_all('#{_implied_layout_name}', #{prefixes.inspect}, false, [], { formats: formats }).first || super"

actionview/lib/action_view/test_case.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require "active_support/core_ext/module/remove_method"
+require "active_support/core_ext/module/redefine_method"
 require "action_controller"
 require "action_controller/test_case"
 require "action_view"
@@ -171,7 +171,7 @@ module ActionView
 
       def say_no_to_protect_against_forgery!
         _helpers.module_eval do
-          remove_possible_method :protect_against_forgery?
+          silence_redefinition_of_method :protect_against_forgery?
           def protect_against_forgery?
             false
           end

activemodel/lib/active_model/naming.rb

@@ -2,7 +2,7 @@
 
 require "active_support/core_ext/hash/except"
 require "active_support/core_ext/module/introspection"
-require "active_support/core_ext/module/remove_method"
+require "active_support/core_ext/module/redefine_method"
 
 module ActiveModel
   class Name
@@ -218,7 +218,7 @@ module ActiveModel
   # provided method below, or rolling your own is required.
   module Naming
     def self.extended(base) #:nodoc:
-      base.remove_possible_method :model_name
+      base.silence_redefinition_of_method :model_name
       base.delegate :model_name, to: :class
     end
 

activerecord/lib/active_record/nested_attributes.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "active_support/core_ext/hash/except"
+require "active_support/core_ext/module/redefine_method"
 require "active_support/core_ext/object/try"
 require "active_support/core_ext/hash/indifferent_access"
 
@@ -355,9 +356,7 @@ module ActiveRecord
         # associations are just regular associations.
         def generate_association_writer(association_name, type)
           generated_association_methods.module_eval <<-eoruby, __FILE__, __LINE__ + 1
-            if method_defined?(:#{association_name}_attributes=)
+            silence_redefinition_of_method :#{association_name}_attributes=
-              remove_method(:#{association_name}_attributes=)
-            end
             def #{association_name}_attributes=(attributes)
               assign_nested_attributes_for_#{type}_association(:#{association_name}, attributes)
             end

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)
+          redefine_method(name) { val }
-          define_method(name) { val }
         end
 
         if singleton_class?
           class_eval do
-            remove_possible_method(name)
+            redefine_method(name) do
-            define_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
+        redefine_method(name) do
-        define_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}?"
+        redefine_method("#{name}?") { !!public_send(name) } if instance_predicate
-        define_method("#{name}?") { !!public_send(name) } if instance_predicate
       end
 
       if instance_writer
-        remove_possible_method "#{name}="
+        redefine_method("#{name}=") do |val|
-        attr_writer name
+          instance_variable_set ivar, val
+        end
       end
 
       unless default_value.nil?

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:
   #

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

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"

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

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
+    singleton_class.remove_possible_method(method)
-      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
   end
 end

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.
+    silence_redefinition_of_method :h
-    remove_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.

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+.

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

guides/source/active_support_core_extensions.md

@@ -864,7 +864,11 @@ There are cases where you need to define a method with `define_method`, but don'
 
 The method `redefine_method` prevents such a potential warning, removing the existing method before if needed.
 
-NOTE: Defined in `active_support/core_ext/module/remove_method.rb`.
+You can also use `silence_redefinition_of_method` if you need to define
+the replacement method yourself (because you're using `delegate`, for
+example).
+
+NOTE: Defined in `active_support/core_ext/module/redefine_method.rb`.
 
 Extensions to `Class`
 ---------------------