Add support for RBS signature comments #2236

Morriar · 2025-03-14T14:18:38Z

Motivation

Before this PR, when generating RBI files for gems using RBS comments, Tapioca was unable to generate signatures.

This PR introduce a preprocessing of all loaded files that rewrites the RBS comments into actual Sorbet signatures blocks.

Thus a file looking like this:

class SomeGem
  #: -> void
  def foo; end
end

Will be translated into this at load time:

class SomeGem
  sig { void }
  def foo; end
end

This will enable sorbet-runtime to properly see the signature and wrap the method as it would do normally.

Then Tapioca will be able to pick up the signature and generate properly types RBIs for gems using RBS comment.

Implementation

We rely on ruby-next for the rewriting. While I not convinced by the gem itself, it does work. We could potentially rewrite the mechanism with an ad-hoc implementation later.

Tests

See automated tests.

Signed-off-by: Alexandre Terrasa <[email protected]>

Signed-off-by: Alexandre Terrasa <[email protected]> Co-authored-by: Ufuk Kayserilioglu <[email protected]>

Signed-off-by: Alexandre Terrasa <[email protected]> Co-authored-by: Kaan Ozkan <[email protected]>

Morriar · 2025-03-14T14:19:09Z

lib/tapioca/internal.rb

@@ -1,6 +1,11 @@
 # typed: strict
 # frozen_string_literal: true

+# The rewriter needs to be loaded very early so RBS comments within Tapioca itself are rewritten
+require "spoom"
+require "tapioca/rbs/rewriter"


We need this to happen super early so we also rewrite the RBS comments we find inside Tapioca

Morriar · 2025-03-14T14:20:21Z

lib/tapioca/loaders/loader.rb

@@ -160,7 +160,7 @@ def with_rails_application(&blk)
        Rails.app_class = Rails.application = rails_application
      end

-      #: -> Array[singleton(Rails::Engine)]
+      T::Sig::WithoutRuntime.sig { returns(T::Array[T.class_of(Rails::Engine)]) }


Now that we re-execute the signatures, having a reference to Rails is a problem. For now I changed them back to a T::Sig::WithoutRuntime. We can add a @without-runtime annotation later.

Morriar · 2025-03-14T14:20:51Z

lib/tapioca/rbs/rewriter.rb

+
+# We need to include `T::Sig` very early to make sure that the `sig` method is available since gems using RBS comments
+# are unlikely to include `T::Sig` in their own classes.
+Module.include(T::Sig)


@KaanOzkan Is there any adverse effect to do this in the context of the LSP addon?

No it should be fine. Rewriting itself could be an issue with the addon, I'll test it.

It seems okay on a first glance, not blocking.

Morriar · 2025-03-14T14:21:24Z

lib/tapioca/rbs/rewriter.rb

+
+RubyNext::Language.include_patterns.clear
+RubyNext::Language.include_patterns << "**/*.rb"
+RubyNext::Language.rewriters = [Tapioca::RBS::Rewriter]


Note that we're replacing any list of rewriters that may have been set

Morriar · 2025-03-14T14:22:36Z

lib/tapioca/rbs/rewriter.rb

+      #: (String source) -> String
+      def rewrite(source)
+        # Do not try to parse files that don't have RBS comments
+        return source unless source =~ /^\s*#\s*typed: (ignore|false|true|strict|strong|__STDLIB_INTERNAL)/


Parsing all the files is a major slow down. We escape early if the file is not likely to contain Sorbet signatures.

Note that I'd like for us to move away from having typed: X in our files, so maybe we should be matching on #: instead?

The more I'm thinking about it and the more I think it's a mistake. I already got confused while writing tests because I forgot the sigil. Matching on #: seems safer and future proof 🤔

We would gain significant performance improvement for tapioca gem with arguments if we could rewrite only the gems we are generating RBIs for. It would help with the performance loss of the early return when we match #: instead.

Morriar · 2025-03-14T14:23:05Z

spec/tapioca/gem/pipeline_spec.rb

+          sig { returns(::String) }
+          def foo; end
+
+          def foo=(_arg0); end


Note that writer were already untyped before this PR.

KaanOzkan · 2025-03-14T18:14:47Z

lib/tapioca/bundler_ext/auto_require_hook.rb

@@ -37,7 +37,7 @@ def override_require_false(exclude:, &blk)
        end
      end

-      #: -> untyped
+      sig { returns(T.untyped).checked(:never) }


Why are some comments now sigs? Is it so that tapioca's own RBIs are generated correctly?

Will it error in CI if someone wrote a RBS comment that was supposed to be a sig?

It's only for the checked(:never) and Sig::WithoutRuntime because now that we re-inject them they get executed. This will only be required for the sigs in Tapioca we do not want to run at runtime.

#2236 (comment)

