Add ActiveSupport signatures for time methods

[olivier-thatch]

Type of Change

• Add RBI for a new gem
• Modify RBI for an existing gem
• Other:

Changes

• Gem name: activesupport
• Gem version: 8.0.1 (but should be compatible with previous versions)
• Gem source: https://github.com/rails/rails/tree/main/activesupport
• Gem API doc: https://api.rubyonrails.org/classes/ActiveSupport.html
• Tapioca version: Tapioca v0.16.9
• Sorbet version: Sorbet typechecker 0.5.11813 git e125bae4c662972686885a21e05d0df7b27d8640 debug_symbols=true clean=1

This PR adds a bunch of signatures for time-related ActiveSupport methods. I wrote these signatures almost a year ago and we've been using them in our codebase without issues, so I'm fairly confident that they are correct.

[KaanOzkan]

Thanks! I think this is similar to this PR where I did some testing and left comments around usability #166 (comment). Does it apply? Curious what your experience has been.

[olivier-thatch]

This hasn't been an issue for us in practice -- I don't think we've often needed to call methods delegated to the underlying numeric value on ActiveSupport::Duration instances or the underlying Time value on ActiveSupport::TimeWithZone instances.

For ActiveSupport::Duration, I would advise using #to_i (or #to_f, #to_r, etc.), e.g. 1.second.to_i.digits. This plays nicely with Sorbet and is still very readable (and maybe even preferable IMHO).

For ActiveSupport::TimeWithZone, it's a little trickier because the return value of methods delegated to the underlying Time instance is wrapped back into an ActiveSupport::TimeWithZone instance. So there is a difference between Time.zone.now.floor (which returns an ActiveSupport::TimeWithZone) and Time.zone.now.to_time.floor (which returns a Time).

I suppose we could add signatures for delegated methods with @method_missing? There aren't that many methods on Time and a bunch of them are explicitly defined on ActiveSupport::TimeWithZone and already have signatures.

[KaanOzkan]

Thanks. Similar to my other comment I'm wary about the toll this would introduce to large codebases. In our monolith we already can't enable this annotation due to high number of errors and this doubles it (~2k). Unfortunately users don't have a way of being more selective so if they run into too many issues they'll mark it as "ignore" and move on.

Right now we can't express the runtime delegation with annotations so I'm leaning towards these annotations being too premature.

suppose we could add signatures for delegated methods with @method_missing? There aren't that many methods on Time and a bunch of them are explicitly defined on ActiveSupport::TimeWithZone and already have signatures.

It's possible, although it's another hard coded list of methods to maintain since we can't express delegation. How would it look in practice? I'm seeing errors such as Expected Time but found ActiveSupport::TimeWithZone due to the signature on ago for example. Would you just change the return type to be Time? I don't know if that can lead to other issues or not.

[olivier-thatch]

That's fair. As is often the case with Rails, the very dynamic behavior of those classes is hard or impossible to properly codify as Sorbet signatures 😞

Going on a bit of a tangent here, but would Tapioca annotations benefit from more granularity? My understanding of the current behavior is: if a gem is present in a project's Gemfile and there are annotations for this gem in the rbi-central repo, then the annotations are pulled into sorbet/rbi/annotations.

Do you think it would be useful to have the ability of have more than one set of annotations per gem, and for each set to come with a description and recommendations? E.g. ActiveSupport could have:

• a default set that's safe for all codebases and is just installed, no questions asked
• a separate set for time-related methods that might cause errors in existing codebases and has to be opted in
• etc.

Ideally Tapioca would be smart enough to try applying each set separately and report if any Sorbet type errors are raised when the signatures from the set are included.

Such a feature would make the annotations system significantly more complex, but the current state of things is kind of a bummer: we can't add these signatures to the rbi-central repo because they risk breaking existing codebases, so users are left to either forego types or spend time writing signatures themselves directly in their own codebase like I did.

[KaanOzkan]

Yes I think from the user's perspective it'd be good to have for the reasons you listed.

[bdewater-thatch]

https://github.com/Shopify/tapioca/blob/main/lib/tapioca/dsl/compilers/active_support_time_ext.rb already narrows this down btw, I think this signature should be removed completely