Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord relations' each method block argument is T.untyped #2034

Open
marknuzz opened this issue Oct 5, 2024 · 4 comments
Open

ActiveRecord relations' each method block argument is T.untyped #2034

marknuzz opened this issue Oct 5, 2024 · 4 comments
Labels
good-first-issue Good for newcomers help-wanted Extra attention is needed

Comments

@marknuzz
Copy link

marknuzz commented Oct 5, 2024

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
Copy link
Author

marknuzz commented Oct 5, 2024

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

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

@amomchilov amomchilov added the good-first-issue Good for newcomers label Oct 7, 2024
@amomchilov amomchilov self-assigned this Oct 7, 2024
@amomchilov amomchilov changed the title active record relations "each" method passes an untyped block argument ActiveRecord relations' each method block argument is T.untyped Oct 7, 2024
@amomchilov
Copy link
Contributor

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?

@amomchilov amomchilov removed their assignment Oct 8, 2024
@KaanOzkan KaanOzkan added the help-wanted Extra attention is needed label Oct 8, 2024
@marknuzz
Copy link
Author

marknuzz commented Oct 8, 2024

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
Copy link
Author

marknuzz commented Oct 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good for newcomers help-wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants