Skip to content

Skip RBS rewriter when file does not contain RBS syntax #917

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

Merged
dejmedus merged 3 commits into from
May 14, 2026
Merged

Skip RBS rewriter when file does not contain RBS syntax #917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3862c1
Skip RBS rewrite when file contains no RBS syntax
dejmedus May 12, 2026
ca8a1fd
Add typed: true to test example files
dejmedus May 12, 2026
5d5eb6a
Don't count unchanged files in translated file number
dejmedus May 12, 2026
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
6 changes: 4 additions & 2 deletions lib/spoom/cli/srb/sigs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,10 @@ def transform_files(files, &block)
contents = contents.force_encoding(encoding)
end

contents = block.call(file, contents)
File.write(file, contents)
new_contents = block.call(file, contents)
next if new_contents == contents

File.write(file, new_contents)
transformed_count += 1
Copy link
Copy Markdown
Contributor Author

@dejmedus dejmedus May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This will only count changed files in the Translated signatures in <x> files. message, but I'm happy to drop the commit if we want to leave as is

Copy link
Copy Markdown
Contributor

@amomchilov amomchilov May 12, 2026

Choose a reason for hiding this comment

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

I like it.

rescue RBI::Error => error
say_warning("Can't parse #{file}: #{error.message}")
Expand Down
7 changes: 7 additions & 0 deletions lib/spoom/sorbet/sigils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ def strictness_in_content(content)
SIGIL_REGEXP.match(content)&.[](1)
end

# returns true if the passed content contains a valid sigil
#: (String content) -> bool
def contains_valid_sigil?(content)
strictness = strictness_in_content(content)
!!strictness && valid_strictness?(strictness)
end

