Make getlocal and getutc always return instances of Time

Previously these methods could return either a DateTime or a Time
depending on how the ActiveSupport::TimeWithZone instance had
been constructed. Changing to always return an instance of Time
eliminates a possible stack level too deep error in to_time where
it was wrapping a DateTime instance.

As a consequence of this the internal time value is now always an
instance of Time in the UTC timezone, whether that's as the UTC
time directly or a representation of the local time in the timezone.

There should be no consequences of this internal change and if
there are it's a bug due to leaky abstractions.

6 files changed, 71 insertions, 23 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 8908247287..45b6de55d1 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,15 @@
+*   Make `getlocal` and `getutc` always return instances of `Time` for
+    `ActiveSupport::TimeWithZone` and `DateTime`. This eliminates a possible
+    stack level too deep error in `to_time` where `ActiveSupport::TimeWithZone`
+    was wrapping a `DateTime` instance. As a consequence of this the internal
+    time value in `ActiveSupport::TimeWithZone` is now always an instance of
+    `Time` in the UTC timezone, whether that's as the UTC time directly or
+    a representation of the local time in the timezone. There should be no
+    consequences of this internal change and if there are it's a bug due to
+    leaky abstractions.
+
+    *Andrew White*
+
 *   Add `DateTime#subsec` to return the fraction of a second as a `Rational`.
 
     *Andrew White*
diff --git a/activesupport/lib/active_support/core_ext/date_time/calculations.rb b/activesupport/lib/active_support/core_ext/date_time/calculations.rb
index 7e214524df..9e89a33491 100644
--- a/activesupport/lib/active_support/core_ext/date_time/calculations.rb
+++ b/activesupport/lib/active_support/core_ext/date_time/calculations.rb
@@ -150,21 +150,9 @@ class DateTime
   end
   alias :at_end_of_minute :end_of_minute
 
-  # Adjusts DateTime to UTC by adding its offset value; offset is set to 0.
-  #
-  #   DateTime.civil(2005, 2, 21, 10, 11, 12, Rational(-6, 24))     # => Mon, 21 Feb 2005 10:11:12 -0600
-  #   DateTime.civil(2005, 2, 21, 10, 11, 12, Rational(-6, 24)).utc # => Mon, 21 Feb 2005 16:11:12 +0000
-  def utc
-    new_offset(0)
-  end
-  alias_method :getgm, :utc
-  alias_method :getutc, :utc
-  alias_method :gmtime, :utc
-
-  # Returns a <tt>Time.local()</tt> instance of the simultaneous time in your
-  # system's <tt>ENV['TZ']</tt> zone.
+  # Returns a <tt>Time</tt> instance of the simultaneous time in the system timezone.
   def localtime(utc_offset = nil)
