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

Add support for #to_a in ActiveRecord relations compiler #1701

Merged

Conversation

olivier-thatch
Copy link
Contributor

@olivier-thatch olivier-thatch commented Oct 27, 2023

Motivation

This PR adds support for #to_a to the ActiveRecord relations compiler.

Sorbet can usually infer that #to_a returns the same type as #to_ary, but because Active Record explicitly aliases #to_ary to #to_a (here), Sorbet thinks #to_a is a separate method with no signature and thus returns T.untyped.

Cf. sorbet.run bug repro.

Implementation

Updated every place where a signature is defined for #to_ary to also define the same signature for #to_a, with the exception of the signature that returns NilClass (cf. comment below).

Tests

Updated the existing test for the compiler with the new signatures.

@paracycle
Copy link
Member

These relation classes already have to_ary methods defined on them (with the model method returning NilClass so that things like flatten work correctly). See here for reference:

klass.create_method("to_ary", return_type: "T::Array[#{constant_name}]")

As for to_a, that should already be working since the relations are Enumerable with the correct Elem type, no?

@paracycle
Copy link
Member

Here is a sorbet.run demo I created from the generated relation file to show that to_a should work as expected.

@olivier-thatch
Copy link
Contributor Author

Oh, weird. The whole reason I opened this PR is because I got a T.untyped... I'll try to reproduce my case on sorbet.run.

@olivier-thatch
Copy link
Contributor Author

It turns out #to_ary returns the proper type, but #to_a doesn't: sorbet.run bug repro

This is because Rails explicitly aliases #to_ary to #to_a (here), so Sorbet picks up on #to_a as a distinct method with no signature (since Tapioca only generates signatures for #to_ary). Without the alias call, Sorbet correctly infers that #to_a has the same signature than #to_ary.

@paracycle How do you think this should be handled? Should the relations compiler use alias too? Or duplicate the #to_ary signatures for #to_a?

@paracycle
Copy link
Member

It turns out #to_ary returns the proper type, but #to_a doesn't: sorbet.run bug repro

This is because Rails explicitly aliases #to_ary to #to_a (here), so Sorbet picks up on #to_a as a distinct method with no signature (since Tapioca only generates signatures for #to_ary). Without the alias call, Sorbet correctly infers that #to_a has the same signature than #to_ary.

Ah ok, thanks for the explanation.

@paracycle How do you think this should be handled? Should the relations compiler use alias too? Or duplicate the #to_ary signatures for #to_a?

I find being explicit in RBI files to be better, so I tend to not use alias or attr_xxx methods to define things in RBIs. I think the approach I would prefer is to add to_a implementations right below the to_ary ones in the current code (except for the one that returns NilClass since that's special case for to_ary on the model itself).

@olivier-thatch olivier-thatch force-pushed the olivier-activerecord-relation-to-a branch from 8467180 to 0964d42 Compare November 7, 2023 19:19
@olivier-thatch olivier-thatch changed the title Add support for #to_a and #to_ary methods to ActiveRecord relations compiler Add support for #to_a inActiveRecord relations compiler Nov 7, 2023
@olivier-thatch olivier-thatch changed the title Add support for #to_a inActiveRecord relations compiler Add support for #to_a in ActiveRecord relations compiler Nov 7, 2023
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great. You'll need to run bin/docs and commit the result, since you've made a docs change as well.

@olivier-thatch olivier-thatch force-pushed the olivier-activerecord-relation-to-a branch from 0964d42 to 27823c9 Compare November 7, 2023 19:26
@andyw8 andyw8 removed their request for review November 9, 2023 14:32
@paracycle paracycle merged commit dc4d4ad into Shopify:main Nov 10, 2023
15 checks passed
@olivier-thatch olivier-thatch deleted the olivier-activerecord-relation-to-a branch November 13, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants