From 292f17f05a7e7862b2f5f72210bb12c09e82bee5 Mon Sep 17 00:00:00 2001 From: Kaan Ozkan Date: Tue, 7 Jul 2026 16:15:59 -0400 Subject: [PATCH] Preserve Sorbet warnings in formatted srb output Capture leading Sorbet stderr lines while parsing typecheck output so formatted `spoom srb tc` and `spoom srb bump` output no longer drops warnings emitted before errors. Assisted-By: devx/eb25d669-b4ee-4e28-9b69-129b45e1e178 --- lib/spoom/cli/srb/bump.rb | 10 +++++- lib/spoom/cli/srb/tc.rb | 6 +++- lib/spoom/sorbet/errors.rb | 42 +++++++++++++++++++++++-- rbi/spoom.rbi | 20 ++++++++++++ test/spoom/cli/srb/bump_test.rb | 53 ++++++++++++++++++++++++++++++++ test/spoom/cli/srb/tc_test.rb | 18 +++++++++++ test/spoom/sorbet/errors_test.rb | 27 ++++++++++++++++ 7 files changed, 171 insertions(+), 5 deletions(-) diff --git a/lib/spoom/cli/srb/bump.rb b/lib/spoom/cli/srb/bump.rb index eb0a8506..c3d48263 100644 --- a/lib/spoom/cli/srb/bump.rb +++ b/lib/spoom/cli/srb/bump.rb @@ -135,6 +135,10 @@ def bump(directory = ".") end if result.status + parse_result = Sorbet::Errors::Parser.parse_result(result.err, error_url_base: error_url_base) + parse_result.warnings.each do |warning| + say_error(warning, status: nil, nl: false) + end Sorbet::Sigils.change_sigil_in_files(files_to_bump, to) unless dry print_changes(files_to_bump, command: cmd, from: from, to: to, dry: dry, path: exec_path) exit(files_to_bump.empty?) @@ -147,7 +151,11 @@ def bump(directory = ".") exit(1) end - errors = Sorbet::Errors::Parser.parse_string(result.err, error_url_base: error_url_base) + parse_result = Sorbet::Errors::Parser.parse_result(result.err, error_url_base: error_url_base) + parse_result.warnings.each do |warning| + say_error(warning, status: nil, nl: false) + end + errors = parse_result.errors all_files = errors.flat_map do |err| [err.file, *err.files_from_error_sections] diff --git a/lib/spoom/cli/srb/tc.rb b/lib/spoom/cli/srb/tc.rb index baeec0e6..f6639518 100644 --- a/lib/spoom/cli/srb/tc.rb +++ b/lib/spoom/cli/srb/tc.rb @@ -73,7 +73,11 @@ def tc(*paths_to_select) exit(1) end - errors = Spoom::Sorbet::Errors::Parser.parse_string(result.err, error_url_base: error_url_base) + parse_result = Spoom::Sorbet::Errors::Parser.parse_result(result.err, error_url_base: error_url_base) + parse_result.warnings.each do |warning| + say_error(warning, status: nil, nl: false) + end + errors = parse_result.errors errors_count = errors.size errors = errors.select { |e| e.code == code } if code diff --git a/lib/spoom/sorbet/errors.rb b/lib/spoom/sorbet/errors.rb index 17ef9aff..12444c64 100644 --- a/lib/spoom/sorbet/errors.rb +++ b/lib/spoom/sorbet/errors.rb @@ -56,22 +56,35 @@ class ParseError < Spoom::Error; end ] #: Array[String] class << self + # Used when only Sorbet errors are needed and leading stderr warnings can be ignored. #: (String output, ?error_url_base: String) -> Array[Error] def parse_string(output, error_url_base: DEFAULT_ERROR_URL_BASE) + parse_result(output, error_url_base: error_url_base).errors + end + + # Used when callers need both parsed Sorbet errors and leading stderr warnings. + #: (String output, ?error_url_base: String) -> ParseResult + def parse_result(output, error_url_base: DEFAULT_ERROR_URL_BASE) parser = Spoom::Sorbet::Errors::Parser.new(error_url_base: error_url_base) - parser.parse(output) + parser.parse_result(output) end end #: (?error_url_base: String) -> void def initialize(error_url_base: DEFAULT_ERROR_URL_BASE) @errors = [] #: Array[Error] + @warnings = [] #: Array[String] @error_line_match_regex = error_line_match_regexp(error_url_base) #: Regexp @current_error = nil #: Error? end #: (String output) -> Array[Error] def parse(output) + parse_result(output).errors + end + + #: (String output) -> ParseResult + def parse_result(output) output.each_line do |line| break if /^No errors! Great job\./.match?(line) break if /^Errors: /.match?(line) @@ -85,10 +98,14 @@ def parse(output) next end - append_error(line) if @current_error + if @current_error + append_error(line) + else + append_warning(line) + end end close_error if @current_error - @errors + ParseResult.new(@errors, @warnings) end private @@ -144,6 +161,11 @@ def append_error(line) end @current_error.more << line end + + #: (String warning) -> void + def append_warning(warning) + @warnings << warning + end end class Error @@ -214,6 +236,20 @@ def to_junit_xml_element testcase_element end end + + class ParseResult + #: Array[Error] + attr_reader :errors + + #: Array[String] + attr_reader :warnings + + #: (Array[Error] errors, Array[String] warnings) -> void + def initialize(errors, warnings) + @errors = errors + @warnings = warnings + end + end end end end diff --git a/rbi/spoom.rbi b/rbi/spoom.rbi index 5ad95fc9..921eb97a 100644 --- a/rbi/spoom.rbi +++ b/rbi/spoom.rbi @@ -2753,6 +2753,17 @@ class Spoom::Sorbet::Errors::Error def to_s; end end +class Spoom::Sorbet::Errors::ParseResult + sig { params(errors: T::Array[::Spoom::Sorbet::Errors::Error], warnings: T::Array[::String]).void } + def initialize(errors, warnings); end + + sig { returns(T::Array[::Spoom::Sorbet::Errors::Error]) } + def errors; end + + sig { returns(T::Array[::String]) } + def warnings; end +end + class Spoom::Sorbet::Errors::Parser sig { params(error_url_base: ::String).void } def initialize(error_url_base: T.unsafe(nil)); end @@ -2760,11 +2771,17 @@ class Spoom::Sorbet::Errors::Parser sig { params(output: ::String).returns(T::Array[::Spoom::Sorbet::Errors::Error]) } def parse(output); end + sig { params(output: ::String).returns(::Spoom::Sorbet::Errors::ParseResult) } + def parse_result(output); end + private sig { params(line: ::String).void } def append_error(line); end + sig { params(warning: ::String).void } + def append_warning(warning); end + sig { void } def close_error; end @@ -2778,6 +2795,9 @@ class Spoom::Sorbet::Errors::Parser def open_error(error); end class << self + sig { params(output: ::String, error_url_base: ::String).returns(::Spoom::Sorbet::Errors::ParseResult) } + def parse_result(output, error_url_base: T.unsafe(nil)); end + sig { params(output: ::String, error_url_base: ::String).returns(T::Array[::Spoom::Sorbet::Errors::Error]) } def parse_string(output, error_url_base: T.unsafe(nil)); end end diff --git a/test/spoom/cli/srb/bump_test.rb b/test/spoom/cli/srb/bump_test.rb index 5035bb56..1e5ee044 100644 --- a/test/spoom/cli/srb/bump_test.rb +++ b/test/spoom/cli/srb/bump_test.rb @@ -533,6 +533,59 @@ def foo assert_equal("false", @project.read_file_strictness("file1.rb")) end + def test_bump_displays_sorbet_warnings + @project.write!("mock_sorbet", <<~RB) + #!/usr/bin/env ruby + $stderr.puts "Option `--enable-experimental-rbs-signatures` has been combined into another option." + $stderr.puts "file.rb:1: Method `foo` does not exist on `T.class_of()` https://srb.help/7003" + exit(100) + RB + @project.exec("chmod +x mock_sorbet") + @project.write!("file.rb", <<~RB) + # typed: false + foo + RB + + result = @project.spoom("srb bump --no-color --sorbet #{@project.absolute_path}/mock_sorbet") + assert_equal(<<~ERR, result.err) + Option `--enable-experimental-rbs-signatures` has been combined into another option. + ERR + assert_equal(<<~OUT, result.out) + Checking files... + + No files to bump from `false` to `true` + OUT + assert(result.status) + assert_equal("false", @project.read_file_strictness("file.rb")) + end + + def test_bump_displays_sorbet_warnings_without_errors + @project.write!("mock_sorbet", <<~RB) + #!/usr/bin/env ruby + $stderr.puts "Option `--enable-experimental-rbs-signatures` has been combined into another option." + $stderr.puts "No errors! Great job." + exit(0) + RB + @project.exec("chmod +x mock_sorbet") + @project.write!("file.rb", <<~RB) + # typed: false + class Foo; end + RB + + result = @project.spoom("srb bump --no-color --sorbet #{@project.absolute_path}/mock_sorbet") + assert_equal(<<~ERR, result.err) + Option `--enable-experimental-rbs-signatures` has been combined into another option. + ERR + assert_equal(<<~OUT, result.out) + Checking files... + + Bumped `1` file from `false` to `true`: + + file.rb + OUT + refute(result.status) + assert_equal("true", @project.read_file_strictness("file.rb")) + end + def test_bump_with_sorbet_segfault # Create a fake Sorbet that will segfault @project.write!("mock_sorbet", <<~RB) diff --git a/test/spoom/cli/srb/tc_test.rb b/test/spoom/cli/srb/tc_test.rb index 6f0d1d9a..b46e6abd 100644 --- a/test/spoom/cli/srb/tc_test.rb +++ b/test/spoom/cli/srb/tc_test.rb @@ -461,6 +461,24 @@ def test_display_sorbet_error refute(result.status) end + def test_display_sorbet_warnings_with_formatted_errors + @project.write!("mock_sorbet", <<~RB) + #!/usr/bin/env ruby + $stderr.puts "Option `--enable-experimental-rbs-signatures` has been combined into another option." + $stderr.puts "foo.rb:1: Method `foo` does not exist on `T.class_of()` https://srb.help/7003" + exit(100) + RB + @project.exec("chmod +x mock_sorbet") + + result = @project.spoom("srb tc --no-color --sorbet #{@project.absolute_path}/mock_sorbet") + assert_equal(<<~MSG, result.err) + Option `--enable-experimental-rbs-signatures` has been combined into another option. + 7003 - foo.rb:1: Method `foo` does not exist on `T.class_of()` + Errors: 1 + MSG + refute(result.status) + end + def test_deprecated_command_spoom_tc @project.remove!("errors") result = @project.spoom("tc --no-color") diff --git a/test/spoom/sorbet/errors_test.rb b/test/spoom/sorbet/errors_test.rb index 80d840ba..3f926fa6 100644 --- a/test/spoom/sorbet/errors_test.rb +++ b/test/spoom/sorbet/errors_test.rb @@ -214,6 +214,33 @@ def test_parses_errors_with_debug_string assert_equal([[], []], errors.map(&:files_from_error_sections).map(&:to_a)) end + def test_parse_result_preserves_warnings + result = Spoom::Sorbet::Errors::Parser.parse_result(<<~ERR) + Option `--enable-experimental-rbs-signatures` has been combined into another option. + Please add `--parser=prism` to your Sorbet config. + + a.rb:80: unexpected token "end" https://srb.help/2001 + 80 |end + ^^^ + Errors: 1 + ERR + + assert_equal( + [ + "Option `--enable-experimental-rbs-signatures` has been combined into another option.\n", + "Please add `--parser=prism` to your Sorbet config.\n", + ], + result.warnings, + ) + assert_equal(1, result.errors.size) + + error = T.must(result.errors.first) + assert_equal("a.rb", error.file) + assert_equal(80, error.line) + assert_equal("unexpected token \"end\"", error.message) + assert_equal(2001, error.code) + end + def test_parses_errors_with_multiple_blank_lines errors = Spoom::Sorbet::Errors::Parser.parse_string(<<~ERR) lib/a.rb:54: Method `foo` does not exist on `String` https://srb.help/7003