tapioca icon indicating copy to clipboard operation
tapioca copied to clipboard

Tapioca doesn't import hand written signatures from the `stripe` gem

Open ms-jpq opened this issue 9 months ago • 19 comments

hola 👋,

Stripe just released their v14 of their SDK, and it ships with generated RBI definitions https://github.com/stripe/stripe-ruby/tree/master/rbi/stripe

But there is a problem:

when you run bundle exec tapioca gem stripe, you get the following output

  # source://stripe//lib/stripe/resources/v2/event.rb#13
  class Stripe::V2::Event::Reason < ::Stripe::StripeObject
    # source://stripe//lib/stripe/resources/v2/event.rb#23
    def request; end
    # source://stripe//lib/stripe/resources/v2/event.rb#21
    def type; end
  end

which contains no type definition for either request or type, when the original RBI file does include them:

see https://github.com/stripe/stripe-ruby/blob/5ce725ae222219a0ebc3d8f1140644e25fc9f740/rbi/stripe/resources/v2/event.rbi#L9


As a library author, how I got around this limitation is that I inserted a snippet at the beginning of my gem, such that when "required" under tapioca, it bails out of the introspection.

see https://github.com/orbcorp/orb-ruby/blob/d109a878d3a85e263521dab7ae604eaf63b81f54/lib/orb.rb#L21


Is there a less hacky way to work around this particular issue?

ms-jpq avatar Apr 08 '25 20:04 ms-jpq

We retrieve the RBIs but the resulting tree after merging doesn't contain the sigs. We'll have to find out why. The tree is huge which may be playing a part.

As a workaround you can exclude the gem's RBIs during tapioca generation and import their RBIs manually

# sorbet/tapioca/config.yml

gem:
  exclude:
    - stripe

KaanOzkan avatar Apr 09 '25 14:04 KaanOzkan

and import their RBIs manually

