PrivateRelation needs to be aware of class methods

[rzane]

Motivation

See #2118

Implementation

This PR looks up the available class methods and copies them to the generated relation modules using create_method_from_def.

One possible issue with this PR is how it might interact with other compilers. For example, ActiveRecord's enum defines a class method .statuses. The enum compiler will define a class method for this enum with types, but this compiler will also define that class method without types.

A better solution would be to define a module GeneratedClassMethods and include it explicitly in PrivateRelation. I'm not totally sure how to achieve this.

Tests

This PR includes tests.

[paracycle]

Thanks for your contribution, but this is not something Tapioca should be doing.

Class methods work on relations because they are delegated to (using method_missing) by the relation classes. That does not mean that the class methods are a part of the relation class for the model. For that reason, that delegation needs a different treatment than adding the methods to the relation class. An ideal solution would be some kind of annotation in Sorbet like delegates_missing_methods_to_type { Foo } which Tapioca could use to describe that delegation.

In the absence of such annotations, Tapioca should not be lying about class methods on models being defined as methods on relation classes.

[rzane]

Thanks for your reply @paracycle. I'm confused by your comment:

Class methods work on relations because they are delegated to (using method_missing) by the relation classes. That does not mean that the class methods are a part of the relation class for the model.

Isn't this also true for scopes? And yet, Tapioca does generate these method definitions for scopes?

I do agree that an annotation like delegates_missing_methods_to_type would be a much better solution to this problem, but since that doesn't exist, isn't the purpose of Tapioca's DSL compiler to provide type definitions for methods that are dynamically defined at runtime?

How is Shopify working around the absence of types for this extremely common pattern?

[paracycle]

Isn't this also true for scopes? And yet, Tapioca does generate these method definitions for scopes?

No. Scopes and class methods are treated fundamentally differently by Active Record. Scope methods and class methods look like they are equivalent, but they have fundamental difference, the main one being that scope methods always guarantee that a relation is returned, and thus, that you can chain them in a relation chain, whereas class methods don't.

You can see that scopes are added as relation methods to the GeneratedRelationsMethods module here: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/scoping/named.rb#L188

You can also run the following code and see that the scope is added to the relations module but the class method isn't:

❯ ruby -ractive_record -e "class Foo < ActiveRecord::Base; scope :foo, -> {}; def self.bar = 42; end; p Foo.const_get('GeneratedRelationMethods').instance_methods(false)"
[:foo]

I do agree that an annotation like delegates_missing_methods_to_type would be a much better solution to this problem, but since that doesn't exist, isn't the purpose of Tapioca's DSL compiler to provide type definitions for methods that are dynamically defined at runtime?

How is Shopify working around the absence of types for this extremely common pattern?

You can either use T.unsafe or manually add the missing methods on relations in shim RBI files.

[rzane]

Technically, a scope does not guarantee that a relation is returned. If a scope returns an integer, it'll return an integer. However, it does return a relation if the block returns nil.

You can either use T.unsafe or manually add the missing methods on relations in shim RBI files.

Neither of those are very appealing options. I'd either effectively lose all type safety related to relations, or I'd be manually curating a set of shims that this PR is capable of generating automatically.

Is the fact that those methods aren't technically defined on the relation really important? I mean, technically PrivateRelation doesn't even exist. So it does seem like we're fine with RBI files not being 100% in line with the way code is actually defined.

What are the downsides of defining these methods on the relation?

[paracycle]

Technically, a scope does not guarantee that a relation is returned. If a scope returns an integer, it'll return an integer. However, it does return a relation if the block returns nil.

You can either use T.unsafe or manually add the missing methods on relations in shim RBI files.

Neither of those are very appealing options. I'd either effectively lose all type safety related to relations, or I'd be manually curating a set of shims that this PR is capable of generating automatically.

Alternatively you could convert class methods to scopes. My suggestions work at Shopify scale, so I am not sure why they wouldn't work for you.

Additionally, what we are doing here is gradual typing. Nothing in your codebase will be fully typed, so I would caution to expect full typing guarantees from all of your codebase.

Is the fact that those methods aren't technically defined on the relation really important? I mean, technically PrivateRelation doesn't even exist. So it does seem like we're fine with RBI files not being 100% in line with the way code is actually defined.

Yes, it is important. PrivateRelation DOES exist, except we changed the name since what exists is a private implementation details of Active Record, and we wanted the name to signify that.

You can see how these are created here: https://github.com/rails/rails/blob/a0dfc671ad719c0fc13344c00fc62f395883ab69/activerecord/lib/active_record/relation/delegation.rb#L34-L44

❯ irb
irb> require "active_record"
#=> true

irb> class Foo < ActiveRecord::Base; end
#=> nil

irb> Foo.const_get("ActiveRecord_Relation")
#=> Foo::ActiveRecord_Relation

irb> Foo.const_get("ActiveRecord_AssociationRelation")
#=> Foo::ActiveRecord_AssociationRelation

irb> Foo.const_get("ActiveRecord_Associations_CollectionProxy")
#=> Foo::ActiveRecord_Associations_CollectionProxy

What are the downsides of defining these methods on the relation?

It would cause much more churn for RBI files, with very little additional benefit. Any time someone extends a module in a model class (which happens all the time with concerns that have class methods), all the methods from that module will be regenerated on the model's relation modules needlessly.

[magni-]

@paracycle what's the suggested workaround for dynamic scopes? I took a look at the aasm compiler to see how its automatic scopes were implemented, but they're absent from the compiler.

edit: looks like the state_machines compiler has something: https://github.com/Shopify/tapioca/blob/main/lib/tapioca/dsl/compilers/state_machines.rb#L341 I'll give that a try 👍🏼

edit2: And that worked ✨