Log Sorbet signature block errors that lead to misleading RBI signatures generated

[phil-monroe]

Motivation

When a sorbet signature block ends up throwing an exception, it gets swallowed up by the signature_for(method) in the Tapioca::Runtime::Reflection utilities, and an incorrect signature is generated. This has created some confusion for my team when the sorbet type checking failed, even though it appeared as if the signature was implemented correctly.

Example
Sorbet Interface in upstream gem
NOTE: the dependency/require statements on the ExampleContracts::WidgetDto had a simple bug that led to it being defined in most scenarios, but was not happing in the tapioca run downstream from these packages.

module Example::WidgetAdapter
  extend T::Sig
  extend T::Helpers
  interface!

  sig do 
    abstract
      .params(widget: ExampleContracts::WidgetDto)
      .returns(T::Boolean)
  end
  def create(widget); end
end

Tapioca generated RBI for interface defined upstream
NOTE: Tapioca does not log any errors or warnings when generating this RBI.

module Example::WidgetAdapter
  interface!

  # source://sorbet-runtime/0.5.11481/lib/types/private/methods/_methods.rb#257
  def create(*args, **_arg1, &blk); end
end

Example sorbet error

sorbet/rbi/gems/example@0.1.0.rbi:221: All methods in an interface must be declared abstract https://srb.help/5022
     221 |  def create(*args, **_arg1, &blk); end
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

sorbet/rbi/gems/example@0.1.0.rbi:265: All methods in an interface must be declared abstract https://srb.help/5022
     265 |  def call(*args, **_arg1, &blk); end
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Errors: 2

As you can see, the original implementation of the Example::WidgetAdapter indeed seemingly does have a signature defined, but the swallowed exception in Tapioca leads to a misleading RBI generated and a sorbet type-checking error.

Implementation

Log any errors raised to $stderr when reflecting on method signatures to increase developer visibility + understanding.

With this change in place, you will now see a logged error message like the following:

Unable to compile signature for method: Example::WidgetsAdapter#create
  Exception raised when loading signature: #<NameError: uninitialized constant ExampleContracts::WidgetDto>

Tests

Tests have been added and/or modified to check for the reported errors.

[phil-monroe]

@paracycle - I've updated the approach to follow the error_handler: ->(err) { } argument pattern, which indeed allows for more contextual error messaging. If this generally looks good to you, I'll add additional tests throughout to verify my changes.

Thanks for your help!

[paracycle]

I think I'd prefer this to be a required keyword argument. Coincidentally, I think that will force you to add support for an error handler for the gem/pipeline_spec tests, which in turn should allow you to properly test this case as a unit test inside that file.

[phil-monroe]

Sounds good. As long as the general design is looking ok, I'll do another pass to make sure that everything is fully covered with tests.

[phil-monroe]

@paracycle - I've updated the approach based on your last feedback. Let me know if the general design matches your expectations and I can go through and add tests.

One thing I was considering, but do not have the time for in this PR: it might be nice for that lookup_signature_for(method) to actually return both the signature or the error raised (kind of like golang error handling), so that we could pass the signature_error to the @pipeline.push_method(...) and underlying Gem::MethodNodeAdded event. This would allow us to also create helpful hint comments in the autogenerated RBIs as well.

[phil-monroe]

NOTE: the T::Sig::WithoutRuntime is needed because tapioca itself does not directly depend on Rails, but will attempt to generate signatures for this method for projects that do not include Rails, and trigger an sig block exception to be logged.

[phil-monroe]

NOTE: the T::Sig::WithoutRuntime is needed because tapioca itself does not directly depend on Rails, but will attempt to generate signatures for this method for projects that do not include Rails, and trigger an sig block exception to be logged.

[paracycle]

Thank you for your recent changes, this is looking really good. I have a small test improvement suggestion.

[paracycle]

I would very much like it if the default behaviour of the error handler was to collect the errors, and, after the compilation is done, to assert that the list of collected errors is empty.

That would be a free negative assertion for all the tests that we don't expect to fail, and the behaviour could be easily overriden by passing an alternative error handler, like how you are doing in the added test.

[phil-monroe]

I like it and can include this change as well. Thanks!

[phil-monroe]

@paracycle - I've updated the specs to include a negative assertion for all the tests that we don't expect to fail and fixed the failing rubocop linting. Let me know if there is anything else you think this needs.

Thanks again for all of your help!

[paracycle]

Thank you for all the work on this PR. I am really happy with the result, except for a minor comment.

[phil-monroe]

@paracycle - I've just addressed the lingering test state for the reported_errors. Let me know if there is anything else left you would like done.

[phil-monroe]

Hmm... the CI failure seems unrelated to my changes. Is there any chance someone can retrigger the build, or should I attempt to force-push an update to trigger another build?

[Morriar]

I reran the failed tests, seems to be related to an infrastructure issue

[paracycle]

@phil-monroe Funny story: It turns out that about 3 years ago, I'd discovered that the exception swallowing behaviour of signature_of was problematic and changed it in a similar way, but promptly forgot about it: https://github.com/Shopify/tapioca/pull/293/files#diff-6c17c60032864d0164ef50591234ad8238992fb3dad7d6ac33158f333ea61190R959

I will rebase that PR on top of this work and ship it finally. Thanks for the reminder :)

[phil-monroe]

Ha, I totally get it. Thanks for all of your help and feel free to reach out if need any help validating additional changes in the future.

The one follow on feature request I kind of wish tapioca had related to these changes:

What if the compiled RBIs also documented the raised exception details in the generated RBIs? That way, if you miss the log messages but go to look at the RBIs while debugging, there is still a historical record that something was indeed wrong with the compilation phase.

Thoughts?

[paracycle]

What if the compiled RBIs also documented the raised exception details in the generated RBIs? That way, if you miss the log messages but go to look at the RBIs while debugging, there is still a historical record that something was indeed wrong with the compilation phase.

Thoughts?

Sure, that is also a good idea. I've been wanting Tapioca to generate more comments on generated methods, especially for DSL RBI methods, explaining why the method is being defined, for a long time. For example, def comments; end method could easily have a comment saying that is was defined because the model has a has_many :comments annotation.

It would definitely be good to note warning/error like messages there, those that aren't bad enough to stop generating RBIs but important enough to have more visibility.

I guess the main question would be how to make it explicit that the comment that is added is not part of the comment that we extract from the source method definition, but that might a good thing to discuss in detail.

Do you mind opening an issue for this?

[phil-monroe]

Yeah, I like that concept, and happy to create a dedicated issue to discuss more!

Thanks again for all of your help @paracycle !