Resolve scopes of referenced types when merging trees

[bcdanieldickison]

This is another attempt to fix tapioca's rbi incorrectly merging of the Stripe gem, as described here: Shopify/tapioca#2248. Previous attempt: #509

Problem

When merging two trees that refer to the same scope using nested vs namespaced syntax, the merge algorithm was treating those as different scopes. For example:

# lhs
module Foo; end
class Foo::Bar; end
class Foo::Baz < Foo::Bar; end

# rhs
module Foo
  class Bar; end; # See note 1
  class Baz < Bar; end
end

The two definitions of Foo::Baz were considered incompatible due to mismatched superclasses even though they technically both refer to Foo::Bar. Note 1: Additionally, in the case of the Stripe SDK, the superclass in right hand tree (the gem's exported rbi) could refer to a type only defined in the left hand tree (tapioca's generated rbi).

This Solution

Wherever there's a type reference in the right hand tree during a merge, it is looked up in the left hand tree using rules mimicking Ruby's semantics. This is applied to the following places where types can be referenced:

• Class superclasses
• Mixins
• Signatures on Attrs and Methods

The output then uses fully-qualified names for those references.

Note that this makes the merge asymetric: the left-hand tree is expected to contain all the types that could be referenced by either tree.

TODO

For further improvement, consider merging Attr and same-named Method. In the case of Stripe SDK, tapioca generates methods from source but the exported rbi defined attr accessors, which should be merged (along with their signatures). This is implemented in a second PR here: bandcampdotcom#2

[bcdanieldickison]

(Legal is reviewing the CLA currently.)

[KaanOzkan]

High level this solution makes sense to me. I can't immediately think of any issues but we'll test it. Let us know how CLA goes and also I triggered CI.

[bcdanieldickison]

Thanks @KaanOzkan. I'm a bit stumped by the type checking error that remains, where I'm passing a Hash into a method using the keyword splat syntax:

lib/rbi/rewriters/merge_trees.rb:345: Expected `RBI::Type` but found `T::Hash[Symbol, RBI::Type]` for argument `params` https://srb.help/7002
     345 |            copy.params(**params) # This should be **params but sorbet seems to get tripped up?
                                  ^^^^^^^^
  Expected `RBI::Type` for argument `params` of method `RBI::Type::Proc#params`:
    lib/rbi/type.rb:736:
     736 |      #: (**Type params) -> self
                           ^^^^^^

Got any tips on how to express this typing correctly? It does function correctly despite the error.

[KaanOzkan]

@bcdanieldickison Yeah looks like the API is designed to be used with kwargs. We could either create another setter function that takes in a non splatted Hash type or use T.unsafe(copy).params(**params). Since it's one usage maybe latter is fine.

[bcdanieldickison]

Thanks for the tip. I like using T.unsafe since it's just one spot, and it seems like sorbet ought to type check this correctly?

[bcdanieldickison]

Legal tells me they should be able to take a look at the CLA later this week… ⏳

[KaanOzkan]

I tried to test the performance of this feature but ran into errors. I'd like to make sure it doesn't effect RBI generation too negatively.

[KaanOzkan]

Mutating in a ? method doesn't feel right but maybe it's okay since it's simpler than alternatives.

[bcdanieldickison]

Yeah I wasn't a fan of this either. Maybe the method can instead return :compatible (Symbol) rather than a boolean, and that way the linter won't demand the method be named with ?.

[bcdanieldickison]

Lmk what you think of 3d2efed

[Morriar]

I don't think this is the right approach, we're shoving way too much logic into the merging and I'd prefer for it to stay as simple as possible: purely syntactic, no name resolution.

I think your previous approach was better but I would tweak it a bit:

1. Provide a flatten rewriter in rbi like you attempted in #509
2. Let Tapioca apply the flatten rewriter onto the imported RBI file
3. Merge the flattened RBIs together

That way we limit the complexity of names handling to the flatten rewriter and we do not need to touch the merge rewriter at all.

[bcdanieldickison]

Thanks for the review @Morriar. Unfortunately, I don't think any flavor of purely syntactic merging is feasible. I wrote about it in a comment when closing the other ticket, but the gist is that non-fully-qualified names can only be resolved by actually looking it up in the scope chain. E.g. when given this in the rhs:

module Foo
  class Bar
    # could be ::Foo::Bar::Baz, ::Foo::Baz, or ::Baz
    sig { returns(Baz) }
    def baz; end
  end
end

it's ambiguous whether Baz refers to ::Foo::Bar::Baz, ::Foo::Baz, or ::Baz without looking up types defined in the lhs. We also can't keep that sig return type as the unqualified Baz if we're merging this sig into a lhs tree that defined Foo::Bar without nesting, because that excludes ::Foo::Baz:

class Foo::Bar
  # can only be ::Foo::Bar::Baz or ::Baz
  sig { returns(Baz) }
  def baz; end
end

It we add the constraint that all type references must be defined in the same rbi tree, then I think a flatten rewriter can unambiguously resolve these types to fully-qualified names by only referencing each individual tree. Alas the Stripe gem rbi doesn't follow this convention—not sure if it's reasonable to say it's a bug with the exported rbi from the gem. I am curious how others are able to effectively use the Stripe gem with typings.

[bcdanieldickison]

CLA has been signed. Can I get another look at this PR wrt the last comment, please?

[KaanOzkan]

@bcdanieldickison sorry for the wait. After some internal discussion, this approach of resolving scopes by walking the chain was deemed too complex. We also think that the usage of nested namespaces in stripe-ruby RBIs is brittle in isolation and using fully qualified names with a flat namespace would be better.

As a result, we think a better solution would be to change the RBI Generator stripe-ruby uses to generate types in the flattened namespace format with fully-qualified constant references that Tapioca uses. This makes the RBI files less brittle and avoids needing constant resolution logic altogether.

The generator appears to be private to Stripe, but what do you think about bringing this up in the stripe-ruby repository with this PR and Shopify/tapioca#2248 as context?

Thank you for all your investigation and the fix efforts, sorry again for the delay.

[bcdanieldickison]

@KaanOzkan no problem. Yes, I think it's reasonable to ask Stripe to generate their exported rbi without nesting, using fully scoped names. We have an open issue that includes this problem stripe/stripe-ruby#1577 so I'll forward this comment along there.

It'll probably be a good idea to explicitly disallow nested scoping in rbi files to avoid this problem elsewhere. I'm not sure if this should be at the rbi level or in tapioca, but would need to be a soft deprecation initially.

[KaanOzkan]

@bcdanieldickison Thank you!

It'll probably be a good idea to explicitly disallow nested scoping in rbi files to avoid this problem elsewhere. I'm not sure if this should be at the rbi level or in tapioca, but would need to be a soft deprecation initially.

I think displaying a warning will be useful but we do want to generate the RBI even if we can't import definitions from therbi/ folder. We could implement it similar to the ConflictTree, behind a flag. However, I'm not sure how to best surface the information as there'll be a lot of violations.

Alternatively, we could have a simpler "approximate" solution in tapioca that looks at a subset of the exported RBI, do a string comparison and output a warning if it observed nested namespacing in that sample.