ActiveRecord relations' `each` method block argument is `T.untyped`

[marknuzz]

Calling #each on a PrivateAssociationRelation, PrivateCollectionProxy, etc. will give an untyped block argument. However, #each_with_index will give a typed argument. This is difficult to work around (adding a line of code with a T.cast for each time #each is used, is not a workaround at all).

These are classes which are also valid Enumerables, but the each method on these classes will use the untyped ActiveRecord::Delegation methods from the rbi/gems path, and not a path defined in rbi/dsl, or even sorbet's T::Enumerable class. This causes the block argument to be untyped, which causes a lot of missed type checking!!

[marknuzz]

Adding these lines to my require.rb in the meantime:

# https://github.com/Shopify/tapioca/issues/2034
ActiveRecord::Delegation.undef_method(:each)

[amomchilov]

Hi Mark,

I think this list of delegated methods in ActiveRecord::Delegation is the culprit.

That module is included after Enumerable in ActiveRecord::Relation. So that (untyped) delegated method takes precedence over the inherited one from Enumerable and prevents the inheritance of the correct sig from Enumerable.

I think the fix here might be as easy as adding the correct sig to ActiveRecord::Relation in RBI central:

https://github.com/Shopify/rbi-central/blob/af4f13dd7023734deb4500018b294367a3ed1cae/rbi/annotations/activerecord.rbi#L198-L201

class ActiveRecord::Relation
  sig { returns(T::Boolean) }
  def blank?; end

+  # adapted from https://github.com/sorbet/sorbet/blob/64b1468/rbi/core/enumerable.rbi#L19-L27
+  sig do
+    abstract
+      .params(blk: T.proc.params(arg0: Elem).returns(BasicObject))
+      .returns(T.untyped)
+  end
+  sig { returns(T.self_type) }
+  def each(&blk); end
end

That would be a great first contribution. Would you like to give it a shot?

[marknuzz]

Sure, I'll give it a shot this week. I've been slammed with deadlines so I can't guarantee it but it sounds simple enough and may make it easier for me to contribute other fixes later.

My workaround ended up breaking the usage of the .each method with no arguments anyway, since there was no concrete implementation of .each in the RBI, and Sorbet's Enumerable each sig returns T.self_type if used with no block.

[marknuzz]

I think the second overload above instead of sig { returns(T.self_type) } would be sig { returns(T::Enumerator[Elem]) }

[marknuzz]

@amomchilov I have a PR open here to address this Shopify/rbi-central#290

[amomchilov]

@marknuzz Approved and merged!

Could you please verify this locally, and confirm that it works before we close this?

[marknuzz]

@amomchilov Yes I have confirmed that the issue is fixed, working correctly for both the chained enumerator and non chained cases.

[marknuzz]

P.S. I would like to contribute to more type annotations but there's some questions I have. Is there a mailing list or group for discussion or should I ask about it in relevant issues/PRs like this Shopify/rbi-central#166

[danielveloso09]

Hi there.

This change raised us some errors, and just want to understand if it is or not expected.
Til today tapioca static analysis was not checking "virtual" fields when using ActiveRecord.select, and than use it like this field is part of the original class, in this case Delayed::Job. See the example bellow to easily understand the use case.

Example with dummy code:

queue_counts = Delayed::Job.select('count(*) as job_count, field').group('field')
queue_counts.each do |queue_count|
  ...
  queue_count.job_count
  ...
end

Error given when running bundle exec srb tc:

Method job_count does not exist on Delayed::Backend::ActiveRecord::Job

To address this issue across multiple projects, we developed a generalized shim like this:

# typed: strict
class ActiveRecord::Relation
  sig { abstract.params(blk: T.proc.params(arg0: T.untyped).returns(BasicObject)).returns(T.untyped) }
  sig { abstract.returns(T::Enumerator[T.untyped]) }
  def each(&blk); end
end

Was this an expected output of these changes?

[marknuzz]

@danielveloso09 I don't think that this change should be blamed for the issue. If you use .to_a.each, wouldn't you get the same error even before the change?

Tapioca considers Delayed::Job::PrivateRelationGroupChain to be a T::Enumerable[Delayed::Job], and that was the case for a long time.

If your model is defined by your application, you can use the attribute class method to define a virtual attribute, like described in this issue

If the model is not defined by your application, you can add something like this to an initializer before running tapioca dsl (just make sure there's no name collisions, and I'm not 100% sure if this is the optimal solution, it's just an idea):

Delayed::Job.class_eval do
  attribute :job_count
  attribute :field
end

Let me know if that works or not