Add duration methods to Numeric

[grncdr]

See: #137

Type of Change

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

Changes

• Gem name: activesupport
• Gem version:
• Gem source:
  https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/numeric/time.rb#L9
  https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/integer/time.rb
• Gem API doc: https://api.rubyonrails.org/classes/Numeric.html#method-i-seconds (similar for other methods).
• Tapioca version: 0.11.6
• Sorbet version:

[andyw8]

I expect it's not commonly used, but let's include fortnight and fortnights for completeness?

And also in_milliseconds.

[KaanOzkan]

I think adding active_support/core_ext/numeric/time to here will fix CI

[grncdr]

Signed the CLA

[KaanOzkan]

The failing methods don't exist on Numeric so they'd have to be removed.

[andyw8]

🤔 Seem they are instead defined here? https://github.com/rails/rails/blob/main/activesupport/lib/active_support/duration.rb

[grncdr]

🤔 Seem they are instead defined here? https://github.com/rails/rails/blob/main/activesupport/lib/active_support/duration.rb

That's the definition of ActiveSupport::Duration. The failing methods are defined in https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/integer/time.rb

I originally added all the definitions on Integer, found the definitions on Numeric when filling out the PR template, and now I realize that they're split like this because a partial year/month doesn't make sense (what is Date.today + 3.14.months?).

Fixing it up now.

[grncdr]

I think I've got it right now. Thanks for your patience everyone.

[KaanOzkan]

I'm conflicted about this PR because it'll cause a lot of typecheck errors due to the delegation happening in ActiveSupport::Duration. I'm not sure if it'll bring in enough value. Applications are using Duration in place of Numeric.

For example, a common issue now would be while doing operations:
Time.now - 1.second errors with Expected Time but found ActiveSupport::Duration for argument arg0 due to sorbet's internal RBIs.

ActiveSupport::Duration is the correct type for 1.second however it can be used in place of Numeric and it's hard to tell Sorbet that. We'll have to be able to override the internal RBI. Looks like sorbet-typed went this route before but I'm not sure how it was working for the above example.

[grncdr]

Hm, those are good points.

I'm conflicted about this PR because it'll cause a lot of typecheck errors due to the delegation happening in ActiveSupport::Duration. I'm not sure if it'll bring in enough value. Applications are using Duration in place of Numeric.

I think this one is a bit less of an issue, as it effects expressions where a duration is the receiver (like 1.second + 1.second) which are less common and/or can be handled by adding more methods to the RBI for ActiveSupport::Duration. (which appears to be what sorbet-typed did before (https://github.com/sorbet/sorbet-typed/blob/440aa126c55570ad35cb5a5d4fe4a392f23d65d3/lib/activesupport/all/activesupport.rbi#L1693)

For example, a common issue now would be while doing operations: Time.now - 1.second errors with Expected Time but found ActiveSupport::Duration for argument arg0 due to sorbet's internal RBIs.

This is definitely annoying. It's also annoying to not be able to write 1.second.ago. I am still pretty new to Sorbet and it's limitations/tradeoffs. Is it technically possible to override the type of Time#-(arg0) (and friends) to make the type of arg0 a union of Numeric and ActiveSupport::Duration?

ActiveSupport::Duration is the correct type for 1.second however it can be used in place of Numeric and it's hard to tell Sorbet that. We'll have to be able to override the internal RBI. Looks like sorbet-typed went this route before but I'm not sure how it was working for the above example.

It looks like there were overrides: https://github.com/sorbet/sorbet-typed/blob/440aa126c55570ad35cb5a5d4fe4a392f23d65d3/lib/activesupport/all/activesupport.rbi#L1252-L1266

(again, very new to Sorbet here, and it seems like the story around overrides might have changed at some point)

[KaanOzkan]

It looks like there were overrides: https://github.com/sorbet/sorbet-typed/blob/440aa126c55570ad35cb5a5d4fe4a392f23d65d3/lib/activesupport/all/activesupport.rbi#L1252-L1266

Yes but I'm unable to make them work in our monolith. The overrides aren't picked up by Sorbet. I will be discussing this with Stripe and see if sorbet-typed is doing something we are not. Lmk if you have other findings in your application.

Even with the overrides I'm not convinced that typing these as Duration has practical typing benefits since we can't represent that delegation yet 😕

It's also annoying to not be able to write 1.second.ago

Yeah currently the return type for second is coming from Tapioca generated gem RBIs for activesupport which is T.untyped. That's how we're able to write 1.second.ago. I'd love to represent this delegation and add type safety to those call sites.

[amomchilov]

since we can't represent that delegation yet

Can we just hand-roll RBI for the methods we know are commonly being delegated?

[KaanOzkan]

Not really. We can't add Duration to sorbet's default RBIs so we'd have to return a Numeric from the methods in this PR which isn't correct.

[ipvalverde]

Any updates on this one? We've been getting some annoying behaviour when we have something like the following:

one_day_ago = Time.now.utc - 1.day
# one_day_ago is inferred to be a `Float`

[KaanOzkan]

I was able to make overloads for stdlib methods like Time#- work so we'll able to cover most of the practical use cases by adding annotations for each of those methods in this file. However, there haven't been any changes to Sorbet regarding delegation so we still can't represent ActiveSupport::Duration accurately, i.e. 1.second.digits will fail with a method digits does not exist o nActiveSupport::Duration error. So when using methods that are called on the underlying type (Integer in this case) one would still need T.cast.