# returns a string which is the passed content but with the sigil updated to a new strictness
#: (String content, String new_strictness) -> String
def update_sigil(content, new_strictness)
Expand Down
2 changes: 1 addition & 1 deletion lib/spoom/sorbet/translate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def sorbet_sigs_to_rbs_comments(
# It also handles type members and class annotations.
#: (String ruby_contents, file: String, ?max_line_length: Integer?) -> String
def rbs_comments_to_sorbet_sigs(ruby_contents, file:, max_line_length: nil)
RBSCommentsToSorbetSigs.new(ruby_contents, file: file, max_line_length: max_line_length).rewrite
RBSCommentsToSorbetSigs.rewrite_if_needed(ruby_contents, file: file, max_line_length: max_line_length)
end

# Converts all `T.let` and `T.cast` nodes to RBS comments in the given Ruby code.
Expand Down
27 changes: 27 additions & 0 deletions lib/spoom/sorbet/translate/rbs_comments_to_sorbet_sigs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,33 @@ module Translate
class RBSCommentsToSorbetSigs < Translator
include Spoom::RBS::ExtractRBSComments

RBS_ANNOTATION_MARKERS = [
"# @abstract",
"# @interface",
"# @sealed",
"# @final",
"# @requires_ancestor:",
"# @override",
"# @overridable",
Comment on lines +16 to +17
Copy link
Copy Markdown
Contributor

@amomchilov amomchilov May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Random thought I had: does factoring out the common prefixes produce a more faster regular expression? E.g. overrid(?:e|able) instead of checking for the two whole words separately.

In short, yes, but barely. Onigmo doesn't optimize out common prefixes in the NFA it creates, but it doesn't make a real world difference here.

Got Claude to benchmark it for me:

require "benchmark"

# loading corpus... 101996 files, 616MB, loaded in 15.09s
# sanity-checking match equivalence... 19104 total matches across corpus
#
#                     # user     system      total        real
# flat              0.536375   0.006953   0.543328 (  0.552176)
# factored          0.530740   0.005332   0.536072 (  0.544158)
#
# factored is 1.01x faster than flat (real time)

MARKERS = [
  "# @abstract",
  "# @interface",
  "# @sealed",
  "# @final",
  "# @requires_ancestor:",
  "# @override",
  "# @overridable",
  "# @without_runtime",
].freeze


FLAT     = Regexp.union(*MARKERS)
FACTORED = /\# @(?:abstract|interface|sealed|final|requires_ancestor:|overrid(?:e|able)|without_runtime)/

raise "regexes diverge on hits"   unless MARKERS.all? { |m| FLAT =~ m && FACTORED =~ m }
raise "regexes diverge on misses" if FLAT =~ "# @other" || FACTORED =~ "# @other"

CORPUS_GLOB = "/your/test/corpus/**/*.rb"

print "loading corpus... "
load_start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
CORPUS = Dir.glob(CORPUS_GLOB).map { |path| File.read(path) rescue nil }.compact.freeze
load_elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - load_start
total_bytes = CORPUS.sum(&:bytesize)
puts "#{CORPUS.size} files, #{total_bytes / 1_000_000}MB, loaded in #{load_elapsed.round(2)}s"

print "sanity-checking match equivalence... "
flat_hits     = CORPUS.sum { |s| s.scan(FLAT).size }
factored_hits = CORPUS.sum { |s| s.scan(FACTORED).size }
raise "regexes diverge: flat=#{flat_hits} factored=#{factored_hits}" unless flat_hits == factored_hits
puts "#{flat_hits} total matches across corpus"

flat_time     = Benchmark.measure("flat")     { CORPUS.each { |s| s.scan(FLAT) } }
factored_time = Benchmark.measure("factored") { CORPUS.each { |s| s.scan(FACTORED) } }

slower, faster = [flat_time, factored_time].sort_by(&:real).reverse
ratio = slower.real / faster.real
faster_name = faster.equal?(flat_time) ? "flat" : "factored"
slower_name = slower.equal?(flat_time) ? "flat" : "factored"

puts <<~RESULTS

                  #{Benchmark::CAPTION}
  flat            #{flat_time.to_s.strip}
  factored        #{factored_time.to_s.strip}

  #{faster_name} is #{ratio.round(2)}x faster than #{slower_name} (real time)
RESULTS

Copy link
Copy Markdown
Contributor Author

@dejmedus dejmedus May 14, 2026

Choose a reason for hiding this comment

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

factored is 1.01x faster than flat

Interesting!

"# @without_runtime",
].freeze #: Array[String]
RBS_REWRITE_PATTERN = Regexp.union(["#:", "#|", *RBS_ANNOTATION_MARKERS]).freeze #: Regexp
private_constant :RBS_ANNOTATION_MARKERS, :RBS_REWRITE_PATTERN

class << self
#: (String source) -> bool
def contains_rbs_syntax?(source)
Sigils.contains_valid_sigil?(source) && source.match?(RBS_REWRITE_PATTERN)
end

#: (String ruby_contents, file: String, ?max_line_length: Integer?) -> String
def rewrite_if_needed(ruby_contents, file:, max_line_length: nil)
return ruby_contents unless contains_rbs_syntax?(ruby_contents)

new(ruby_contents, file:, max_line_length:).rewrite
end
end

#: (String, file: String, ?max_line_length: Integer?) -> void
def initialize(ruby_contents, file:, max_line_length: nil)
super(ruby_contents, file: file)
Expand Down
14 changes: 14 additions & 0 deletions rbi/spoom.rbi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2823,6 +2823,9 @@ module Spoom::Sorbet::Sigils
sig { params(path_list: T::Array[::String], new_strictness: ::String).returns(T::Array[::String]) }
def change_sigil_in_files(path_list, new_strictness); end

sig { params(content: ::String).returns(T::Boolean) }
def contains_valid_sigil?(content); end

sig { params(path: T.any(::Pathname, ::String)).returns(T.nilable(::String)) }
def file_strictness(path); end

Expand Down Expand Up @@ -2938,8 +2941,19 @@ class Spoom::Sorbet::Translate::RBSCommentsToSorbetSigs < ::Spoom::Sorbet::Trans

sig { params(node: ::Prism::CallNode).void }
def visit_attr(node); end

class << self
sig { params(source: ::String).returns(T::Boolean) }
def contains_rbs_syntax?(source); end

sig { params(ruby_contents: ::String, file: ::String, max_line_length: T.nilable(::Integer)).returns(::String) }
def rewrite_if_needed(ruby_contents, file:, max_line_length: T.unsafe(nil)); end
end
end

Spoom::Sorbet::Translate::RBSCommentsToSorbetSigs::RBS_ANNOTATION_MARKERS = T.let(T.unsafe(nil), Array)
Copy link
Copy Markdown
Contributor

@amomchilov amomchilov May 12, 2026

Choose a reason for hiding this comment

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

Let's mark these with private_constant where they're defined, so we don't expose them to the public.

Spoom::Sorbet::Translate::RBSCommentsToSorbetSigs::RBS_REWRITE_PATTERN = T.let(T.unsafe(nil), Regexp)

class Spoom::Sorbet::Translate::SorbetAssertionsToRBSComments < ::Spoom::Sorbet::Translate::Translator
sig do
params(
Expand Down
36 changes: 36 additions & 0 deletions test/spoom/cli/srb/sigs_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,24 @@ def test_translate_no_files
refute(result.status)
end

def test_translate_does_not_count_files_with_nothing_to_translate
@project.write!("file.rb", <<~RB)
# typed: true

def foo; end
RB

result = @project.spoom("srb sigs translate --from rbs --to rbi --no-color")

assert_empty(result.err)
assert(result.status)
assert_equal(<<~OUT, result.out)
Translating signatures from `rbs` to `rbi` in `1` file...

Translated signatures in `0` files.
OUT
end

def test_translate_only_selected_files
@project.write!("a/file1.rb", <<~RB)
sig { void }
Expand Down Expand Up @@ -351,6 +369,8 @@ def foo = raise NotImplementedError, "Abstract method called"

def test_translate_from_rbs_to_rbi
@project.write!("file.rb", <<~RB)
# typed: true

#: -> void
def foo; end
RB
Expand All @@ -366,13 +386,17 @@ def foo; end
assert(result.status)

assert_equal(<<~RB, @project.read("file.rb"))
# typed: true

sig { void }
def foo; end
RB
end

def test_translate_to_rbi_with_max_line_length_by_default
@project.write!("file.rb", <<~RB)
# typed: true

#: (
#| param1: AVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongType,
#| param2: AVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongType
Expand All @@ -386,6 +410,8 @@ def foo(param1:, param2:); end
assert(result.status)

assert_equal(<<~RB, @project.read("file.rb"))
# typed: true

sig do
params(
param1: AVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongType,
Expand All @@ -398,6 +424,8 @@ def foo(param1:, param2:); end

def test_translate_to_rbi_without_max_line_length
@project.write!("file.rb", <<~RB)
# typed: true

#: (
#| param1: AVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongType,
#| param2: AVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongType
Expand All @@ -411,6 +439,8 @@ def foo(param1:, param2:); end
assert(result.status)

assert_equal(<<~RB, @project.read("file.rb"))
# typed: true

sig { params(param1: AVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongType, param2: AVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongType).void }
def foo(param1:, param2:); end
RB
Expand Down Expand Up @@ -444,6 +474,8 @@ def test_export_cant_locate_entry_point
def test_export_create_rbi_file
@project.write!("foo.gemspec", GEMSPEC)
@project.write!("lib/foo.rb", <<~RB)
# typed: true

class Foo
# ignored comment
#: -> void
Expand Down Expand Up @@ -473,6 +505,8 @@ def foo; end
def test_export_check_sync_raises_if_rbi_is_not_up_to_date
@project.write!("foo.gemspec", GEMSPEC)
@project.write!("lib/foo.rb", <<~RB)
# typed: true

class Foo
#: -> void
def bar; end
Expand Down Expand Up @@ -524,6 +558,8 @@ def foo; end
def test_export_check_sync_does_not_raise_if_rbi_is_up_to_date
@project.write!("foo.gemspec", GEMSPEC)
@project.write!("lib/foo.rb", <<~RB)
# typed: true

class Foo
#: -> void
def bar; end
Expand Down
6 changes: 6 additions & 0 deletions test/spoom/sorbet/sigils_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ class A; end
refute(Sigils.valid_strictness?(T.must(strictness)))
end

def test_contains_valid_sigil
assert(Sigils.contains_valid_sigil?("# typed: true\nclass A; end\n"))
refute(Sigils.contains_valid_sigil?("class A; end\n"))
refute(Sigils.contains_valid_sigil?("# typed: foo\nclass A; end\n"))
end

def test_update_sigil_to_use_valid_strictness
content = <<~STR
# typed: ignore
Expand Down
137 changes: 136 additions & 1 deletion test/spoom/sorbet/translate/rbs_comments_to_sorbet_sigs_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,11 +678,146 @@ class Range
RB
end

def test_contains_rbs_syntax_returns_true_for_supported_rbs_annotations
[
"# @abstract",
"# @interface",
"# @sealed",
"# @final",
"# @requires_ancestor:",
"# @override",
"# @override(allow_incompatible: true)",
"# @override(allow_incompatible: false)",
"# @override(allow_incompatible: :visibility)",
"# @overridable",
"# @without_runtime",
].each do |marker|
assert(
RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB),
# typed: true

#{marker}
class Foo; end
RB
"#contains_rbs_syntax? should return true for files containing #{marker}",
)
end
end

def test_contains_rbs_syntax_returns_true_for_supported_typed_sigils
[
"# typed: ignore",
"# typed: false",
"# typed: true",
"# typed: strict",
"# typed: strong",
"# typed: __STDLIB_INTERNAL",
].each do |sigil|
assert(
RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB),
#{sigil}

#: -> void
def foo; end
RB
"#contains_rbs_syntax? should return true for files containing #{sigil}",
)
end
end

def test_contains_rbs_syntax_returns_true_when_typed_sigil_is_after_other_magic_comments
assert(RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB))
# frozen_string_literal: true
# typed: true