-    utc = getutc
+    utc = new_offset(0)
 
     Time.utc(
       utc.year, utc.month, utc.day,
@@ -173,6 +161,22 @@ class DateTime
   end
   alias_method :getlocal, :localtime
 
+  # Returns a <tt>Time</tt> instance of the simultaneous time in the UTC timezone.
+  #
+  #   DateTime.civil(2005, 2, 21, 10, 11, 12, Rational(-6, 24))     # => Mon, 21 Feb 2005 10:11:12 -0600
+  #   DateTime.civil(2005, 2, 21, 10, 11, 12, Rational(-6, 24)).utc # => Mon, 21 Feb 2005 16:11:12 UTC
+  def utc
+    utc = new_offset(0)
+
+    Time.utc(
+      utc.year, utc.month, utc.day,
+      utc.hour, utc.min, utc.sec + utc.sec_fraction
+    )
+  end
+  alias_method :getgm, :utc
+  alias_method :getutc, :utc
+  alias_method :gmtime, :utc
+
   # Returns +true+ if <tt>offset == 0</tt>.
   def utc?
     offset == 0
diff --git a/activesupport/lib/active_support/time_with_zone.rb b/activesupport/lib/active_support/time_with_zone.rb
index a44041af82..b1cec43124 100644
--- a/activesupport/lib/active_support/time_with_zone.rb
+++ b/activesupport/lib/active_support/time_with_zone.rb
@@ -49,16 +49,17 @@ module ActiveSupport
     attr_reader :time_zone
 
     def initialize(utc_time, time_zone, local_time = nil, period = nil)
-      @utc, @time_zone, @time = utc_time, time_zone, local_time
+      @utc = utc_time ? transfer_time_values_to_utc_constructor(utc_time) : nil
+      @time_zone, @time = time_zone, local_time
       @period = @utc ? period : get_period_and_ensure_valid_local_time(period)
     end
 
-    # Returns a Time or DateTime instance that represents the time in +time_zone+.
+    # Returns a <tt>Time</tt> instance that represents the time in +time_zone+.
     def time
       @time ||= period.to_local(@utc)
     end
 
-    # Returns a Time or DateTime instance that represents the time in UTC.
+    # Returns a <tt>Time</tt> instance of the simultaneous time in the UTC timezone.
     def utc
       @utc ||= period.to_utc(@time)
     end
@@ -78,10 +79,9 @@ module ActiveSupport
       utc.in_time_zone(new_zone)
     end
 
-    # Returns a <tt>Time.local()</tt> instance of the simultaneous time in your
-    # system's <tt>ENV['TZ']</tt> zone.
+    # Returns a <tt>Time</tt> instance of the simultaneous time in the system timezone.
     def localtime(utc_offset = nil)
-      utc.respond_to?(:getlocal) ? utc.getlocal(utc_offset) : utc.to_time.getlocal(utc_offset)
+      utc.getlocal(utc_offset)
     end
     alias_method :getlocal, :localtime
 
@@ -450,7 +450,6 @@ module ActiveSupport
     # Ensure proxy class responds to all methods that underlying time instance
     # responds to.
     def respond_to_missing?(sym, include_priv)
-      # consistently respond false to acts_like?(:date), regardless of whether #time is a Time or DateTime
       return false if sym.to_sym == :acts_like_date?
       time.respond_to?(sym, include_priv)
     end
@@ -478,7 +477,7 @@ module ActiveSupport
       end
 
       def transfer_time_values_to_utc_constructor(time)
-        ::Time.utc(time.year, time.month, time.day, time.hour, time.min, time.sec, Rational(time.nsec, 1000))
+        ::Time.utc(time.year, time.month, time.day, time.hour, time.min, time.sec + time.subsec)
       end
 
       def duration_of_variable_length?(obj)
diff --git a/activesupport/test/core_ext/date_and_time_compatibility_test.rb b/activesupport/test/core_ext/date_and_time_compatibility_test.rb
index 7cc2fae5be..11cb1469da 100644
--- a/activesupport/test/core_ext/date_and_time_compatibility_test.rb
+++ b/activesupport/test/core_ext/date_and_time_compatibility_test.rb
@@ -7,6 +7,7 @@ class DateAndTimeCompatibilityTest < ActiveSupport::TestCase
 
   def setup
     @utc_time = Time.utc(2016, 4, 23, 14, 11, 12)
+    @date_time = DateTime.new(2016, 4, 23, 14, 11, 12, 0)
     @utc_offset = 3600
     @system_offset = -14400
     @zone = ActiveSupport::TimeZone['London']
@@ -67,6 +68,14 @@ class DateAndTimeCompatibilityTest < ActiveSupport::TestCase
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
+        assert_instance_of Time, time.getutc
+        assert_equal @utc_offset, time.utc_offset
+
+        time = ActiveSupport::TimeWithZone.new(@date_time, @zone).to_time
+
+        assert_instance_of Time, time
+        assert_equal @date_time, time.getutc
+        assert_instance_of Time, time.getutc
         assert_equal @utc_offset, time.utc_offset
       end
     end
@@ -79,6 +88,14 @@ class DateAndTimeCompatibilityTest < ActiveSupport::TestCase
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
+        assert_instance_of Time, time.getutc
+        assert_equal @system_offset, time.utc_offset
+
+        time = ActiveSupport::TimeWithZone.new(@date_time, @zone).to_time
+
+        assert_instance_of Time, time
+        assert_equal @date_time, time.getutc
+        assert_instance_of Time, time.getutc
         assert_equal @system_offset, time.utc_offset
       end
     end
diff --git a/activesupport/test/core_ext/date_time_ext_test.rb b/activesupport/test/core_ext/date_time_ext_test.rb
index c19992553f..fcd739c804 100644
--- a/activesupport/test/core_ext/date_time_ext_test.rb
+++ b/activesupport/test/core_ext/date_time_ext_test.rb
@@ -40,8 +40,18 @@ class DateTimeExtCalculationsTest < ActiveSupport::TestCase
     Time::DATE_FORMATS.delete(:custom)
   end
 
+  def test_localtime
+    with_env_tz 'US/Eastern' do
+      assert_instance_of Time, DateTime.new(2016, 3, 11, 15, 11, 12, 0).localtime
+      assert_equal Time.local(2016, 3, 11, 10, 11, 12), DateTime.new(2016, 3, 11, 15, 11, 12, 0).localtime
+      assert_equal Time.local(2016, 3, 21, 11, 11, 12), DateTime.new(2016, 3, 21, 15, 11, 12, 0).localtime
+      assert_equal Time.local(2016, 4, 1, 11, 11, 12), DateTime.new(2016, 4, 1, 16, 11, 12, Rational(1,24)).localtime
+    end
+  end
+
   def test_getlocal
     with_env_tz 'US/Eastern' do
+      assert_instance_of Time, DateTime.new(2016, 3, 11, 15, 11, 12, 0).getlocal
       assert_equal Time.local(2016, 3, 11, 10, 11, 12), DateTime.new(2016, 3, 11, 15, 11, 12, 0).getlocal
       assert_equal Time.local(2016, 3, 21, 11, 11, 12), DateTime.new(2016, 3, 21, 15, 11, 12, 0).getlocal
       assert_equal Time.local(2016, 4, 1, 11, 11, 12), DateTime.new(2016, 4, 1, 16, 11, 12, Rational(1,24)).getlocal
@@ -58,7 +68,7 @@ class DateTimeExtCalculationsTest < ActiveSupport::TestCase
 
   def test_to_time
     with_env_tz 'US/Eastern' do
-      assert_equal Time, DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time.class
+      assert_instance_of Time, DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time
       assert_equal Time.local(2005, 2, 21, 5, 11, 12), DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time
       assert_equal Time.local(2005, 2, 21, 5, 11, 12).utc_offset, DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time.utc_offset
     end
@@ -318,6 +328,7 @@ class DateTimeExtCalculationsTest < ActiveSupport::TestCase
   end
 
   def test_utc
+    assert_instance_of Time, DateTime.civil(2005, 2, 21, 10, 11, 12, Rational(-6, 24)).utc
     assert_equal DateTime.civil(2005, 2, 21, 16, 11, 12, 0), DateTime.civil(2005, 2, 21, 10, 11, 12, Rational(-6, 24)).utc
     assert_equal DateTime.civil(2005, 2, 21, 15, 11, 12, 0), DateTime.civil(2005, 2, 21, 10, 11, 12, Rational(-5, 24)).utc
     assert_equal DateTime.civil(2005, 2, 21, 10, 11, 12, 0), DateTime.civil(2005, 2, 21, 10, 11, 12, 0).utc
diff --git a/activesupport/test/core_ext/time_with_zone_test.rb b/activesupport/test/core_ext/time_with_zone_test.rb
index 7acada011d..d90714acdb 100644
--- a/activesupport/test/core_ext/time_with_zone_test.rb
+++ b/activesupport/test/core_ext/time_with_zone_test.rb
@@ -11,10 +11,13 @@ class TimeWithZoneTest < ActiveSupport::TestCase
     @utc = Time.utc(2000, 1, 1, 0)
     @time_zone = ActiveSupport::TimeZone['Eastern Time (US & Canada)']
     @twz = ActiveSupport::TimeWithZone.new(@utc, @time_zone)
+    @dt_twz = ActiveSupport::TimeWithZone.new(@utc.to_datetime, @time_zone)
   end
 
   def test_utc
     assert_equal @utc, @twz.utc
+    assert_instance_of Time, @twz.utc
+    assert_instance_of Time, @dt_twz.utc
   end
 
   def test_time
@@ -47,6 +50,8 @@ class TimeWithZoneTest < ActiveSupport::TestCase
 
   def test_localtime
     assert_equal @twz.localtime, @twz.utc.getlocal
+    assert_instance_of Time, @twz.localtime
+    assert_instance_of Time, @dt_twz.localtime
   end
 
   def test_utc?