Thanks, I missed the checked(:never) calls. I think this is fine for now, we can look at speeding it up later.

Locally I reverted some of these sigs to comments but I don't get any test failures. I'm wary that we'll forget to use sigs in this case and only encounter the issue after we release/merge. Is there a way we can run into this failure in CI and create a check?

CI should fail if any of this method gets reverted to one checked at runtime. Try with the BasicObject test for example: bin/test -n "/can handle Basic/"

Oh yeah. I tested it with RBI generation which strangely succeeds but CI fails. Thanks.

KaanOzkan · 2025-03-14T18:18:03Z

lib/tapioca/rbs/rewriter.rb

+
+# We need to include `T::Sig` very early to make sure that the `sig` method is available since gems using RBS comments
+# are unlikely to include `T::Sig` in their own classes.
+Module.include(T::Sig)


No it should be fine. Rewriting itself could be an issue with the addon, I'll test it.

KaanOzkan · 2025-03-14T18:21:57Z

lib/tapioca/rbs/rewriter.rb

+      #: (String source) -> String
+      def rewrite(source)
+        # Do not try to parse files that don't have RBS comments
+        return source unless source =~ /^\s*#\s*typed: (ignore|false|true|strict|strong|__STDLIB_INTERNAL)/


We would gain significant performance improvement for tapioca gem with arguments if we could rewrite only the gems we are generating RBIs for. It would help with the performance loss of the early return when we match #: instead.

Morriar · 2025-03-14T18:43:32Z

We would gain significant performance improvement for tapioca gem with arguments if we could rewrite only the gems we are generating RBIs for. It would help with the performance loss of the early return when we match #: instead.

Yeah it's a good idea, though we would need to register the rewriter much later so we can have access to the list of gems which means no signatures for Tapioca itself.

If we want to go this route I think it would mean exporting the RBI for Tapioca itself with spoom srb sigs export like we do for rbi and spoom.

This would also solve the issue of the signatures I had to revert to sig { ...checked(:never) }.

Signed-off-by: Alexandre Terrasa <[email protected]>

…ame way Signed-off-by: Alexandre Terrasa <[email protected]>

Verseth · 2025-03-17T10:46:07Z

This would be great. I am incredibly excited to see RBS comments fully embraced by sorbet/tapioca.

My team is closely following your progress in that matter.
We've already started experimenting with RBS and we fell in love with it.

Good luck and cheers 👍

vinistock

The approach we're taking here is to do a full rewriter pass to translate RBS -> RBI and then letting Tapioca do its thing with no functional changes.

Would it be possible to change Tapioca to handle the RBS comments directly instead? For example, when we find a method, we could search its comments, find an RBS signature and then build a signature entry for it.

If this suggestion is feasible, I think (without actually benchmarking anything) we may be able to achieve better performance on top of avoiding the ruby-next dependency.

What do you think? Am I missing context?

Morriar · 2025-03-17T16:33:31Z

Would it be possible to change Tapioca to handle the RBS comments directly instead? For example, when we find a method, we could search its comments, find an RBS signature and then build a signature entry for it.

This was the idea with our first prototype to solve this issue.

First we need to access the comments which we can't from a pure runtime perspective and we need to rely on reparsing the file (indexer, yard, whatever).

Once we have the comment there is the issue of resolving constants. Consider this:

module Foo
  class Bar; end

  class Baz
    #: -> Bar
    def foo; end
  end
end

The way Tapioca is currently built, it creates a flattened view of classes so if we were to just translate the sig the result would look like this:

module Foo; end

class Foo::Bar; end

class Foo::Baz
  sig { returns(Bar) } # error: Unresolved constant `Bar`
  def foo; end
end

Since the code is already loaded, we can use the runtime to resolve the constants, something like this would work for example:

const = eval <<~RB
  module Foo
    class Baz
      Bar
    end
  end
RB

Finally with this information we can write the final sig. And that would work but it wouldn't solve the issue of DSL compilers that look for the method signature at runtime.

So what we could do at this point is use the same approach but instead of generating the sig as a RBI string, we could call into sorbet-runtime sig builder to instantiate the signature at runtime and re-wrap the method. Which in turn comes with a bunch of other issues since we reach into sorbet-runtime internals. And at this point we're doing almost the same thing than the approach used in this PR.

One interesting thing about the load time rewriter we use here is that we could in the future extract it as a standalone feature to re-enable runtime type checks in tests for example. Imagine using RBS comments, you could just require sorbet-runtime/rbs and get back runtime checks.

vinistock

Thanks for explaining the other considerations. I think using the indexer to detect the RBS signatures would be nice, but it seems like it would be a much more involved change that expands simply adopting it.

I would rather us move forward with solving the problem and then iterate on improving the performance / designing a longer term solution

vinistock · 2025-03-17T21:46:46Z

