Rescue StandardError when processing file #343

alexevanczuk · 2023-04-18T16:05:32Z

What are you trying to accomplish?

This should help #299.

While it doesn't solve the parsing error, it should improve debug reports by printing out which files are erroring with the error message.

Right now, when there is a NodeHelpers::TypeError OR a Sorbet TypeError (the test case I added reproduces this by creating a false positive AR association), Packwerk just crashes with a messy message:

$ bin/packwerk check
Running via Spring preloader in process 8043
📦 Packwerk is inspecting 100 files
......................................................................................................................................................................................................................................................................................./Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/sorbet-runtime-0.5.10773/lib/types/_types.rb:218:in `must': Passed `nil` into T.must (TypeError)
        from /Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/bundler/gems/packwerk-816c080d296e/lib/packwerk/association_inspector.rb:67:in `association_name'
        from /Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/sorbet-runtime-0.5.10773/lib/types/private/methods/call_validation_2_7.rb:703:in `bind_call'
        from /Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/sorbet-runtime-0.5.10773/lib/types/private/methods/call_validation_2_7.rb:703:in `block in create_validator_method_medium1'
        from /Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/bundler/gems/packwerk-816c080d296e/lib/packwerk/association_inspector.rb:38:in `constant_name_from_node'
        from /Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/sorbet-runtime-0.5.10773/lib/types/private/methods/call_validation.rb:153:in `bind_call'
        from /Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/sorbet-runtime-0.5.10773/lib/types/private/methods/call_validation.rb:153:in `validate_call_skip_block_type'
        from /Users/alex.evanczuk/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/sorbet-runtime-0.5.10773/lib/types/private/methods/call_validation.

It's not clear what file is causing packwerk to break. It might be better to list out the files that have issues to exclude them, fix the issues, or better report the issue to packwerk maintainers.

What approach did you choose and why?

I decided to rescue all of StandardError, because there are a number of possibly unexpected things that can cause packwerk parsing to break. In theory we could target more specific errors, but then we wouldn't be catching all possible errors and ambiguous errors continue to be reported ambiguously.

What should reviewers focus on?

I wasn't able to reproduce the specific case in the issue – any thoughts on what is going on there?
Let me know if I should approach this a different way.

Type of Change

Bugfix
New feature
Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
It is safe to rollback this change.

gmcgibbon · 2023-04-19T21:19:59Z

test/integration/packwerk/custom_executable_integration_test.rb

+
+        assert_match(/Packwerk is inspecting 13 files/, captured_output)
+        assert_match(%r{components/timeline/app/models/bad_file.rb}, captured_output)
+        assert_match(/Passed `nil` into T.must/, captured_output)


Should we be making this code more fault tolerant? If this stack trace is within Packwerk, I imagine the end user should not be concerned with this and it is actually a bug.

My main goal here is solving for unknowns by allowing packwerk to have more helpful debugging output. Perhaps instead this should say something to the effect of: "packwerk encountered an internal error. Please file an issue and include this error message and stacktrace."

What do you think?

Made this update

rafaelfranca · 2023-04-20T01:38:19Z

lib/packwerk/file_processor.rb

@@ -53,6 +53,9 @@ def call(relative_file)
      ProcessedFile.new(unresolved_references: unresolved_references)
    rescue Parsers::ParseError => e
      ProcessedFile.new(offenses: [e.result])
+    rescue StandardError => e


What exactly is raising the error? The way this code is, if I write bad code in packwerk it is our users that will see the error, not us.

I've updated the message to make the intent clearer – that this is an internal error.

Now, instead of packwerk just blowing up with no information about what file is causing issues:

It will list the file that is causing issues

It will make it clear it's an internal issue

It gives the user information about how to unblock

It gives information helpful for debugging and issue reporting

rafaelfranca · 2023-04-20T01:38:52Z

test/integration/packwerk/custom_executable_integration_test.rb

+
+          RSpec.describe ActiveAdmin::Resource::BelongsTo do
+            it "should have an owner" do
+              expect(belongs_to.owner).to eq post_config


I'm not sure I get this example. It is failing because belongs_to doesn't exist? Is packwerk executing the Ruby code instead of just parsing it?

No – it's only parsing it. However, when it parses it, the AssociationInspector calls association_name which calls:

association_name_node = T.must(arguments[0])

In this case, arguments[0] is nil, which throws an error.

I've simplified the code to better capture the error.

I think we should fix this parsing issue, for sure, and then perhaps replace this test with a stub to reflect other unknowns. The main thing is I want consumers of packwerk to be able to report better errors.

alexevanczuk requested a review from a team as a code owner April 18, 2023 16:05

alexevanczuk changed the title ~~Rescue TypeError when processing file~~ Rescue StandardError when processing file Apr 18, 2023

alexevanczuk force-pushed the ae-always-spit-out-error-when-processing-files branch 3 times, most recently from ee3d5fa to 9eda05a Compare April 18, 2023 16:06

This was referenced Apr 19, 2023

Rescue StandardError when processing a file #348

Closed

Rescue StandardError when processing a file gradual-systems/packwerk#2

Merged

gmcgibbon reviewed Apr 19, 2023

View reviewed changes

rafaelfranca reviewed Apr 20, 2023

View reviewed changes

rafaelfranca approved these changes Apr 20, 2023

View reviewed changes

Rescue StandardError when processing a file

c3bba85

alexevanczuk force-pushed the ae-always-spit-out-error-when-processing-files branch from 2dd00f0 to c3bba85 Compare April 20, 2023 18:24

alexevanczuk merged commit 6bcf2ae into main Apr 20, 2023

alexevanczuk deleted the ae-always-spit-out-error-when-processing-files branch April 20, 2023 18:30

shopify-shipit bot temporarily deployed to rubygems August 8, 2023 20:48 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rescue StandardError when processing file #343

Rescue StandardError when processing file #343

alexevanczuk commented Apr 18, 2023

gmcgibbon Apr 19, 2023

alexevanczuk Apr 19, 2023

alexevanczuk Apr 20, 2023

rafaelfranca Apr 20, 2023

alexevanczuk Apr 20, 2023

rafaelfranca Apr 20, 2023

alexevanczuk Apr 20, 2023

Rescue StandardError when processing file #343

Rescue StandardError when processing file #343

Conversation

alexevanczuk commented Apr 18, 2023

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Checklist

gmcgibbon Apr 19, 2023

Choose a reason for hiding this comment

alexevanczuk Apr 19, 2023

Choose a reason for hiding this comment

alexevanczuk Apr 20, 2023

Choose a reason for hiding this comment

rafaelfranca Apr 20, 2023

Choose a reason for hiding this comment

alexevanczuk Apr 20, 2023

Choose a reason for hiding this comment

rafaelfranca Apr 20, 2023

Choose a reason for hiding this comment

alexevanczuk Apr 20, 2023

Choose a reason for hiding this comment