Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for RBS signature comments #2236

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add support for RBS signature comments #2236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c6f8239
Add dependency to `ruby-next`
Morriar Mar 3, 2025
7ca486e
Bump rbi to v0.3.1
Morriar Mar 13, 2025
c9591e8
Bump spoom to v1.6.1
Morriar Mar 13, 2025
4b11f0e
Bump sorbet to v0.5.11930
Morriar Mar 13, 2025
efda63e
Consider `<compiled>` source locations
Morriar Mar 11, 2025
8bed834
Add runtime rewriter for RBS signature comments
Morriar Mar 11, 2025
fa9f056
Revert sigs without runtime
Morriar Mar 14, 2025
bd3f803
Add test for DSL compilers using RBS comments
Morriar Mar 14, 2025
08d19f6
Reorder requires so the RBS rewriter gets loaded first
Morriar Mar 14, 2025
5995622
Update expectation on `require` since the file is not loaded in the s…
Morriar Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ PATH
bundler (>= 2.2.25)
netrc (>= 0.11.0)
parallel (>= 1.21.0)
rbi (~> 0.2)
rbi (>= 0.3.1)
ruby-next (~> 1.0)
sorbet-static-and-runtime (>= 0.5.11087)
spoom (>= 1.2.0)
spoom (>= 1.6.1)
thor (>= 1.2.0)
yard-sorbet

Expand Down Expand Up @@ -130,6 +131,7 @@ GEM
irb (~> 1.10)
reline (>= 0.3.8)
deep_merge (1.2.2)
diff-lcs (1.6.0)
drb (2.2.1)
erubi (1.13.1)
faraday (2.9.0)
Expand Down Expand Up @@ -233,6 +235,7 @@ GEM
nokogiri (1.18.3-x86_64-linux-musl)
racc (~> 1.4)
ostruct (0.6.0)
paco (0.2.3)
parallel (1.26.3)
parser (3.3.7.1)
ast (~> 2.4.1)
Expand Down Expand Up @@ -284,7 +287,7 @@ GEM
zeitwerk (~> 2.6)
rainbow (3.1.1)
rake (13.2.1)
rbi (0.3.0)
rbi (0.3.1)
prism (~> 1.0)
rbs (>= 3.4.4)
sorbet-runtime (>= 0.5.9204)
Expand All @@ -299,6 +302,7 @@ GEM
regexp_parser (2.10.0)
reline (0.6.0)
io-console (~> 0.5)
require-hooks (0.2.2)
rexml (3.4.1)
rubocop (1.73.2)
json (~> 2.3)
Expand Down Expand Up @@ -328,6 +332,15 @@ GEM
sorbet-runtime (>= 0.5.10782)
ruby-lsp-rails (0.4.0)
ruby-lsp (>= 0.23.0, < 0.24.0)
ruby-next (1.1.1)
paco (~> 0.2)
require-hooks (~> 0.2)
ruby-next-core (= 1.1.1)
ruby-next-parser (>= 3.4.0.2)
unparser (~> 0.6.0)
ruby-next-core (1.1.1)
ruby-next-parser (3.4.0.2)
parser (>= 3.0.3.1)
ruby-progressbar (1.13.0)
securerandom (0.4.0)
shopify-money (3.0.2)
Expand All @@ -338,16 +351,16 @@ GEM
rack (>= 2.2.4)
redis-client (>= 0.22.2)
smart_properties (1.17.0)
sorbet (0.5.11911)
sorbet-static (= 0.5.11911)
sorbet-runtime (0.5.11911)
sorbet-static (0.5.11911-aarch64-linux)
sorbet-static (0.5.11911-universal-darwin)
sorbet-static (0.5.11911-x86_64-linux)
sorbet-static-and-runtime (0.5.11911)
sorbet (= 0.5.11911)
sorbet-runtime (= 0.5.11911)
spoom (1.6.0)
sorbet (0.5.11930)
sorbet-static (= 0.5.11930)
sorbet-runtime (0.5.11930)
sorbet-static (0.5.11930-aarch64-linux)
sorbet-static (0.5.11930-universal-darwin)
sorbet-static (0.5.11930-x86_64-linux)
sorbet-static-and-runtime (0.5.11930)
sorbet (= 0.5.11930)
sorbet-runtime (= 0.5.11930)
spoom (1.6.1)
erubi (>= 1.10.0)
prism (>= 0.28.0)
rbi (>= 0.2.3)
Expand All @@ -371,6 +384,9 @@ GEM
unicode-display_width (3.1.4)
unicode-emoji (~> 4.0, >= 4.0.4)
unicode-emoji (4.0.4)
unparser (0.6.15)
diff-lcs (~> 1.3)
parser (>= 3.3.0)
uri (0.13.2)
webmock (3.25.1)
addressable (>= 2.8.0)
Expand Down
2 changes: 1 addition & 1 deletion lib/tapioca/bundler_ext/auto_require_hook.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def override_require_false(exclude:, &blk)
end
end

