Remove implicit coercion deprecation of durations

In #28204 we deprecated implicit conversion of durations to a
numeric which represented the number of seconds in the duration
because of unwanted side effects with calculations on durations
and dates. This unfortunately had the side effect of forcing a
explicit cast when configuring third-party libraries like
expiration in Redis, e.g:

    redis.expire("foo", 5.minutes)

To work around this we've removed the deprecation and added a
private class that wraps the numeric and can perform calculation
involving durations and ensure that they remain a duration
irrespective of the order of operations.

3 files changed, 211 insertions, 35 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index de1418bb86..738b6d5d1d 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,19 @@
+*   Remove implicit coercion deprecation of durations
+
+    In #28204 we deprecated implicit conversion of durations to a numeric which
+    represented the number of seconds in the duration because of unwanted side
+    effects with calculations on durations and dates. This unfortunately had
+    the side effect of forcing a explicit cast when configuring third-party
+    libraries like expiration in Redis, e.g:
+
+        redis.expire("foo", 5.minutes)
+
+    To work around this we've removed the deprecation and added a private class
+    that wraps the numeric and can perform calculation involving durations and
+    ensure that they remain a duration irrespective of the order of operations.
+
+    *Andrew White*
+
 *   Update `titleize` regex to allow apostrophes
 
     In 4b685aa the regex in `titleize` was updated to not match apostrophes to
diff --git a/activesupport/lib/active_support/duration.rb b/activesupport/lib/active_support/duration.rb
index d26bbac511..99080e34a1 100644
--- a/activesupport/lib/active_support/duration.rb
+++ b/activesupport/lib/active_support/duration.rb
@@ -1,4 +1,5 @@
 require "active_support/core_ext/array/conversions"
+require "active_support/core_ext/module/delegation"
 require "active_support/core_ext/object/acts_like"
 require "active_support/core_ext/string/filters"
 require "active_support/deprecation"
@@ -9,6 +10,66 @@ module ActiveSupport
   #
   #   1.month.ago       # equivalent to Time.now.advance(months: -1)
   class Duration
+    class Scalar < Numeric #:nodoc:
+      attr_reader :value
+      delegate :to_i, :to_f, :to_s, to: :value
+
+      def initialize(value)
+        @value = value
+      end
+
+      def coerce(other)
+        [Scalar.new(other), self]
+      end
+
+      def -@
+        Scalar.new(-value)
+      end
+
+      def <=>(other)
+        if Scalar === other || Duration === other
+          value <=> other.value
+        elsif Numeric === other
+          value <=> other
+        else
+          nil
+        end
+      end
+
+      def +(other)
+        calculate(:+, other)
+      end
+
+      def -(other)
+        calculate(:-, other)
+      end
+
+      def *(other)
+        calculate(:*, other)
+      end
+
+      def /(other)
+        calculate(:/, other)
+      end
+
+      private
+        def calculate(op, other)
+          if Scalar === other
+            Scalar.new(value.public_send(op, other.value))
+          elsif Duration === other
+            Duration.seconds(value).public_send(op, other)
+          elsif Numeric === other
+            Scalar.new(value.public_send(op, other))
+          else
+            raise_type_error(other)
+          end
+        end
+
+        def raise_type_error(other)
+          raise TypeError, "no implicit conversion of #{other.class} into #{self.class}"
+        end
+    end
+
     SECONDS_PER_MINUTE = 60
     SECONDS_PER_HOUR   = 3600
     SECONDS_PER_DAY    = 86400
@@ -91,12 +152,11 @@ module ActiveSupport
     end
 
     def coerce(other) #:nodoc:
-      ActiveSupport::Deprecation.warn(<<-MSG.squish)
-        Implicit coercion of ActiveSupport::Duration to a Numeric
-        is deprecated and will raise a TypeError in Rails 5.2.
-      MSG
-
-      [other, value]
+      if Scalar === other
+        [other, self]
+      else
+        [Scalar.new(other), self]
+      end
     end
 
     # Compares one Duration with another or a Numeric to this Duration.
@@ -132,19 +192,23 @@ module ActiveSupport
 
     # Multiplies this Duration by a Numeric and returns a new Duration.
     def *(other)
