Active record relations compiler generates incorrect `sum` sigs

[stathis-alexander]

The active record relations compiler generates incorrect sigs for sum.

It is typed only allow the a first argument of a column name, but the active record defintion allows for passing an initial argument as well. See tapioca compiler here and active record definition here.

Additionally, it types the return to a Numeric when the result can be anything, depending on the block provided (or initial argument).

I looked into putting up a PR for this behavior myself, but I see that @jeffcarbs just merged a related change. It seems like it might be appropriate to use overloaded sigs here, but I'm not entirely sure. The Sorbet docs recommend against overloading sigs.

[KaanOzkan]

Looking at this further I think it can return nil or column type. We could add overloaded sigs but we'd have to cover every column type in rails in an separate sig. T.untyped in a T.any isn't possible since it takes priority. I'm fine with having overloaded sigs in the RBI and these ones won't be too hard to understand but it'd be very long.

[stathis-alexander]

Looking at this further I think it can return nil or column type.

It's a little worse right?

my_model.my_association.sum(some_init_arg) do |my_association_obj|
  # ... do nontrivial transformation of my_association_obj
end

It feels more like an Elem thing to me: If initial arg is passed in, then the result to whatever type that is. If column name is passed in, then the return type is the column type.

For instance, we use monetize for many of our models:

class MyModel < ApplicationRecord
  # ...
  monetize :amount_cents # exposes an `amount` method
end
# ...

MyModel.where(...).sum(Money.zero, &:amount)
MyModel.where(...).sum(0, &:amount).to_money

[jeffcarbs]

I just played around in a local console with the non-block form:

> User.sum(:email)
=> "0.0"
> User.sum(:created_at)
=> nil
> User.sum(:sign_in_count)
=> 16584

I tried with monetize and it looks like you can do sum(:amount) and it will coerce the result into a Money:

> MyModel.sum(:amount)
=> #<Money fractional:0 currency:USD>
> MyModel.sum(:amount_cents)
=> 0

With the block form, I think the return value will be whatever the type of the initial_value_or_column argument would be:

> User.sum(10, &:email)
~/activerecord/lib/active_record/relation/calculations.rb:174:in `+': String can't be coerced into Integer (TypeError)
> User.sum(10, &:sign_in_count)
=> 16594
> User.sum('emails: ', &:email)
=> "emails: [email protected]...." (truncated for brevity)

I think that sig would be something like:

sig do
  type_parameters(:U)
    .params(
      initial_value_or_column: T.type_parameter(:U),
      blk: T.proc.params(arg0: MyModel).void,
    ).returns(T.type_parameter(:U))
end

I could see adding the overload for the block form, and then reverting back the non-block form to just return T.untyped. If we start trying to say it could return a giant T.any I think it makes the return value of the sig pretty useless since you'll wind up needing to cast it anyway.

[stathis-alexander]

Nice, thanks @jeffcarbs . Interesting to see it assume a numeric, I assume that's because by default it uses 0 to seed the sum and likely has some sort of safe "try cast" behavior for non-numerics.

I'm aligned with not trying to handle too many cases.

Something like:

sig { params(initial_value_or_column: T.nilable(T.any(String, Symbol))).returns(Numeric) }
sig do
  type_parameters(:U)
    .params(
      initial_value_or_column: T.type_parameter(:U),
      blk: T.proc.params(arg0: Elem).void,
    ).returns(T.type_parameter(:U))
end
def sum(initial_value_or_column = nil, &blk); end

would probably cover all of the cases here?

[stathis-alexander]

I was able to get this to work with some nasty monkey patching in my local environment. I modified here to be:

sigs = [
  common_relation_methods_module.create_sig(
    parameters: [create_opt_param('initial_value_or_column', type: 'T.nilable(T.any(String, Symbol))', default: 'nil')],
    return_type: 'Numeric',
   ),
   RBI::Sig.new(
     type_params: ['T'],
     params: [
        RBI::SigParam.new('initial_value_or_column', 'T.nilable(T.type_parameter(:T))'),
        RBI::SigParam.new('block', "T.proc.params(object: #{constant_name}).returns(T.type_parameter(:T))"),
     ],
     return_type: 'T.type_parameter(:T)',
   ),
 ]
common_relation_methods_module.create_method_with_sigs(
  method_name.to_s,
  sigs: sigs,
  parameters: [
    RBI::OptParam.new('initial_value_or_column', 'nil'),
    RBI::BlockParam.new('block'),
  ],
)

and it eliminated the hundred or so sorbet errors caused by this discrepancy.

I am not familiar with submitting changes to this project, but would be happy to create a PR to get the ball rolling on this change.

[KaanOzkan]

I think the sig part can be implemented as a helper here https://github.com/Shopify/tapioca/blob/main/lib/tapioca/helpers/rbi_helper.rb so that it's reusable. It'd also encapsulate RBI:: calls to the helper.

[stathis-alexander]

https://github.com/Shopify/tapioca/pull/1830