Freeze ActiveSupport::Duration#parts hash

Durations are meant to be value objects and should not be mutated.

activesupport/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Freeze `ActiveSupport::Duration#parts` and remove writer methods.
+
+    Durations are meant to be value objects and should not be mutated.
+
+    *Andrew White*
+
 *   Fix `ActiveSupport::TimeZone#utc_to_local` with fractional seconds.
 
     When `utc_to_local_returns_utc_offset_times` is false and the time

activesupport/lib/active_support/duration.rb

@@ -39,11 +39,11 @@ module ActiveSupport
 
       def +(other)
         if Duration === other
-          seconds   = value + other.parts.fetch(:seconds, 0)
-          new_parts = other.parts.merge(seconds: seconds)
+          seconds   = value + other._parts.fetch(:seconds, 0)
+          new_parts = other._parts.merge(seconds: seconds)
           new_value = value + other.value
 
-          Duration.new(new_value, new_parts)
+          Duration.new(new_value, new_parts, other.variable?)
         else
           calculate(:+, other)
         end
@@ -51,12 +51,12 @@ module ActiveSupport
 
       def -(other)
         if Duration === other
-          seconds   = value - other.parts.fetch(:seconds, 0)
-          new_parts = other.parts.transform_values(&:-@)
+          seconds   = value - other._parts.fetch(:seconds, 0)
+          new_parts = other._parts.transform_values(&:-@)
           new_parts = new_parts.merge(seconds: seconds)
           new_value = value - other.value
 
-          Duration.new(new_value, new_parts)
+          Duration.new(new_value, new_parts, other.variable?)
         else
           calculate(:-, other)
         end
@@ -64,10 +64,10 @@ module ActiveSupport
 
       def *(other)
         if Duration === other
-          new_parts = other.parts.transform_values { |other_value| value * other_value }
+          new_parts = other._parts.transform_values { |other_value| value * other_value }
           new_value = value * other.value
 
-          Duration.new(new_value, new_parts)
+          Duration.new(new_value, new_parts, other.variable?)
         else
           calculate(:*, other)
         end
@@ -89,6 +89,10 @@ module ActiveSupport
         end
       end
 
+      def variable? #:nodoc:
+        false
+      end
+
       private
         def calculate(op, other)
           if Scalar === other
@@ -123,8 +127,9 @@ module ActiveSupport
     }.freeze
 
     PARTS = [:years, :months, :weeks, :days, :hours, :minutes, :seconds].freeze
+    VARIABLE_PARTS = [:years, :months, :weeks, :days].freeze
 
-    attr_accessor :value, :parts
+    attr_reader :value
 
     autoload :ISO8601Parser,     "active_support/duration/iso8601_parser"
     autoload :ISO8601Serializer, "active_support/duration/iso8601_serializer"
@@ -147,31 +152,31 @@ module ActiveSupport
       end
 
       def seconds(value) #:nodoc:
-        new(value, seconds: value)
+        new(value, { seconds: value }, false)
       end
 
       def minutes(value) #:nodoc:
-        new(value * SECONDS_PER_MINUTE, minutes: value)
+        new(value * SECONDS_PER_MINUTE, { minutes: value }, false)
       end
 
       def hours(value) #:nodoc:
-        new(value * SECONDS_PER_HOUR, hours: value)
+        new(value * SECONDS_PER_HOUR, { hours: value }, false)
       end
 
       def days(value) #:nodoc:
-        new(value * SECONDS_PER_DAY, days: value)
+        new(value * SECONDS_PER_DAY, { days: value }, true)
       end
 
       def weeks(value) #:nodoc:
-        new(value * SECONDS_PER_WEEK, weeks: value)
+        new(value * SECONDS_PER_WEEK, { weeks: value }, true)
       end
 
       def months(value) #:nodoc:
-        new(value * SECONDS_PER_MONTH, months: value)
+        new(value * SECONDS_PER_MONTH, { months: value }, true)
       end
 
       def years(value) #:nodoc:
-        new(value * SECONDS_PER_YEAR, years: value)
+        new(value * SECONDS_PER_YEAR, { years: value }, true)
       end
 
       # Creates a new Duration from a seconds value that is converted
@@ -187,18 +192,23 @@ module ActiveSupport
 
         parts = {}
         remainder = value.round(9)
+        variable = false
 
         PARTS.each do |part|
           unless part == :seconds
             part_in_seconds = PARTS_IN_SECONDS[part]
             parts[part] = remainder.div(part_in_seconds)
             remainder %= part_in_seconds
+
+            unless parts[part].zero?
+              variable ||= VARIABLE_PARTS.include?(part)
+            end
           end
         end unless value == 0
 
         parts[:seconds] = remainder
 
-        new(value, parts)
+        new(value, parts, variable)
       end
 
       private
@@ -209,9 +219,20 @@ module ActiveSupport
         end
     end
 
-    def initialize(value, parts) #:nodoc:
+    def initialize(value, parts, variable = nil) #:nodoc:
       @value, @parts = value, parts
       @parts.reject! { |k, v| v.zero? } unless value == 0
+      @parts.freeze
+      @variable = variable
+
+      if @variable.nil?
+        @variable = @parts.any? { |part, _| VARIABLE_PARTS.include?(part) }
+      end
+    end
+
+    # Returns a copy of the parts hash that defines the duration
+    def parts
+      @parts.dup
     end
 
     def coerce(other) #:nodoc:
@@ -239,13 +260,13 @@ module ActiveSupport
     # are treated as seconds.
     def +(other)
       if Duration === other
-        parts = @parts.merge(other.parts) do |_key, value, other_value|
+        parts = @parts.merge(other._parts) do |_key, value, other_value|
           value + other_value
         end
-        Duration.new(value + other.value, parts)
+        Duration.new(value + other.value, parts, @variable || other.variable?)
       else
         seconds = @parts.fetch(:seconds, 0) + other
-        Duration.new(value + other, @parts.merge(seconds: seconds))
+        Duration.new(value + other, @parts.merge(seconds: seconds), @variable)
       end
     end
 
@@ -258,9 +279,9 @@ module ActiveSupport
     # Multiplies this Duration by a Numeric and returns a new Duration.
     def *(other)
       if Scalar === other || Duration === other
-        Duration.new(value * other.value, parts.transform_values { |number| number * other.value })
+        Duration.new(value * other.value, @parts.transform_values { |number| number * other.value }, @variable || other.variable?)
       elsif Numeric === other
-        Duration.new(value * other, parts.transform_values { |number| number * other })
+        Duration.new(value * other, @parts.transform_values { |number| number * other }, @variable)
       else
         raise_type_error(other)
       end
@@ -269,11 +290,11 @@ module ActiveSupport
     # Divides this Duration by a Numeric and returns a new Duration.
     def /(other)
       if Scalar === other
-        Duration.new(value / other.value, parts.transform_values { |number| number / other.value })
+        Duration.new(value / other.value, @parts.transform_values { |number| number / other.value }, @variable)
       elsif Duration === other
         value / other.value
       elsif Numeric === other
-        Duration.new(value / other, parts.transform_values { |number| number / other })
+        Duration.new(value / other, @parts.transform_values { |number| number / other }, @variable)
       else
         raise_type_error(other)
       end
@@ -292,7 +313,7 @@ module ActiveSupport
     end
 
     def -@ #:nodoc:
-      Duration.new(-value, parts.transform_values(&:-@))
+      Duration.new(-value, @parts.transform_values(&:-@), @variable)
     end
 
     def +@ #:nodoc:
@@ -420,9 +441,9 @@ module ActiveSupport
     alias :before :ago
 
     def inspect #:nodoc:
-      return "#{value} seconds" if parts.empty?
+      return "#{value} seconds" if @parts.empty?
 
-      parts.
+      @parts.
         sort_by { |unit,  _ | PARTS.index(unit) }.
         map     { |unit, val| "#{val} #{val == 1 ? unit.to_s.chop : unit.to_s}" }.
         to_sentence(locale: ::I18n.default_locale)
@@ -446,16 +467,24 @@ module ActiveSupport
       ISO8601Serializer.new(self, precision: precision).serialize
     end
 
+    def variable? #:nodoc:
+      @variable
+    end
+
+    def _parts #:nodoc:
+      @parts
+    end
+
     private
       def sum(sign, time = ::Time.current)
         unless time.acts_like?(:time) || time.acts_like?(:date)
           raise ::ArgumentError, "expected a time or date, got #{time.inspect}"
         end
 
-        if parts.empty?
+        if @parts.empty?
           time.since(sign * value)
         else
-          parts.inject(time) do |t, (type, number)|
+          @parts.inject(time) do |t, (type, number)|
             if type == :seconds
               t.since(sign * number)
             elsif type == :minutes

activesupport/lib/active_support/time_with_zone.rb

@@ -576,7 +576,7 @@ module ActiveSupport
       end
 
       def duration_of_variable_length?(obj)
-        ActiveSupport::Duration === obj && obj.parts.any? { |p| [:years, :months, :weeks, :days].include?(p[0]) }
+        ActiveSupport::Duration === obj && obj.variable?
       end
 
       def wrap_with_time_zone(time)

activesupport/test/core_ext/duration_test.rb

@@ -734,6 +734,22 @@ class DurationTest < ActiveSupport::TestCase
     assert_equal "can't build an ActiveSupport::Duration from a NilClass", error.message
   end
 
+  def test_variable
+    assert_not 12.seconds.variable?
+    assert_not 12.minutes.variable?
+    assert_not 12.hours.variable?
+
+    assert 12.days.variable?
+    assert 12.weeks.variable?
+    assert 12.months.variable?
+    assert 12.years.variable?
+
+    assert_not (12.hours + 12.minutes).variable?
+
+    assert (12.hours + 1.day).variable?
+    assert (1.day + 12.hours).variable?
+  end
+
   private
     def eastern_time_zone
       if Gem.win_platform?