Deprecate implicit convertion of Duration

[skipkayhil]

Summary

The presence of method_missing on Duration can lead to some very confusing behavior:

1.year.days # => 31556952 Days

Methods to convert from duration to a specific unit were added in 7bd9603778c059d8f0969feaadb26672534dbece and a2535d9fe91f58aafc7206a8a51dd29a06ecbf61, but the confusing methods they were meant to replace were left in.

This change fixes this by deprecating method_missing and reimplementing or explicitly delegating methods that make sense to keep.

In addition, there are some other methods on Duration like * that result in unintuitive behavior. Specific cases in those methods (Duration * Duration) have also been deprecated.

1.day * 2.days # => 172800 days

Fixes #45433

[matthewd]

These methods are the most egregiously confusing, but I wonder if we should go further, and deprecate the method_missing entirely, carving out an explicit delegation for any specific methods we think are worth keeping. I'm inclined to claim that any time a duration silently acts like an integer is similarly surprising.

[skipkayhil]

I had that thought as well. Just running Active Support tests the following methods are used:

abs is used when finding time zones, but it could easily be rewritten to not conflate Numeric and Duration: https://github.com/rails/rails/blob/b55e8347485e3d8d75b3b5901c2faa3e05c4ebe3/activesupport/lib/active_support/values/time_zone.rb#L242-L245

in_milliseconds is defined on the numeric/time core_ext (should it be moved into Duration?): https://github.com/rails/rails/blob/b55e8347485e3d8d75b3b5901c2faa3e05c4ebe3/activesupport/lib/active_support/core_ext/numeric/time.rb#L63-L65

to_f is used to normalize the expires_at param in the cache: https://github.com/rails/rails/blob/b55e8347485e3d8d75b3b5901c2faa3e05c4ebe3/activesupport/lib/active_support/cache.rb#L958

times is one of the weirdest, but its also specifically tested for: https://github.com/rails/rails/blob/b55e8347485e3d8d75b3b5901c2faa3e05c4ebe3/activesupport/test/core_ext/duration_test.rb#L346-L352

zero? is used in calculating message expiry but that's pretty easy to change to use in_seconds: https://github.com/rails/rails/blob/b55e8347485e3d8d75b3b5901c2faa3e05c4ebe3/activesupport/lib/active_support/messages/metadata.rb#L35

but its also tested explicitly: https://github.com/rails/rails/blob/b55e8347485e3d8d75b3b5901c2faa3e05c4ebe3/activesupport/test/core_ext/duration_test.rb#L373

I think in_milliseconds and to_f may be worth keeping with an explicit delegation but the rest are not. How does that sound?

[matthewd]

abs and zero? both sound reasonable to me too: a duration isn't an integer, but is (loosely, though not literally) numeric, so it has the concept of a "zero" value, and of being positive/negative.

That times test just sounds like it's an arbitrary choice of delegated method, not an affirmative expectation that a duration is countable (which goes to the idea that it's the bad sort of method: unlike the above that [can] treat the duration as a self-defining quantity, times is treating it as a count of seconds.

Given it's always been a full delegation, we'll probably need to go through the full list of methods on Integer... there's no historical reason this class would have comprehensive test coverage of its API.

[natematykiewicz]

I spotted this like a month ago and should have done something about it. I don't understand the purpose of the method_missing. I feel like it only causes problems.

https://twitter.com/natematykiewicz/status/1529710207059779586

[natematykiewicz]

Also, I was never able to track down when it was added. All I know is it's at least 15 years old and was added when the repo was SVN. Here's the oldest git commit I could find with the method missing in it.

https://github.com/rails/rails/commit/887870f20c347179aef0545ee2019c02ed9f74d1

[skipkayhil]

@matthewd updated to implement all of the methods we discussed, I still need to write more tests and documentation but figured I'd ask for another round of feedback first

Also, I was never able to track down when it was added. All I know is it's at least 15 years old and was added when the repo was SVN. Here's the oldest git commit I could find with the method missing in it.

887870f

@natematykiewicz It looks to be part of the original implementation when Duration was added in 276c9f29cde80fafa23814b0039f67504255e0fd. My understanding is that since 1.days and friends previously returned integers, method_missing was chosen at the time to reduce the friction of that change.

[natematykiewicz]

@skipkayhil Ahh, that makes sense. But nowadays there's no real expectation that it's numeric, so the method_missing can do a deprecation cycle and eventually get removed. Nice!

[skipkayhil]

Finally got back around to this, updated to add more deprecations and tests for the new methods.

I think *(Duration) is silently treating other as Just A Number, so should probably be deprecated too.

Based on this comment, I thought more about what other operations don't make sense without implicit conversion and added deprecations for the following:

• comparing Durations and Numerics (5 > 2.seconds)
• adding/subtracting Durations and Numerics (5 + 2.seconds)
• dividing a Numeric by a Duration (3 / 2.seconds)
• Numeric % Duration (3 % 2.seconds)
• Duration * Duration (3.seconds * 2.seconds)
• Duration#is_a? returning true for value's class
• Duration#instance_of? returning true for value's class
• equality of Duration and Numeric (1.minute == 60)

Something I'm not entirely sure what to do with is % and related methods (modulo, divmod). % currently always implicitly treats the caller as a number of seconds:

5.minutes % 120 # => 1.minute
7.seconds % 3.seconds # => 1.second

7.minutes % 3 # => 0 
# because 420 seconds is divisible by 3

Should Duration % Numeric just be deprecated? Or maybe we try to fix it so 7.minutes % 3 # => 1.minute? I've thought about this for a bit but definitely need to think some more.