-      if Numeric === other
+      if Scalar === other || Duration === other
+        Duration.new(value * other.value, parts.map { |type, number| [type, number * other.value] })
+      elsif Numeric === other
         Duration.new(value * other, parts.map { |type, number| [type, number * other] })
       else
-        value * other
+        raise_type_error(other)
       end
     end
 
     # Divides this Duration by a Numeric and returns a new Duration.
     def /(other)
-      if Numeric === other
+      if Scalar === other || Duration === other
+        Duration.new(value / other.value, parts.map { |type, number| [type, number / other.value] })
+      elsif Numeric === other
         Duration.new(value / other, parts.map { |type, number| [type, number / other] })
       else
-        value / other
+        raise_type_error(other)
       end
     end
 
@@ -274,5 +338,9 @@ module ActiveSupport
       def method_missing(method, *args, &block)
         value.send(method, *args, &block)
       end
+
+      def raise_type_error(other)
+        raise TypeError, "no implicit conversion of #{other.class} into #{self.class}"
+      end
   end
 end
diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb
index 2b1a715b7a..52fdbe677d 100644
--- a/activesupport/test/core_ext/duration_test.rb
+++ b/activesupport/test/core_ext/duration_test.rb
@@ -96,24 +96,20 @@ class DurationTest < ActiveSupport::TestCase
     assert_instance_of ActiveSupport::Duration, 2.seconds - 1.second
     assert_equal 1.second, 2.seconds - 1
     assert_instance_of ActiveSupport::Duration, 2.seconds - 1
+    assert_equal 1.second, 2 - 1.second
+    assert_instance_of ActiveSupport::Duration, 2.seconds - 1
   end
 
   def test_multiply
     assert_equal 7.days, 1.day * 7
     assert_instance_of ActiveSupport::Duration, 1.day * 7
-
-    assert_deprecated do
-      assert_equal 86400, 1.day * 1.second
-    end
+    assert_equal 86400, 1.day * 1.second
   end
 
   def test_divide
     assert_equal 1.day, 7.days / 7
     assert_instance_of ActiveSupport::Duration, 7.days / 7
-
-    assert_deprecated do
-      assert_equal 1, 1.day / 1.day
-    end
+    assert_equal 1, 1.day / 1.day
   end
 
   def test_date_added_with_multiplied_duration
@@ -121,9 +117,7 @@ class DurationTest < ActiveSupport::TestCase
   end
 
   def test_plus_with_time
-    assert_deprecated do
-      assert_equal 1 + 1.second, 1.second + 1, "Duration + Numeric should == Numeric + Duration"
-    end
+    assert_equal 1 + 1.second, 1.second + 1, "Duration + Numeric should == Numeric + Duration"
   end
 
   def test_time_plus_duration_returns_same_time_datatype
@@ -142,13 +136,6 @@ class DurationTest < ActiveSupport::TestCase
     assert_equal 'expected a time or date, got ""', e.message, "ensure ArgumentError is not being raised by dependencies.rb"
   end
 
-  def test_implicit_coercion_is_deprecated
-    assert_deprecated { 1 + 1.second }
-    assert_deprecated { 1 - 1.second }
-    assert_deprecated { 1 * 1.second }
-    assert_deprecated { 1 / 1.second }
-  end
-
   def test_fractional_weeks
     assert_equal((86400 * 7) * 1.5, 1.5.weeks)
     assert_equal((86400 * 7) * 1.7, 1.7.weeks)
@@ -286,20 +273,125 @@ class DurationTest < ActiveSupport::TestCase
   def test_comparable
     assert_equal(-1, (0.seconds <=> 1.second))
     assert_equal(-1, (1.second <=> 1.minute))
-
-    assert_deprecated do
-      assert_equal(-1, (1 <=> 1.minute))
-    end
-
+    assert_equal(-1, (1 <=> 1.minute))
     assert_equal(0, (0.seconds <=> 0.seconds))
     assert_equal(0, (0.seconds <=> 0.minutes))
     assert_equal(0, (1.second <=> 1.second))
     assert_equal(1, (1.second <=> 0.second))
     assert_equal(1, (1.minute <=> 1.second))
+    assert_equal(1, (61 <=> 1.minute))
+  end
 
-    assert_deprecated do
-      assert_equal(1, (61 <=> 1.minute))
+  def test_implicit_coercion
+    assert_equal 2.days, 2 * 1.day
+    assert_instance_of ActiveSupport::Duration, 2 * 1.day
+    assert_equal Time.utc(2017, 1, 3), Time.utc(2017, 1, 1) + 2 * 1.day
+    assert_equal Date.civil(2017, 1, 3), Date.civil(2017, 1, 1) + 2 * 1.day
+  end
+
+  def test_scalar_coerce
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+    assert_instance_of ActiveSupport::Duration::Scalar, 10 + scalar
+    assert_instance_of ActiveSupport::Duration, 10.seconds + scalar
+  end
+
+  def test_scalar_delegations
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+    assert_kind_of Float, scalar.to_f
+    assert_kind_of Integer, scalar.to_i
+    assert_kind_of String, scalar.to_s
+  end
+
+  def test_scalar_unary_minus
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+
+    assert_equal -10, -scalar
+    assert_instance_of ActiveSupport::Duration::Scalar, -scalar
+  end
+
+  def test_scalar_compare
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+
+    assert_equal 1, scalar <=> 5
+    assert_equal 0, scalar <=> 10
+    assert_equal -1, scalar <=> 15
+    assert_equal nil, scalar <=> "foo"
+  end
+
+  def test_scalar_plus
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+
+    assert_equal 20, 10 + scalar
+    assert_instance_of ActiveSupport::Duration::Scalar, 10 + scalar
+    assert_equal 20, scalar + 10
+    assert_instance_of ActiveSupport::Duration::Scalar, scalar + 10
+    assert_equal 20, 10.seconds + scalar
+    assert_instance_of ActiveSupport::Duration, 10.seconds + scalar
+    assert_equal 20, scalar + 10.seconds
+    assert_instance_of ActiveSupport::Duration, scalar + 10.seconds
+
+    exception = assert_raises(TypeError) do
+      scalar + "foo"
+    end
+
+    assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message
+  end
+
+  def test_scalar_minus
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+
+    assert_equal 10, 20 - scalar
+    assert_instance_of ActiveSupport::Duration::Scalar, 20 - scalar
+    assert_equal 5, scalar - 5
+    assert_instance_of ActiveSupport::Duration::Scalar, scalar - 5
+    assert_equal 10, 20.seconds - scalar
+    assert_instance_of ActiveSupport::Duration, 20.seconds - scalar
+    assert_equal 5, scalar - 5.seconds
+    assert_instance_of ActiveSupport::Duration, scalar - 5.seconds
+
+    exception = assert_raises(TypeError) do
+      scalar - "foo"
+    end
+
+    assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message
+  end
+
+  def test_scalar_multiply
+    scalar = ActiveSupport::Duration::Scalar.new(5)
+
+    assert_equal 10, 2 * scalar
+    assert_instance_of ActiveSupport::Duration::Scalar, 2 * scalar
+    assert_equal 10, scalar * 2
+    assert_instance_of ActiveSupport::Duration::Scalar, scalar * 2
+    assert_equal 10, 2.seconds * scalar
+    assert_instance_of ActiveSupport::Duration, 2.seconds * scalar
+    assert_equal 10, scalar * 2.seconds
+    assert_instance_of ActiveSupport::Duration, scalar * 2.seconds
+
+    exception = assert_raises(TypeError) do
+      scalar * "foo"
     end
+
+    assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message
+  end
+
+  def test_scalar_divide
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+
+    assert_equal 10, 100 / scalar
+    assert_instance_of ActiveSupport::Duration::Scalar, 100 / scalar
+    assert_equal 5, scalar / 2
+    assert_instance_of ActiveSupport::Duration::Scalar, scalar / 2
+    assert_equal 10, 100.seconds / scalar
+    assert_instance_of ActiveSupport::Duration, 2.seconds * scalar
+    assert_equal 5, scalar / 2.seconds
+    assert_instance_of ActiveSupport::Duration, scalar / 2.seconds
+
+    exception = assert_raises(TypeError) do
+      scalar / "foo"
+    end
+
+    assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message
   end
 
   def test_twelve_months_equals_one_year