Consider not using `Numeric` for ActiveRecord `#sum`
olivier-thatch opened this issue · 6 comments
v0.13.0 introduced typed signatures for ActiveRecord #sum so that these methods now return a Numeric.
While I can see how, on paper, this is an improvement over returning in T.untyped, in practice it's a bit of an annoyance.
In plain Ruby, Numeric covers the following types: Float, Integer, Complex and Rational.
If bigdecimal is required, then the BigDecimal and Date::Infinity types are also added to the list of Numeric descendants. (No clue how or why Date::Infinity is added when requiring bigdecimal...)
In a typical Rails app, ActiveSupport also adds ActiveSupport::Duration::Scalar to the list of Numeric descendants.
The net result is that Numeric is not a very useful supertype because it covers a bunch of rather heterogenous subtypes, some of which will never be relevant to #sum (e.g. I don't see how #sum would ever return a Date::Infinity or ActiveSupport::Duration::Scalar, and even Complex or Rational seem like extremely fringe use cases).
If we want to use any method that exist on the actual type returned by #sum (which I expect to nearly always be either an Integer, a Float or a BigDecimal) but not on all Numeric descendants, we are now forced to cast the result with T.cast.
This is admittedly subjective and just my opinion, but to me this feels like a worse experience than returning a T.untyped and being able to call the desired method without casting.
I would suggest either returning to the previous state of returning T.untyped, or use a new type alias that is less broad than Numeric (something like SumReturnType = T.type_alias { T.any(Integer, Float, BigDecimal) }).
I'm going to close this as a duplicate of #1822. Hopefully the associated pull request will improve the experience of using the sum method with types! Let me know if it doesn't and we can address any remaining issues.
#1830 will fix the issue where sum is used with a block (which is great, as we also ran into that issue in our codebase), but the non-block signature is unchanged and still returns Numeric.
Would you be open to changing the return type to something less broad than Numeric like T.any(Integer, Float, BigDecimal)? I'm happy to submit a PR if so. Or maybe we can keep the current behavior by default, but make it possible to pass a configuration value to the ActiveRecordRelations compiler to override the return type? I'm not sure if there's precedent for passing configuration values to Tapioca compilers.
T.any(Integer, Float, BigDecimal) should work. I think the actual SQL query would return one of those, not a more complex "Numeric" type.
@egiurleo Can this issue be reopened and/or can we get your thoughts on changing the return type to T.any(Integer, Float, BigDecimal)?
@olivier-thatch I see what you're saying -- apologies for closing the issue prematurely. I'm looking into this now and I'll bring it to the team for discussion on Monday.
Yeah @olivier-thatch please go ahead and open a PR! We'll review.