Shopify / tapioca

The swiss army knife of RBI generation
MIT License
746 stars 128 forks source link

ActiveStorage macro methods failing type checks #557

Closed talhasyed closed 1 year ago

talhasyed commented 3 years ago

When using ActiveStorage macro methods like has_many_attached in model files, the type check fails with an error along the lines of:

app/models/my_model.rb: Method has_many_attached does not exist on T.class_of(MyModel) https://srb.help/7003
--
has_many_attached :files
^^^^^^^^^^^^^^^^^^^

There is a known workaround that involves adding knowledge about the ActiveStorage::Attached::Model methods to ActiveRecord::Base in a shim file:

# typed: true

class ActiveRecord::Base
  include ActiveStorage::Attached::Model
end

This workaround should not be required, and the generated .rbi files for ActiveRecord models should already include ActiveStorage related methods.


EDIT: We can generate this information as part of the ActiveStorage generator.

paracycle commented 3 years ago

The fact that has_many_attached or has_one_attached exists as methods on models depends on if the application is using Active Storage or not, so this include should not be the default.

It is up to the application to configure this as well as configuring Active Storage. Otherwise, we will act like those methods exist for projects that don't use Active Storage at all, which is not correct.

talhasyed commented 3 years ago

Thanks, @paracycle

From a developer experience point of view, though, should the generated .rbi files not include the methods for (in this case) ActiveStorage without having to do the shim workaround? And have tapioca figure out if they need to be included or not? Or is that not possible with Sorbet at this point?

paracycle commented 3 years ago

I think we are talking about slightly different things. Tapioca does generate all the correct methods in the ActiveStorage::Attached::Model module. Both has_many_attached and has_one_attached exist inside that module in the activestorage RBI file.

What is missing is the fact that that module is included in ActiveRecord::Base which Tapioca cannot know about when generating gem RBIs. It is not a Sorbet limitation, it is something about how your application is using those modules dynamically. Since Tapioca does not load your application when doing gem RBI generation at all, there is no way it can know how the application is using those modules exported from gems.

I do understand the developer experience perspective but the shim is not a workaround, it is a fallback way to tell Sorbet that is not visible statically.

One thing we can do, though, is to teach our DSL generators about this include and make the DSL generator generate this if Active Storage is enabled. Since DSL generators are app specific and load the application, that would work really well.

I am reopening this and changing the ask to be about DSL generators instead. Thanks

paracycle commented 1 year ago

Now that #1389 made it so that Tapioca is running initializers inside Rails engines, it so happens that we are now able to trigger the initializer that is defining this include and generate it in the gem RBI file.