search

Shopify / tapioca

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

[Feature request] Include sigs from gem rbi files #139

Closed kirbycool closed 3 years ago

kirbycool commented 4 years ago

Hi! I've been really happy using tapioca for my company's codebase, but have run into a snag on more recent versions of sorbet.

We have an internal gem that contains our protobuf definitions and is used by a variety of other apps. If your not familiar, protobufs have a compilation step that take *.proto files which define protobuf messages (essentially structs) and output *_pb.rb files that define corresponding ruby classes. I've been using this protoc plugin to generate rbi files at compile time and putting them in the gem's rbi/ folder.

This had been working pretty well; sorbet picks up the rbi files from the gem and typechecks the proto classes in our apps. The rbis stay in sync because they're auto-generated at compile time.

Unfortunately, a recent change in sorbet prevents sorbet from using gem rbis when tapioca is being used, with the rationale that tapioca copies sigs from gems when you run tapioca generate. As far as I can tell, this is the case for inline sigs, but does not cover our use case of generated rbi files.

I've read some of the discussion on the sorbet slack and understand that this hasn't been a priority to implement, but I want to present this use case where copying gem rbis would be useful.

If you think this is a worthwhile feature and feasible, I'm happy to spend some time working on it.

paracycle commented 4 years ago

Hello @kirbycool, thanks for using Tapioca and for reaching out about this. I see the use-case and I think we can make copying the RBI file distributed in the /rbi folder the default action in Tapioca over trying to generate a typed-RBI file from source.

I can take a look at implementing this this week or next. If you are willing to take a stab at it, though, I'd also be happy to help you contribute this as a feature.

Let me know how you want to proceed.

kirbycool commented 4 years ago

Great! I should have some time this weekend to work on this.

kirbycool commented 4 years ago

Ah, I'm just seeing the recent work on the dsl feature which would cover my protobuf use case. I'm actually busier now than I'd hoped, so I'm not sure I'll have time to work on this right now.

che-burashco commented 3 years ago

I actually had a similar problem, so I wrote a script to copy over the RBIs from gems. Shouldn't be a big deal to incorporate it into Tapioca. @paracycle qq for you: right now there is one RBI generated by the Tapioca per gem, technically gems can have multiple RBI files in them, I can concatenate all the files into one RBI written to the users sorbet/tapioca folder or I can modify the behavior to have a folder for gem and copy over the RBI files as is. Any preferences?

paracycle commented 3 years ago

So, the copying operation itself wouldn't be that hard, and, yes, we would like to keep the "single file per gem" simplicity of the current operation.

What is more complicated are gems that export RBI files, but actually have inline signatures from which Tapioca can generate perfect RBIs with signatures from the source instead of relying on a potentially incomplete or outdated RBI file bundled with the gem.

In those cases, I would like to default to generating from source and not using the bundled RBI file. How to decide when to do which is the big question for me.

jathu commented 3 years ago

@paracycle what are your thoughts on ignoring gems that have bundled RBI files, but provide an option to force generate RBIs via something like --force or --ignore-bundled-rbi? This would be useful for tapioca sync. I should note that bundled RBI types are supported by Sorbet and the recommended way going forward [1]:

We expect that as adoption grows, gems will include their external interfaces RBI files in an rbi/ directory. When they do this, Sorbet will automatically include those definitions when typechecking a project. In the future, we anticipate this to be the preferred way to include RBI files into a project.

[1] https://sorbet.org/docs/rbi#rbis-within-gems

paracycle commented 3 years ago

Ok, I think we are in a good place to tackle this issue now that https://github.com/Shopify/rbi/pull/76 is merged in the rbi repo.

Here is my roadmap for it:

  1. Tapioca parses all RBI files in a gem that exists in the gem's /rbi folder. This becomes RBI Tree A.
  2. Tapioca, as usual, generates an RBI tree as it processes the gem source code. This becomes RBI Tree B.
  3. Tapioca merges Tree A and Tree B into Tree C, with the conflict resolution preferring definitions from Tree B (tree generated from source).
  4. The resulting Tree C is serialized as the RBI file corresponding to the gem.

These steps address all of the concerns in this issue:

  1. If a gem has no sigs in its source, but exports RBI files from /rbi folder, then Tapioca would be just generating class/module definitions with methods, etc from the source. The sigs from the RBI files should mostly stay intact as they come from the RBI file. This addresses the original case in this issue.
  2. If the gem has no /rbi folder, then Tapioca will just work as it is working now.
  3. If the gem has both a /rbi folder and sigs in its codebase, then Tapioca can merge them by preferring the source code information over the RBI file one, which we expect to be more correct, since ultimately it is the sigs in the source that will be checked at runtime and not the RBI file.