Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a custom URI class to handle source location comments #1094

Merged
merged 7 commits into from Aug 16, 2022

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Aug 2, 2022

Motivation

We are currently relying on regexes and string substitutions to find the right paths for gems. This is not reliable and has proven to not find the right paths.

Implementation

This PR introduces a custom URI object to handle the source:// scheme. This makes it easier to validate the consistency of the URI and we can use a similar object on the Ruby LSP to do the inverse operation of parsing it.

It also improves the detection of default gem paths, by using Gemfile::GemSpec, which already has the mechanisms to figure out these paths for us.

The format of source URIs is now:

source://tapioca/1.2.3/lib/foo.rb#13
^ scheme
         ^ gem name
                 ^ gem version (optional)
                       ^ path
                                  ^ line number

Tests

Updated existing tests.

@vinistock vinistock self-assigned this Aug 2, 2022
@vinistock vinistock requested a review from a team as a code owner August 2, 2022 15:05
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I think it may be too much effort but are we able to have a test for the BUNDLER_HOME scenario?

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

I am not sure if I understand why we are doing more breakdowns here. Isn't it the case that if I have foo-1.2.3 as a dependency, that it should be referenced in the RBI file as foo-1.2.3? Do we care if I suddenly change my Bundler setup so that it installs foo to the bundle path? If so, why do we care?

@vinistock
Copy link
Member Author

@paracycle while testing this on Core, I realized that we have a mix of both. Some gems are installed under Bundler.home and others are installed under Bundle.bundle_path.

If this information is not present somehow in the source location comments, the Ruby LSP has no way of figuring out where the appropriate path is.

I have no clue why we have gems on both locations though. If you have more context on why this could be, let me know.

@paracycle
Copy link
Member

If this information is not present somehow in the source location comments, the Ruby LSP has no way of figuring out where the appropriate path is.

LSP can search in both locations?

@vinistock
Copy link
Member Author

Is that better though? That means that instead of processing that ahead of time in Tapioca, we'll need to search on both when serving the request.

@paracycle
Copy link
Member

Is that better though? That means that instead of processing that ahead of time in Tapioca, we'll need to search on both when serving the request.

Yes, I think it is better. IMO, this mechanism should be open to the gem moving from one valid place to another, and as long as the gem is the same version, we should still be able to locate it without an error.

So, encoding a lot of information on the generation side is not the best way to go, the client should be able to look things up on consumption side. After all, that's exactly what happens with Ruby tools, they all resolve a gem reference by looking it up in various place the gem could be.

@vinistock vinistock force-pushed the vs/fix_source_location_for_gems_installed_from_source branch from 798da50 to 363b9fc Compare August 2, 2022 21:10
@vinistock vinistock changed the title Insert BUNDLER_HOME and BUNDLER_BUNDLE_PATH in source location comments Sanitize Bundler home from source location comments Aug 2, 2022
@vinistock vinistock force-pushed the vs/fix_source_location_for_gems_installed_from_source branch 3 times, most recently from f8272bd to 0da60d8 Compare August 11, 2022 17:24
@vinistock vinistock changed the title Sanitize Bundler home from source location comments Use a custom URI class to handle source location comments Aug 11, 2022
@vinistock vinistock force-pushed the vs/fix_source_location_for_gems_installed_from_source branch 3 times, most recently from 5fcf582 to 183baf6 Compare August 12, 2022 15:57
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

This is looking really good but I still have a few comments.

lib/tapioca/helpers/source_uri.rb Outdated Show resolved Hide resolved
lib/tapioca/gem/listeners/source_location.rb Outdated Show resolved Hide resolved
lib/tapioca/gem/listeners/source_location.rb Outdated Show resolved Hide resolved
lib/tapioca/gem/listeners/source_location.rb Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs/fix_source_location_for_gems_installed_from_source branch 3 times, most recently from d3d045c to a689279 Compare August 15, 2022 20:58
@dirceu
Copy link
Contributor

dirceu commented Aug 16, 2022

Code looks good to me. Questions for my own understanding:

  1. Is ruby-lsp the only client that currently consumes those source location comments?
  2. Given that Tapioca usually doesn't regenerate RBIs unless you change the version of a gem, does that mean that ruby-lsp will have to support multiple formats of source location comments?

@vinistock
Copy link
Member Author

@dirceu Yeah, for now the Ruby LSP is the only consumer of the source location comments. However, Sorbet could also consume them to include them as a location on go to definition.

What do you mean by multiple formats?

@dirceu
Copy link
Contributor

dirceu commented Aug 16, 2022

@vinistock I meant source://tapioca/1.2.3/lib/foo.rb#13 vs source://tapioca-1.2.3/lib/foo.rb#13 for example (which is I believe the current format in main). The format did change slightly right?

@vinistock
Copy link
Member Author

@dirceu yes, we changed the format to source://gem_name/version/path/file.rb#line_number. This allows us to use the custom URI class to properly parse the components. With the dashes, it's hard to tell if the dash is a part of the gem name or just the dash separating the version number.

@vinistock vinistock force-pushed the vs/fix_source_location_for_gems_installed_from_source branch from b57b4f3 to eca75c5 Compare August 16, 2022 14:46
@vinistock vinistock force-pushed the vs/fix_source_location_for_gems_installed_from_source branch from eca75c5 to 539c687 Compare August 16, 2022 14:47
lib/tapioca/gemfile.rb Outdated Show resolved Hide resolved
@vinistock vinistock merged commit 5765840 into main Aug 16, 2022
@vinistock vinistock deleted the vs/fix_source_location_for_gems_installed_from_source branch August 16, 2022 17:56
@paracycle paracycle added the enhancement New feature or request label Aug 22, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production August 31, 2022 14:40 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to 0-10-stable September 14, 2022 02:35 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants