Skip to content

Migrate parsing to YARP - #227

Merged
Morriar merged 4 commits into
mainfrom
at-yarp
Sep 7, 2023
Merged

Migrate parsing to YARP#227
Morriar merged 4 commits into
mainfrom
at-yarp

Conversation

@Morriar

@Morriar Morriar commented Sep 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

I've been experimenting with different parser backends to benchmark RBI parsing.

Here are the time required to parse all the 11801 RBI files in on big project:

Backend Parsing Time
Whitequark 288s
SyntaxTree 548s
YARP 22s

This PR migrates RBI parsing to YARP and removes the dependecy to Whitequark and unparser.

Running tapioca check-shims before (using parser gem):

Loading Sorbet payload...  Done (3.72s)
Loading shim RBIs from sorbet/rbi/shims... Done (0.97s)
Loading gem RBIs from sorbet/rbi/gems...  Done (204.15s)
Loading dsl RBIs from sorbet/rbi/dsl...  Done (69.79s)
Loading annotation RBIs from sorbet/rbi/annotations...  Done (1.04s)
Looking for duplicates...  Done (0.56s)

No duplicates found in shim RBIs

real	4m43.293s

Running tapioca check-shims after (using YARP):

Loading Sorbet payload...  Done (3.09s)
Loading shim RBIs from sorbet/rbi/shims... Done (2.28s)
Loading gem RBIs from sorbet/rbi/gems...  Done (18.49s)
Loading dsl RBIs from sorbet/rbi/dsl...  Done (21.41s)
Loading annotation RBIs from sorbet/rbi/annotations...  Done (0.51s)
Looking for duplicates...  Done (0.52s)

No duplicates found in shim RBIs

real	0m56.171s

@Morriar Morriar added enhancement New feature or request dependencies Pull requests that update a dependency file labels Sep 5, 2023
@Morriar
Morriar requested a review from a team as a code owner September 5, 2023 19:27
@Morriar Morriar self-assigned this Sep 5, 2023
andyw8
andyw8 reviewed Sep 5, 2023
View reviewed changes
Comment thread rbi.gemspec
spec.add_dependency("ast")
spec.add_dependency("parser", ">= 3.0.0")
spec.add_dependency("sorbet-runtime", ">= 0.5.9204")
spec.add_dependency("unparser", ">= 0.5.6")

@andyw8 andyw8 Sep 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since YARP releases may contain breaking changes while still under develpment, should be put an upper limit on this?

@Morriar Morriar Sep 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This may help us find and fix these bugs faster

andyw8
andyw8 reviewed Sep 5, 2023
View reviewed changes
Comment thread lib/rbi/loc.rb
begin_line: yarp_location.start_line,
end_line: yarp_location.end_line,
begin_column: yarp_location.start_column,
end_column: yarp_location.end_column + 1, # TODO: Why is this off by one?

@andyw8 andyw8 Sep 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Known issue, I'm verifying Kevin's suggestion for a fix here.

@Morriar Morriar Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great! I'm going to merge and update this once fixed 👍

@Morriar

Morriar commented Sep 6, 2023

Copy link
Copy Markdown
Contributor Author

I bumped YARP to 0.0.10 and tested the changes on core 👍

Base automatically changed from at-ruby27 to main September 6, 2023 15:56
@Morriar
Morriar requested a review from andyw8 September 7, 2023 15:09
andyw8
andyw8 approved these changes Sep 7, 2023
View reviewed changes

@andyw8 andyw8 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't read through every line of the lib/rbi/parser.rb changes, but I'm trusting the tests are sufficiently covering it.

andyw8
andyw8 reviewed Sep 7, 2023
View reviewed changes
Comment thread test/rbi/parser_test.rb Outdated
assert_equal("unexpected token $end", e.message)
assert_equal("test_parse_real_file_with_error.rbi:2:0-2:0", e.location.to_s)
assert_equal("Expected to be able to parse an expression. Expected `end` to close `class` statement.", e.message)
# assert_equal("test_parse_real_file_with_error.rbi:1:10-1:10", e.location.to_s)

@andyw8 andyw8 Sep 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be reinstated?

@Morriar Morriar Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah good idea. I commented it until we figure the weird locations returned by YARP but I can keep the wrong location in the assert instead

andyw8
andyw8 reviewed Sep 7, 2023
View reviewed changes
Comment thread sorbet/rbi/shims/gems/yarp.rbi
@Morriar
Add dependency to YARP
74899df 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Migrate parser to YARP
267349a 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Remove dependency to unparser
82ab88a 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Regenerate RBIs
2a4a820 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Morriar merged commit f5f1c43 into main Sep 7, 2023
@Morriar
Morriar deleted the at-yarp branch September 7, 2023 15:42
@shopify-shipit
shopify-shipit Bot temporarily deployed to production September 7, 2023 15:47 Inactive
@Morriar Morriar added the breaking-change Change breaking retro-compatibility label Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bitwise-aiden bitwise-aiden Awaiting requested review from bitwise-aiden bitwise-aiden is a code owner automatically assigned from Shopify/sorbet
1 more reviewer
@andyw8 andyw8 andyw8 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@Morriar Morriar

Labels

breaking-change Change breaking retro-compatibility dependencies Pull requests that update a dependency file enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Morriar @andyw8