how would this work? (sorry I'm new to Tapioca)

rattrayalex avatar Apr 09 '25 19:04 rattrayalex

Is there a way a gem author could signal to Tapioca, "please trust/use the RBIs I provide", both for speed and accuracy/control purposes?

rattrayalex avatar Apr 09 '25 19:04 rattrayalex

Tapioca supports importing hand written RBIs from gems https://github.com/Shopify/tapioca?tab=readme-ov-file#importing-hand-written-signatures-from-gems-rbi-folder.

In the case of the stripe gem this functionality doesn't work and it'd have to be done manually, by copying and pasting.

KaanOzkan avatar Apr 09 '25 21:04 KaanOzkan

Hmm, I think this bit has been problematic for us:

and combine them with the RBIs it generates

This has caused us (I work with @ms-jpq) to have RBIs in our gem that are correct become incorrect when users import them with Tapioca.

Is there a way for a gem author to opt out of Tapioca doing its own generation in this way?

(Maybe I should open this as a separate issue, since this issue is about the functionality not working at all for Stripe/others?)

rattrayalex avatar Apr 09 '25 21:04 rattrayalex

I think the root cause is that Stripe RBIs use attr_reader and Tapioca only sees def methods (related to https://github.com/Shopify/tapioca/issues/1918).

The first step of fixing this fix was merged into rbi: https://github.com/Shopify/rbi/pull/308. I think we just need to add it to the formatter here: https://github.com/Shopify/rbi/blob/main/lib/rbi/formatter.rb (see https://github.com/Shopify/rbi/pull/450) then enable it here: https://github.com/Shopify/tapioca/blob/7f6109afd2014707e78ef195394c019f188b736d/lib/tapioca/rbi_formatter.rb#L25

Morriar avatar Apr 10 '25 15:04 Morriar

Is there a way for a gem author to opt out of Tapioca doing its own generation in this way?

This is an opt in feature by using the convention of root rbi/ folder.

If you're asking if tapioca could just "copy and paste" without "doing anything to the hand written RBI" that's not possible due to the root cause of the problem. It's an integral part of how tapioca operates. As I said above you can tell tapioca to not generate RBIs for this gem and do the importing yourself.

KaanOzkan avatar Apr 11 '25 14:04 KaanOzkan

Is there a way for a gem author to opt out of Tapioca doing its own generation in this way?

This is an opt in feature by using the convention of root rbi/ folder.

If you're asking if tapioca could just "copy and paste" without "doing anything to the hand written RBI" that's not possible due to the root case of the problem. It's an integral part of how tapioca operates. As I said above you can tell tapioca to not generate RBIs for this gem and do the importing yourself.

I have a terrible idea, us as library authors can use tapioca's introspection phase to execute some code that builds the correct rbi type definition.

Would this be stable enough to ship to production?

ms-jpq avatar Apr 11 '25 17:04 ms-jpq

Feel free to monkey patch tapioca before this is fixed but we can't guarantee that'll work in the future.

Assuming the finding above is accurate I think fixing this for everyone should be very doable. We haven't prioritized this issue yet but as always contributions are welcome.

KaanOzkan avatar Apr 11 '25 19:04 KaanOzkan

As I said above you can tell tapioca to not generate RBIs for this gem and do the importing yourself.

This requires manual work for the consumers of the gem, right? I'm looking for something where gem authors like us do some work, but developers installing the gem have a seamless experience.

rattrayalex avatar Apr 14 '25 03:04 rattrayalex

I think best way to achieve that would be to fix the root cause in rbi and tapioca gems.

KaanOzkan avatar Apr 14 '25 14:04 KaanOzkan

Okay great!

Assuming the https://github.com/Shopify/tapioca/issues/2248#issuecomment-2794152966 above is accurate I think fixing this for everyone should be very doable. We haven't prioritized this issue yet but as always contributions are welcome.

I don't think we'll have the bandwidth to contribute here in the near term, as we're focused on publishing ruby gems for both Anthropic (currently in beta) and OpenAI quite soon.

Like Stripe they both are affected by this issue, so it may rise in importance – is there any chance of prioritizing the fix(es)?

rattrayalex avatar Apr 15 '25 14:04 rattrayalex

We're hitting this issue also. Digging into the source, it looks like passing Keep::LEFT when merging the gem-provided rbi into the tapioca-generated rbi could be problematic, since it would always take the generated untyped output over the handcrafted typings: https://github.com/bandcampdotcom/tapioca/blob/c5e1c2477638e1b3de3b69611143b8210b3147d0/lib/tapioca/commands/abstract_gem.rb#L263

I tried changing that to Keep::RIGHT, but then I end up with malformed output where many of the classes end up in the global scope rather than the deep nesting that Stripe's SDK uses. I believe this, in turn, might be a bug in rbi around https://github.com/Shopify/rbi/blob/22a634e5eedf792e8d571320847d2ef58f6681d3/lib/rbi/rewriters/merge_trees.rb#L206

Will attempt to provide PRs.

bcdanieldickison avatar Sep 11 '25 22:09 bcdanieldickison

Below is my analysis so far. Unfortunately I don't think the fix will be trivial.

The underlying problem is that rbi's MergeTrees algorithm treats nested module names (module Foo; module Bar; end; end) differently from scoped module names (module Foo::Bar; end). This has a few effects:

  1. It causes node.compatible_with?(prev) to incorrectly return false if a class's superclass in one of the rbi files is specified with a scoped name while the other uses nested module lookup.
  2. It incorrectly creates a misnamed module in replace_scope_header when the nesting/scoping mismatches.

rbi could be updated so that superclass matching uses fully-qualified names, but this would mean the lookup needs to fully match ruby's namespace rules (nested modules can refer to outer modules by simple name, while namespaced modules need to use fully qualified names inside their body). It would also need to ensure copies of Class nodes reflect the correctly looked-up superclass name.

It just so happens that Stripe's exported rbi generally uses nested syntax, so if tapioca's generated rbi followed the same pattern, they would probably merge ok. But that's probably also a pretty big change.

Thoughts on how best to proceed?

bcdanieldickison avatar Sep 12 '25 00:09 bcdanieldickison

so if tapioca's generated rbi followed the same pattern, they would probably merge ok

I think this is unlikely because it looks like it'll add complexity. Right now we seem to get the fully qualified name from Sorbet and we do a simple RBI::Class creation https://github.com/Shopify/tapioca/blob/c5e1c2477638e1b3de3b69611143b8210b3147d0/lib/tapioca/gem/pipeline.rb#L342. No need to worry about nesting.

Ideally this would be fixed in rbi but I don't know how feasible that is. We could also add an option to only pull in hand written RBIs as a workaround.

KaanOzkan avatar Sep 12 '25 15:09 KaanOzkan

I'm thinking rbi's merge should normalize all scopes to use fully qualified names at the toplevel. I'll see if I can get that working and if it would be a reasonable change to make in rbi itself.

bcdanieldickison avatar Sep 12 '25 19:09 bcdanieldickison

I have what I believe is a solution in https://github.com/Shopify/rbi/pull/513. It ensures types referenced by the Stripe gem's exported rbi is matched against tapioca's generated rbi.

After applying that change, there's still a remaining issue with mismatches between Attrs and Methods. Tapioca generates def methods for accessors, while Stripe gem's rbi adds signatures on attr_reader, attr_accessor, etc. These don't currently get merged by rbi, and tapioca drops the signatures from the exported rbi accessors. Would be ideal to fix this in rbi also, but this can be worked around by changing tapioca's merge to use Keep::RIGHT instead of Keep::LEFT here: https://github.com/Shopify/tapioca/blob/87746e0f9d84543a51f9e103e335faf862fc0f90/lib/tapioca/commands/abstract_gem.rb#L263

Would be great to get some eyeballs on this possible solution and your thoughts on it.

bcdanieldickison avatar Sep 22 '25 22:09 bcdanieldickison

Alas using Keep::RIGHT for the merge causes other issues with method arity for methods that take optional params, e.g.

sorbet/rbi/gems/[email protected]:13: Method Date#initialize redefined without matching argument count. Expected: 4, got: 1 https://srb.help/4010
    13 |  def initialize(*_arg0); end
          ^^^^^^^^^^^^^^^^^^^^^^
    https://github.com/sorbet/sorbet/tree/d8bc4cf8c32907c73fc4e37bad875ffaff7658c6/rbi/stdlib/date.rbi#L200: Previous definition
     200 |  def initialize(year=-4712, month=1, mday=1, start=Date::ITALY); end
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Might need to tackle the method vs attr thing after all.

bcdanieldickison avatar Sep 23 '25 17:09 bcdanieldickison

Method and Attr merging is in here: https://github.com/bandcampdotcom/rbi/pull/2

bcdanieldickison avatar Sep 23 '25 20:09 bcdanieldickison