class Foo
#: -> String
def foo; end
end
RB

assert(RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB))
# frozen_string_literal: true

# typed: true

class Foo
#: -> String
def foo; end
end
RB
end

def test_contains_rbs_syntax_returns_true_for_rbs_comments
assert(RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB))
# typed: true

class Foo
#: -> String
def foo; end
end
RB
end

def test_contains_rbs_syntax_returns_true_for_multiline_rbs_comments
assert(RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB))
# typed: true

class Foo
#: -> Array[
#| String
#| ]
def foo; end
end
RB
end

def test_contains_rbs_syntax_returns_false_for_files_without_typed_sigil
refute(RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB))
#: -> void
def foo; end
RB
end

def test_contains_rbs_syntax_returns_false_for_files_without_rbs_syntax
refute(RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB))
# typed: true

class Foo
def foo; end
end
RB
end

def test_contains_rbs_syntax_returns_false_for_unrelated_yard_tags
Copy link
Copy Markdown
Contributor

@amomchilov amomchilov May 13, 2026

Choose a reason for hiding this comment

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

Good call

refute(RBSCommentsToSorbetSigs.contains_rbs_syntax?(<<~RB))
# typed: true

# @param value [String]
# @return [String]
def foo(value); end
RB
end

def test_rewrite_does_not_call_new_for_files_without_rbs_syntax
source = <<~RB
# typed: true

class Foo
def foo; end
end
RB

RBSCommentsToSorbetSigs.stub(:new, ->(*) { flunk("should not be called") }) do
assert_equal(source, RBSCommentsToSorbetSigs.rewrite_if_needed(source, file: "test.rb"))
end
end

private

#: (String, ?max_line_length: Integer?) -> String
def rbs_comments_to_sorbet_sigs(ruby_contents, max_line_length: nil)
Translate.rbs_comments_to_sorbet_sigs(ruby_contents, file: "test.rb", max_line_length: max_line_length)
RBSCommentsToSorbetSigs.new(ruby_contents, file: "test.rb", max_line_length: max_line_length).rewrite
end
end
end
Expand Down
Loading