#: -> untyped
sig { returns(T.untyped).checked(:never) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def autorequire
value = super

Expand Down
23 changes: 16 additions & 7 deletions lib/tapioca/gem/events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ class ConstantFound < Event
#: String
attr_reader :symbol

#: BasicObject
sig { returns(BasicObject).checked(:never) }
attr_reader :constant

#: (String symbol, BasicObject constant) -> void
sig { params(symbol: String, constant: BasicObject).void.checked(:never) }
def initialize(symbol, constant)
super()
@symbol = symbol
Expand Down Expand Up @@ -64,10 +64,10 @@ class NodeAdded < Event
#: String
attr_reader :symbol

#: Module
sig { returns(Module).checked(:never) }
attr_reader :constant

#: (String symbol, Module constant) -> void
sig { params(symbol: String, constant: Module).void.checked(:never) }
def initialize(symbol, constant)
super()
@symbol = symbol
Expand All @@ -81,7 +81,7 @@ class ConstNodeAdded < NodeAdded
#: RBI::Const
attr_reader :node

#: (String symbol, Module constant, RBI::Const node) -> void
sig { params(symbol: String, constant: Module, node: RBI::Const).void.checked(:never) }
def initialize(symbol, constant, node)
super(symbol, constant)
@node = node
Expand All @@ -94,7 +94,7 @@ class ScopeNodeAdded < NodeAdded
#: RBI::Scope
attr_reader :node

#: (String symbol, Module constant, RBI::Scope node) -> void
sig { params(symbol: String, constant: Module, node: RBI::Scope).void.checked(:never) }
def initialize(symbol, constant, node)
super(symbol, constant)
@node = node
Expand All @@ -118,7 +118,16 @@ class MethodNodeAdded < NodeAdded
#: Array[[Symbol, String]]
attr_reader :parameters

#: (String symbol, Module constant, UnboundMethod method, RBI::Method node, untyped signature, Array[[Symbol, String]] parameters) -> void
sig do
params(
symbol: String,
constant: Module,
method: UnboundMethod,
node: RBI::Method,
signature: T.untyped,
parameters: T::Array[[Symbol, String]],
).void.checked(:never)
end
def initialize(symbol, constant, method, node, signature, parameters) # rubocop:disable Metrics/ParameterLists
super(symbol, constant)
@node = node
Expand Down
29 changes: 19 additions & 10 deletions lib/tapioca/gem/pipeline.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,32 +66,41 @@ def push_symbol(symbol)
@events << Gem::SymbolFound.new(symbol)
end

#: (String symbol, BasicObject constant) -> void
sig { params(symbol: String, constant: BasicObject).void.checked(:never) }
def push_constant(symbol, constant)
@events << Gem::ConstantFound.new(symbol, constant)
end

#: (String symbol, Module constant) -> void
sig { params(symbol: String, constant: Module).void.checked(:never) }
def push_foreign_constant(symbol, constant)
@events << Gem::ForeignConstantFound.new(symbol, constant)
end

#: (String symbol, Module constant, RBI::Const node) -> void
sig { params(symbol: String, constant: Module, node: RBI::Const).void.checked(:never) }
def push_const(symbol, constant, node)
@events << Gem::ConstNodeAdded.new(symbol, constant, node)
end

#: (String symbol, Module constant, RBI::Scope node) -> void
sig { params(symbol: String, constant: Module, node: RBI::Scope).void.checked(:never) }
def push_scope(symbol, constant, node)
@events << Gem::ScopeNodeAdded.new(symbol, constant, node)
end

#: (String symbol, Module constant, RBI::Scope node) -> void
sig { params(symbol: String, constant: Module, node: RBI::Scope).void.checked(:never) }
def push_foreign_scope(symbol, constant, node)
@events << Gem::ForeignScopeNodeAdded.new(symbol, constant, node)
end

#: (String symbol, Module constant, UnboundMethod method, RBI::Method node, untyped signature, Array[[Symbol, String]] parameters) -> void
sig do
params(
symbol: String,
constant: Module,
method: UnboundMethod,
node: RBI::Method,
signature: T.untyped,
parameters: T::Array[[Symbol, String]],
).void.checked(:never)
end
def push_method(symbol, constant, method, node, signature, parameters) # rubocop:disable Metrics/ParameterLists
@events << Gem::MethodNodeAdded.new(symbol, constant, method, node, signature, parameters)
end
Expand Down Expand Up @@ -222,7 +231,7 @@ def compile_foreign_constant(symbol, constant)
push_foreign_scope(symbol, constant, scope)
end

#: (String symbol, BasicObject constant) -> void
sig { params(symbol: String, constant: BasicObject).void.checked(:never) }
def compile_constant(symbol, constant)
case constant
when Module
Expand Down Expand Up @@ -257,7 +266,7 @@ def compile_alias(name, constant)
@root << node
end

#: (String name, BasicObject value) -> void
sig { params(name: String, value: BasicObject).void.checked(:never) }
def compile_object(name, value)
return if seen?(name)

Expand Down Expand Up @@ -371,7 +380,7 @@ def skip_symbol?(name)
symbol_in_payload?(name) && !@bootstrap_symbols.include?(name)
end

#: (String name, top constant) -> bool
sig { params(name: String, constant: T.untyped).returns(T::Boolean).checked(:never) }
def skip_constant?(name, constant)
return true if name.strip.empty?
return true if name.start_with?("#<")
Expand All @@ -392,7 +401,7 @@ def skip_alias?(name, constant)
false
end

#: (String name, BasicObject constant) -> bool
sig { params(name: String, constant: BasicObject).returns(T::Boolean).checked(:never) }
def skip_object?(name, constant)
return true if symbol_in_payload?(name)
return true unless constant_in_gem?(name)
Expand Down
6 changes: 5 additions & 1 deletion lib/tapioca/internal.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
require "tapioca/runtime/reflection"
require "tapioca/runtime/trackers"

# The rewriter needs to be loaded very early so RBS comments within Tapioca itself are rewritten
require "spoom"
require "tapioca/rbs/rewriter"
# ^ Do not change the order of these requires

require "benchmark"
require "bundler"
require "erb"
Expand All @@ -32,7 +37,6 @@
require "tapioca/sorbet_ext/proc_bind_patch"
require "tapioca/runtime/generic_type_registry"

require "spoom"
require "tapioca/helpers/gem_helper"
require "tapioca/helpers/git_attributes"
require "tapioca/helpers/sorbet_helper"
Expand Down
4 changes: 2 additions & 2 deletions lib/tapioca/loaders/loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def engines
return [] unless defined?(Rails::Engine)

Expand Down Expand Up @@ -216,7 +216,7 @@ def require_helper(file)
# The `eager_load_paths` method still exists, but doesn't return all paths anymore and causes Tapioca to miss some
# engine paths. The following commit is the change:
# https://github.com/rails/rails/commit/ebfca905db14020589c22e6937382e6f8f687664
#: (singleton(Rails::Engine) engine) -> Array[String]
T::Sig::WithoutRuntime.sig { params(engine: T.class_of(Rails::Engine)).returns(T::Array[String]) }
def eager_load_paths(engine)
config = engine.config

Expand Down
34 changes: 34 additions & 0 deletions lib/tapioca/rbs/rewriter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# typed: strict
# frozen_string_literal: true

require "ruby-next/language/runtime"

# This module rewrites RBS comments back into Sorbet's signatures as the files are being loaded.
# This will allow `sorbet-runtime` to wrap the methods as if they were originally written with the `sig{}` blocks.
# This will in turn allow Tapioca to use this signatures to generate typed RBI files.

# 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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems okay on a first glance, not blocking.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#: (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)/
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


context.track!(self)
Spoom::Sorbet::Sigs.rbs_to_rbi(source)
end
end
end
end

RubyNext::Language.include_patterns.clear
RubyNext::Language.include_patterns << "**/*.rb"
Comment on lines +32 to +33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RubyNext::Language.rewriters = [Tapioca::RBS::Rewriter]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

12 changes: 6 additions & 6 deletions lib/tapioca/runtime/reflection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,21 @@ module Reflection
METHOD_METHOD = T.let(Kernel.instance_method(:method), UnboundMethod)
UNDEFINED_CONSTANT = T.let(Module.new.freeze, Module)

REQUIRED_FROM_LABELS = T.let(["<top (required)>", "<main>"].freeze, T::Array[String])
REQUIRED_FROM_LABELS = T.let(["<top (required)>", "<main>", "<compiled>"].freeze, T::Array[String])

#: (BasicObject constant) -> bool
T::Sig::WithoutRuntime.sig { params(constant: BasicObject).returns(T::Boolean) }
def constant_defined?(constant)
!UNDEFINED_CONSTANT.eql?(constant)
end

#: (String symbol, ?inherit: bool, ?namespace: Module) -> BasicObject
sig { params(symbol: String, inherit: T::Boolean, namespace: Module).returns(BasicObject).checked(:never) }
def constantize(symbol, inherit: false, namespace: Object)
namespace.const_get(symbol, inherit)
rescue NameError, LoadError, RuntimeError, ArgumentError, TypeError
UNDEFINED_CONSTANT
end

#: (BasicObject object) -> Class[top]
sig { params(object: BasicObject).returns(T::Class[T.anything]).checked(:never) }
def class_of(object)
CLASS_METHOD.bind_call(object)
end
Expand Down Expand Up @@ -77,12 +77,12 @@ def superclass_of(constant)
SUPERCLASS_METHOD.bind_call(constant)
end

#: (BasicObject object) -> Integer
sig { params(object: BasicObject).returns(Integer).checked(:never) }
def object_id_of(object)
OBJECT_ID_METHOD.bind_call(object)
end

#: (BasicObject object, BasicObject other) -> bool
sig { params(object: BasicObject, other: BasicObject).returns(T::Boolean).checked(:never) }
def are_equal?(object, other)
EQUAL_METHOD.bind_call(object, other)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/tapioca/static/symbol_loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def symbols_from_paths(paths)

private

#: -> Array[singleton(Rails::Engine)]
T::Sig::WithoutRuntime.sig { returns(T::Array[T.class_of(Rails::Engine)]) }
def engines
@engines ||= T.let(
if Object.const_defined?("Rails::Engine")
Expand Down
Loading
Loading