From 88e872e579d7f7e6ff842699bf16ac6ee0159259 Mon Sep 17 00:00:00 2001 From: Phil Monroe Date: Tue, 6 Aug 2024 12:45:15 -0400 Subject: [PATCH] Log an error message if a Sorbet signature raises an exception --- lib/tapioca/commands/abstract_gem.rb | 9 +++- lib/tapioca/gem/listeners/methods.rb | 14 +++++- lib/tapioca/gem/pipeline.rb | 20 ++++++++- lib/tapioca/loaders/loader.rb | 2 +- lib/tapioca/runtime/reflection.rb | 7 ++- lib/tapioca/static/symbol_loader.rb | 2 +- spec/tapioca/gem/pipeline_spec.rb | 52 +++++++++++++++++++--- spec/tapioca/runtime/reflection_spec.rb | 57 +++++++++++++++++++++++++ 8 files changed, 151 insertions(+), 12 deletions(-) diff --git a/lib/tapioca/commands/abstract_gem.rb b/lib/tapioca/commands/abstract_gem.rb index c4b66916b..c4752730d 100644 --- a/lib/tapioca/commands/abstract_gem.rb +++ b/lib/tapioca/commands/abstract_gem.rb @@ -119,7 +119,14 @@ def compile_gem_rbi(gem) ) if @file_header rbi.root = Runtime::Trackers::Autoload.with_disabled_exits do - Tapioca::Gem::Pipeline.new(gem, include_doc: @include_doc, include_loc: @include_loc).compile + Tapioca::Gem::Pipeline.new( + gem, + include_doc: @include_doc, + include_loc: @include_loc, + error_handler: ->(error) { + say_error(error, :bold, :red) + }, + ).compile end merge_with_exported_rbi(gem, rbi) if @include_exported_rbis diff --git a/lib/tapioca/gem/listeners/methods.rb b/lib/tapioca/gem/listeners/methods.rb index e30dac189..1b3714d6f 100644 --- a/lib/tapioca/gem/listeners/methods.rb +++ b/lib/tapioca/gem/listeners/methods.rb @@ -73,7 +73,7 @@ def compile_method(tree, symbol_name, constant, method, visibility = RBI::Public return unless method_owned_by_constant?(method, constant) return if @pipeline.symbol_in_payload?(symbol_name) && !@pipeline.method_in_gem?(method) - signature = signature_of(method) + signature = lookup_signature_of(method) method = T.let(signature.method, UnboundMethod) if signature method_name = method.name.to_s @@ -211,6 +211,18 @@ def initialize_method_for(constant) def ignore?(event) event.is_a?(Tapioca::Gem::ForeignScopeNodeAdded) end + + sig { params(method: UnboundMethod).returns(T.untyped) } + def lookup_signature_of(method) + signature_of!(method) + rescue LoadError, StandardError => error + @pipeline.error_handler.call(<<~MSG) + Unable to compile signature for method: #{method.owner}##{method.name} + Exception raised when loading signature: #{error.inspect} + MSG + + nil + end end end end diff --git a/lib/tapioca/gem/pipeline.rb b/lib/tapioca/gem/pipeline.rb index 61013e533..2b1b655d3 100644 --- a/lib/tapioca/gem/pipeline.rb +++ b/lib/tapioca/gem/pipeline.rb @@ -13,12 +13,28 @@ class Pipeline sig { returns(Gemfile::GemSpec) } attr_reader :gem - sig { params(gem: Gemfile::GemSpec, include_doc: T::Boolean, include_loc: T::Boolean).void } - def initialize(gem, include_doc: false, include_loc: false) + sig { returns(T.proc.params(error: String).void) } + attr_reader :error_handler + + sig do + params( + gem: Gemfile::GemSpec, + error_handler: T.proc.params(error: String).void, + include_doc: T::Boolean, + include_loc: T::Boolean, + ).void + end + def initialize( + gem, + error_handler:, + include_doc: false, + include_loc: false + ) @root = T.let(RBI::Tree.new, RBI::Tree) @gem = gem @seen = T.let(Set.new, T::Set[String]) @alias_namespace = T.let(Set.new, T::Set[String]) + @error_handler = error_handler @events = T.let([], T::Array[Gem::Event]) diff --git a/lib/tapioca/loaders/loader.rb b/lib/tapioca/loaders/loader.rb index 0e1aeff4d..a1242aefb 100644 --- a/lib/tapioca/loaders/loader.rb +++ b/lib/tapioca/loaders/loader.rb @@ -230,7 +230,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 - sig { params(engine: T.class_of(Rails::Engine)).returns(T::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 diff --git a/lib/tapioca/runtime/reflection.rb b/lib/tapioca/runtime/reflection.rb index 370925b70..b21032222 100644 --- a/lib/tapioca/runtime/reflection.rb +++ b/lib/tapioca/runtime/reflection.rb @@ -130,8 +130,13 @@ def qualified_name_of(constant) end sig { params(method: T.any(UnboundMethod, Method)).returns(T.untyped) } - def signature_of(method) + def signature_of!(method) T::Utils.signature_for_method(method) + end + + sig { params(method: T.any(UnboundMethod, Method)).returns(T.untyped) } + def signature_of(method) + signature_of!(method) rescue LoadError, StandardError nil end diff --git a/lib/tapioca/static/symbol_loader.rb b/lib/tapioca/static/symbol_loader.rb index f8599b2b1..86fe326e2 100644 --- a/lib/tapioca/static/symbol_loader.rb +++ b/lib/tapioca/static/symbol_loader.rb @@ -65,7 +65,7 @@ def symbols_from_paths(paths) private - sig { returns(T::Array[T.class_of(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") diff --git a/spec/tapioca/gem/pipeline_spec.rb b/spec/tapioca/gem/pipeline_spec.rb index 601b04236..9e253fb6e 100644 --- a/spec/tapioca/gem/pipeline_spec.rb +++ b/spec/tapioca/gem/pipeline_spec.rb @@ -13,11 +13,24 @@ class Tapioca::Gem::PipelineSpec < Minitest::HooksSpec include Tapioca::SorbetHelper DEFAULT_GEM_NAME = T.let("the-default-gem", String) + NO_UNHANDLED_PIPELINE_COMPILATION_ERRORS_MESSAGE = <<~MSG + Tapioca::Gem::Pipeline#compile should not have any unhandled errors reported. + Please either fix your changes to ensure no errors are reported, or explicity pass + in a custom `error_handler: ->(err) { ... }` to the `compile()` method to handle + expected errors. + MSG private - sig { params(gem_name: String, include_doc: T::Boolean, include_loc: T::Boolean).returns(String) } - def compile(gem_name = DEFAULT_GEM_NAME, include_doc: false, include_loc: false) + sig do + params( + gem_name: String, + include_doc: T::Boolean, + include_loc: T::Boolean, + reported_errors_expected: T::Boolean, + ).returns(String) + end + def compile(gem_name = DEFAULT_GEM_NAME, include_doc: false, include_loc: false, reported_errors_expected: false) mock_gem_path = mock_gems[gem_name] # If we are compiling for a mock gem, we need to create a fake gemspec @@ -35,8 +48,16 @@ def compile(gem_name = DEFAULT_GEM_NAME, include_doc: false, include_loc: false) # wrap it in our gemspec wrapper gem = Tapioca::Gemfile::GemSpec.new(Gem::Specification.stubs[gem_name].first) + # clear out previously reported errors + reported_errors.clear + # push it through the pipeline - tree = Tapioca::Gem::Pipeline.new(gem, include_doc: include_doc, include_loc: include_loc).compile + tree = Tapioca::Gem::Pipeline.new( + gem, + include_doc: include_doc, + include_loc: include_loc, + error_handler: ->(error) { reported_errors << error }, + ).compile # NOTE: This is not returning a `RBI::File`. # The following test suite is based on the string output of the `RBI::Tree` rather @@ -44,6 +65,7 @@ def compile(gem_name = DEFAULT_GEM_NAME, include_doc: false, include_loc: false) # We should eventually update these tests to be based on the `RBI::File`. Tapioca::DEFAULT_RBI_FORMATTER.format_tree(tree) + assert_empty(reported_errors, NO_UNHANDLED_PIPELINE_COMPILATION_ERRORS_MESSAGE) unless reported_errors_expected tree.string end @@ -65,6 +87,11 @@ def mock_gems @gems end + sig { returns(T::Array[String]) } + def reported_errors + @reported_errors ||= T.let([], T.nilable(T::Array[String])) + end + describe Tapioca::Gem::Pipeline do before do # We need to undefine and unload `ActiveSupport` so that the test object @@ -79,6 +106,11 @@ def mock_gems end end + after do + # clean up any lingering reported errors + reported_errors.clear + end + it "compiles DelegateClass" do add_ruby_file("bar.rb", <<~RUBY) class Bar @@ -3861,7 +3893,7 @@ module ActiveSupport::Rescuable::ClassMethods; end assert_equal(output, compile) end - it "skips signatures if they raise" do + it "skips signatures and reports the errors if the sig block raises" do add_ruby_file("foo.rb", <<~RUBY) class Foo extend(T::Sig) @@ -3900,7 +3932,17 @@ def foo(*args, &blk); end end RBI - assert_equal(output, compile) + assert_equal(output, compile(reported_errors_expected: true)) + assert_equal(reported_errors, [ + <<~ERROR, + Unable to compile signature for method: Foo#bar + Exception raised when loading signature: # + ERROR + <<~ERROR, + Unable to compile signature for method: Foo#foo + Exception raised when loading signature: # + ERROR + ]) end it "handles signatures with attached classes" do diff --git a/spec/tapioca/runtime/reflection_spec.rb b/spec/tapioca/runtime/reflection_spec.rb index 26ad5fe11..8643fd533 100644 --- a/spec/tapioca/runtime/reflection_spec.rb +++ b/spec/tapioca/runtime/reflection_spec.rb @@ -59,6 +59,29 @@ def equal?(other) end end + class SignatureFoo + extend T::Sig + + sig { returns(String) } + def good_method + "Thank you." + end + + # NOTE: leveraging eval to avoid actual sorbet typechecking + eval <<~RUBY + sig do + raise ArgumentError + end + def bad_method + "oh no..." + end + RUBY + + def unknown_method + ' ¯\_(ツ)_/¯ ' + end + end + class ReflectionSpec < Minitest::Spec describe Tapioca::Runtime::Reflection do it "might return the wrong results without Reflection helpers" do @@ -111,6 +134,40 @@ class ReflectionSpec < Minitest::Spec it "returns top level anchored name for named class" do assert_equal("::Tapioca::Runtime::LyingFoo", Runtime::Reflection.qualified_name_of(LyingFoo)) end + + describe "signature_for" do + it "returns a valid signature" do + method = SignatureFoo.instance_method(:good_method) + refute_nil(Runtime::Reflection.signature_of(method)) + end + + it "returns nil when a signature is not defined" do + method = SignatureFoo.instance_method(:unknown_method) + assert_nil(Runtime::Reflection.signature_of(method)) + end + + it "returns nil when a signature block raises an exception" do + method = SignatureFoo.instance_method(:bad_method) + assert_nil(Runtime::Reflection.signature_of(method)) + end + end + + describe "signature_for!" do + it "returns a valid signature" do + method = SignatureFoo.instance_method(:good_method) + refute_nil(Runtime::Reflection.signature_of!(method)) + end + + it "returns nil when a signature is not defined" do + method = SignatureFoo.instance_method(:unknown_method) + assert_nil(Runtime::Reflection.signature_of!(method)) + end + + it "returns nil when a signature block raises an exception" do + method = SignatureFoo.instance_method(:bad_method) + assert_raises(ArgumentError) { Runtime::Reflection.signature_of!(method) } + end + end end end end