lib/tapioca/rbs/rewriter.rb

+RubyNext::Language.include_patterns.clear
+RubyNext::Language.include_patterns << "**/*.rb"


Why do these need to be cleared first? Does it include something other than Ruby files by default?

We could do

Suggested change

RubyNext::Language.include_patterns.clear

RubyNext::Language.include_patterns << "**/*.rb"

RubyNext::Language.include_patterns = ["**/*.rb"]

or just append if it's already empty.

vinistock · 2025-03-17T21:47:27Z

lib/tapioca/rbs/rewriter.rb

+  module RBS
+    # Transpiles RBS comments back into Sorbet's `sig{}` blocks
+    class Rewriter < RubyNext::Language::Rewriters::Text
+      NAME = "rbs_rewriter"


Out of curiosity, is this used by ruby-next?

Morriar and others added 6 commits March 13, 2025 16:09

Add dependency to ruby-next

c6f8239

Signed-off-by: Alexandre Terrasa <[email protected]>

Bump rbi to v0.3.1

7ca486e

Signed-off-by: Alexandre Terrasa <[email protected]>

Bump spoom to v1.6.1

c9591e8

Signed-off-by: Alexandre Terrasa <[email protected]>

Bump sorbet to v0.5.11930

4b11f0e

Signed-off-by: Alexandre Terrasa <[email protected]>

Consider <compiled> source locations

efda63e

Signed-off-by: Alexandre Terrasa <[email protected]> Co-authored-by: Ufuk Kayserilioglu <[email protected]>

Add runtime rewriter for RBS signature comments

8bed834

Signed-off-by: Alexandre Terrasa <[email protected]> Co-authored-by: Kaan Ozkan <[email protected]>

Morriar added the feature label Mar 14, 2025

Morriar self-assigned this Mar 14, 2025

Morriar requested a review from a team as a code owner March 14, 2025 14:18

Morriar commented Mar 14, 2025

View reviewed changes

Morriar added enhancement New feature or request and removed feature labels Mar 14, 2025

Morriar requested review from paracycle and KaanOzkan March 14, 2025 14:24

Morriar force-pushed the at-rbs-rewriter branch 2 times, most recently from 2b9454e to c116258 Compare March 14, 2025 17:18

KaanOzkan reviewed Mar 14, 2025

View reviewed changes

Morriar added 4 commits March 14, 2025 15:09

Revert sigs without runtime

fa9f056

Signed-off-by: Alexandre Terrasa <[email protected]>

Add test for DSL compilers using RBS comments

bd3f803

Signed-off-by: Alexandre Terrasa <[email protected]>

Reorder requires so the RBS rewriter gets loaded first

08d19f6

Signed-off-by: Alexandre Terrasa <[email protected]>

Update expectation on require since the file is not loaded in the s…

5995622

…ame way Signed-off-by: Alexandre Terrasa <[email protected]>

Morriar force-pushed the at-rbs-rewriter branch from 8151094 to 5995622 Compare March 14, 2025 19:10

vinistock reviewed Mar 17, 2025

View reviewed changes

vinistock approved these changes Mar 17, 2025

View reviewed changes

KaanOzkan approved these changes Mar 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for RBS signature comments #2236

Add support for RBS signature comments #2236

Morriar commented Mar 14, 2025

Morriar Mar 14, 2025

Morriar Mar 14, 2025

Morriar Mar 14, 2025

KaanOzkan Mar 14, 2025

KaanOzkan Mar 17, 2025

Morriar Mar 14, 2025

Morriar Mar 14, 2025

Morriar Mar 14, 2025

KaanOzkan Mar 14, 2025

Morriar Mar 14, 2025

KaanOzkan Mar 14, 2025

Morriar Mar 14, 2025

KaanOzkan Mar 14, 2025

Morriar Mar 14, 2025

KaanOzkan Mar 17, 2025

KaanOzkan Mar 14, 2025

KaanOzkan Mar 14, 2025

Morriar commented Mar 14, 2025

Verseth commented Mar 17, 2025

vinistock left a comment

Morriar commented Mar 17, 2025

vinistock left a comment

vinistock Mar 17, 2025

vinistock Mar 17, 2025

		RubyNext::Language.include_patterns.clear
		RubyNext::Language.include_patterns << "*/.rb"

	RubyNext::Language.include_patterns.clear
	RubyNext::Language.include_patterns << "*/.rb"
	RubyNext::Language.include_patterns = ["*/.rb"]

Add support for RBS signature comments #2236

Are you sure you want to change the base?

Add support for RBS signature comments #2236

Conversation

Morriar commented Mar 14, 2025

Motivation

Implementation

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morriar commented Mar 14, 2025

Verseth commented Mar 17, 2025

vinistock left a comment

Choose a reason for hiding this comment

Morriar commented Mar 17, 2025

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment