Updated Time annotations breaks `model.updated_at = Time.current`

[jshirley]

Problem

We started to get errors when typechecking, tracing back to https://github.com/Shopify/rbi-central/pull/259 and want to ensure that either we're not using the annotations wrong, or the side effects here are intended.

Context

We have methods on some models that aren't complex, and set some fields and related timestamps. ActiveRecord expects the fields (e.g., updated_at to be T.nilable(ActiveSupport::TimeWithZone)):

This is the generated rbi for that model:

    sig { returns(T.nilable(::ActiveSupport::TimeWithZone)) }
    def updated_at; end

    sig { params(value: ::ActiveSupport::TimeWithZone).returns(::ActiveSupport::TimeWithZone) }
    def updated_at=(value); end

The old method looked something like this (other fields removed):

  sig { params(timestamp: ActiveSupport::TimeWithZone).void }
  def update_timestamps_v1(timestamp = Time.current)
    self.updated_at = timestamp
    save
  end

Prior to this update, this worked and passed typechecking, now:

app/models/example_model.rb:64: Argument does not have asserted type ActiveSupport::TimeWithZone https://srb.help/7007
    64 |  def update_timestamps_v1(timestamp = Time.current)
                                               ^^^^^^^^^^^^
  Got T.any(ActiveSupport::TimeWithZone, Time) originating from:
    app/models/delivery_attempt.rb:64:
    64 |  def update_timestamps_v1(timestamp = Time.current)
                                               ^^^^^^^^^^^^

app/models/example_model.rb:65: Assigning a value to value that does not match expected type ActiveSupport::TimeWithZone https://srb.help/7002
    65 |    self.updated_at = timestamp
                              ^^^^^^^^^
  Got T.any(ActiveSupport::TimeWithZone, Time) originating from:
    app/models/delivery_attempt.rb:64:
    64 |  def update_timestamps_v1(timestamp = Time.current)
                                   ^^^^^^^^^
    app/models/delivery_attempt.rb:64:
    64 |  def update_timestamps_v1(timestamp = Time.current)

If I update the method with the T.any(...) and cast to what ActiveRecord is expecting, it will pass typecheck:

  sig { params(timestamp: T.any(ActiveSupport::TimeWithZone, Time)).void }
  def update_timestamps_v2(timestamp = Time.current)
    self.updated_at = T.cast(timestamp, ActiveSupport::TimeWithZone)
    save
  end

This requires a bit of boilerplate, but it feels awkward that self.updated_at = Time.current now throws a typecheck error.

It can be reproduced in a Rails app with generating a new model (e.g., bin/rails g model Example) with the following contents:

# typed: strict
# frozen_string_literal: true

class Example < ApplicationRecord
  extend T::Sig

  sig { void }
  def update_timestamp
    self.updated_at = Time.current
  end
end

After generating rbi and running srb tc it fails typechecking.

Thank you, and happy to help further debug this!

• Gem name: activesupport/annotations
• Gem version: N/A
• Gem source: #259
• Gem API doc: N/A
• Tapioca version: tapioca@0.15.0.rbi
• Sorbet version: Sorbet typechecker 0.5.11218 git a4457bfc25b26f8953eef8e529ec5a7488f2e1ac debug_symbols=true clean=1

[KaanOzkan]

Thanks for the report. It might be better to relax this constraint for better DX. I think config.time_zone is set by default. If 99% of the applications will have it set I think it's better to change the type to ActiveSupport::TimeWithZone.

@ipvalverde thoughts?

[ipvalverde]

Thanks for raising this! Happy to have it changed to a ActiveReport::TimeWithZone to quickly fix this.

It works in core because we added the following shim:

class ActiveSupport::TimeWithZone < Time; end

Would it make sense to add something like that the rbi central repo as well? It is a sensible change given TimeWithZone behaves like a Time object.

[KaanOzkan]

Would it make sense to add something like that the rbi central repo as well? It is a sensible change given TimeWithZone behaves like a Time object.

@ipvalverde Yeah I'm okay with that too.

[ipvalverde]

Up for review: https://github.com/Shopify/rbi-central/pull/262