feat: publish RBS and RBI types with Gem

[chris-olszewski]

What was changed

Include RBS types in published Gem along with newly added RBI types.

RBS

The codebase is already type checked with RBS, we just never included these type definitions as part of the gem. We now include these in the gem with experimental support.

RBI

These are net new, initially generated with Tapioca, narrowed via Claude/myself, and validated using srb tc and enabling sobet-runtime in our tests.

extras/sorbet_check

A minimal file that is type checked with Sorbet to validate usage of the RBI.

SigApplicator

To better verify that the RBI is correct, we run our tests with each method redefined with sorbet-runtime signatures generated from our RBI file. These will also be released as experimental.

We cannot apply this to every class as some:

• Would require generic which we cannot easily declare e.g. Temporalio::Workflow::Future
• We intentionally avoid following the type definition e.g. base constructor for interceptors
• Classes we don't control which cause issues e.g. those generated by protobuf

Using SigApplicator is controlled by the TEMPORAL_SORBET_RUNTIME_CHECK environment variable.

We did have to update some tests where we intentionally pass in mistyped data.

Review Guidelines

I would focus highly on the SigApplicator module as this is our primary tool for verifying our RBI file. Looking at the RBI definitions for Workflow is worthwhile, but unsure the value of crawling through those 7k lines of type definitions.

Why?

Community ask

Checklist

1. Closes [Feature Request] Publish RBI types #412

2. How was this tested:
   See RBI section above.

3. Any docs updates needed?
   Updated README, further docs will be updated as we move this out of experimental.

[jazev-stripe]

This looks really promising, and I think would already be pretty useful to us. The approach of applying the runtime checks during tests is new to me--seems quite cool!

I left a few comments; we also have a thread in the shared Stripe <> Temporal Slack channel (#support-stripe-temporal) about this.

[jazev-stripe]

For some expressions like this (and in various other places in this file), could we add T.assert_type!(...) calls? https://sorbet.org/docs/type-assertions#tassert_type

The runtime typechecking that assertion performs wouldn't run, but the static typechecking would

[jazev-stripe]

Would it be possible to include the RBI types for these as well? At Stripe we maintain https://github.com/sorbet/protoc-gen-rbi, which I'd recommend. Tapioca looks like it also has its own support: https://github.com/Shopify/tapioca/blob/main/manual/compiler_protobuf.md, though I'm not sure how it compares.

To give some motivation, this is the most common source of needing to do T.unsafe(...) in our integration with sdk-ruby at Stripe today (which is still in its early stages). We have code that looks like this:

workflow_service_client = T.let(
  temporal_client.workflow_service,
  Temporalio::Client::Connection::WorkflowService
)
namespaces_result = workflow_service_client.list_namespaces(
  T.unsafe(Temporalio::Api::WorkflowService::V1::ListNamespacesRequest).new(
    page_size: page_size,
    next_page_token: next_page_token
  )
)

The existing types being added in this PR would let us remove the T.let(...) cast on the first statement [0], but we'd need to keep the T.unsafe(cls).new(...) on the second statement unless there were RBI stubs for the Protobuf types.

It's understandable if the SigApplicator system wouldn't work for these (since I think the methods might not actually exist on the classes?

[0]: (we could remove that today, but we'd be calling a method on a T.untyped)

[jazev-stripe]

If sdk-ruby weren't able to publish the RBI types for the protobufs, I think we'd strongly consider setting up our own additional RBI generation to cover the protobuf classes, at Stripe.

I think it's still globally preferable if this is provided by the upstream, though (since all users will get them).

[jazev-stripe]

Oh neat, was this a true positive error that Sorbet caught?

[chris-olszewski]

Yes!

[jazev-stripe]

Could we have non-T.untyped type annotations for the service stack as well?

At Stripe we currently have a bespoke interceptor stack inside Temporalio::Client::Connection that sits between the bridge RPC stubs and the rest of the SDK / the Temporalio::Client (so that we can apply RPC metadata to client calls not supported by Temporalio::Client::Interceptor). Having type annotations on these classes/methods would be useful for us.

[jazev-stripe]

If this type is unknown and can't be easily constrained using a generic (or if it's not worth it), using Object or T.anything would be preferable:

Suggested change

	known_outcome: T.untyped,
	known_outcome: T.anything,

[jazev-stripe]

Can this provide a more specific type (are decoded headers always strings? or does it depend on the converter behavior?). If there is none, T.anything would be preferable, I think

[jazev-stripe]

Would you consider minimizing the number of T.untyped throughout this file as much as possible? (except some of the things like the .new types on the Data classes)

If you were able to minimize the instances of that, adding a ratchet to prevent future changes to the RBI types (when new APIs are added to the SDK) from being added as T.untyped might also be prudent.

[jez]

@jazev-stripe mentioned that you might want some feedback on this approach, don't mean to intrude, very excited to see this in any form it takes, it's extremely cool!

[jez]

Something you might consider is monkey patching Module to have this, rather than doing one extend per method

class ::Module
  include T::Sig
end

or do something like

::Module.include(::T::Sig)

in the start of apply_all!. Just avoids doing extra work once-per-method when it only needs to be done once-per-codebase.

[jez]

I don't quite know what effects this could have, it's akin to doing this:

def foo
  my_logic
end

sig { void }

def foo
  my_logic
end

For example, some places where this could go haywire:

• At load time, someone could do something to grab a method object for a method: FooMethod = MyClass.instance_method(:foo). After redefining the method, the method object still holds a reference to the not-wrapped method.
• I believe that stack trace lines will show the definition of foo at this line in sig_applicator, instead of in its original file. That would have already been the case due to the signature_for_method call which wraps the method, but anyone who then uses reflection to do something like signature.method.source_location in an attempt to get a reference to the "original" method's source location will see this file, instead of the file where foo is actually defined (haven't confirmed this, I'm just guessing)

Not sure if either of those matter in practice for this use case.

The other thing which you might be able to try is to "fake" the logic that Sorbet currently does in its method_added hook. Normally the way that

sig { returns(Integer) }
def foo = 0

works is that sig runs, it stores a T::Private::Methods::DeclarationBlock in some thread-local variable, then Ruby runs the def, it evaluates, and fires the method_added hook. In that, Sorbet looks up the sig from the thread local and uses it to redefine the method with the signature. So if you really wanted to do something as close as possible to what sorbet-runtime would have done, you might be able to write your own method_added hook which

1. Figures out which method was just defined
2. Looks it up in the parsed RBI
3. Evaluates the sig with class_eval like above (i.e. "just in time"), which should populate the same thread local that sorbet-runtime would have populated, had the sig been evaluated like normal
4. Calls Sorbet's T::Private::Methods._on_method_added hook, which will read that thread local and do the rest of the wrapping logic it would have done for any method definition. That would be a way to get rid of this define_method that you have here yourself.

Again, that might be overkill for this use case, it's possible that the double define_method (the one here, and the one inside signature_for_method) is actually fine.

I believe that this would also mean you could get rid of or at least simplify the define_method below to "Restore the original method without the sig wrapper" because you wouldn't have redefined it in the first place.