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

Handle the Ruby root in source locations listener #1065

Merged
merged 2 commits into from
Jul 15, 2022

Conversation

vinistock
Copy link
Member

Motivation

When methods for gem RBIs come from Ruby itself, we are currently adding the full location. This is not ideal, since each user could have Ruby installed in a different folder, which may also include platform specific labels.

We have to sanitize the RUBY_ROOT from the locations printed in order to properly add it in the Ruby LSP and provide navigation to Ruby's original source.

Additionally, we were also adding source locations for native extension files (like .so and .bundle). I don't think there's value in adding those, since it is not possible to navigate to them.

Implementation

To address point 1, I have added a new sub that changes the Ruby root for the literal RUBY_ROOT string, which we can easily replace in the Ruby LSP for the real ENV["RUBY_ROOT"] to point to the right URI.

To address point 2, I added an early return in case the symbol was defined in a file that isn't a Ruby file.

Tests

Added a test include Mutex_m to show the Ruby root behaviour.

@vinistock vinistock self-assigned this Jul 15, 2022
@vinistock vinistock requested a review from a team as a code owner July 15, 2022 14:18
@vinistock vinistock force-pushed the vs/handle_ruby_root_in_source_locations branch from 9079209 to a3d6814 Compare July 15, 2022 14:28
end

# Strip out the RUBY_ROOT prefix, which is different for each user
path = path.sub(%r{^.*(?=/lib/ruby/[0-9\.]+)}, "RUBY_ROOT")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to use something like ENV['GEM_HOME'] or ENV['GEM_PATH'] to strip out the Ruby root? I would feel more confident if we could identify the Ruby root, rather than assuming it's whatever comes before /lib/ruby/<version number>. (Nice regex though!)

Copy link
Member Author

@vinistock vinistock Jul 15, 2022

Choose a reason for hiding this comment

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

I thought about using RUBY_ROOT, but I don't know if we can assume that every user will have a version manager that sets these environment variables. If someone is using the system Ruby, those aren't set.

But the Ruby path will always have lib/ruby/<version> as far as I know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah bummer. Okay this makes sense to me then.

Copy link
Member

Choose a reason for hiding this comment

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

I think Gem.paths can be used here? It should come with any Ruby version Tapioca supports (at least 2.6 already has it).

I tried it with the system Ruby on my machine:

$ ruby -v
ruby 2.6.8p205 (2021-07-07 revision 67951) [universal.arm64e-darwin21]
$ irb
irb(main):001:0> Gem
=> Gem
irb(main):002:0> Gem.path
=> ["/Users/hung-wulo/.gem/ruby/2.6.0", "/Library/Ruby/Gems/2.6.0", "/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/gems/2.6.0"]
irb(main):003:0> caller
=>
["/Library/Ruby/Gems/2.6.0/gems/irb-1.4.1/lib/irb/workspace.rb:119:in `eval'",
 "/Library/Ruby/Gems/2.6.0/gems/irb-1.4.1/lib/irb/workspace.rb:119:in `evaluate'",

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd need to go through the array returned to find which one is the Ruby path and not the gems path and then use it for the replacement. What's the advantage over the regex?

Copy link
Member

Choose a reason for hiding this comment

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

I think what we are looking for is RbConfig::CONFIG["rubylibdir"]. We can use it on both sides of this equation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! That's exactly what we're looking for.

@vinistock vinistock force-pushed the vs/handle_ruby_root_in_source_locations branch from a3d6814 to da918af Compare July 15, 2022 15:03
@vinistock vinistock merged commit 6c77e88 into main Jul 15, 2022
@vinistock vinistock deleted the vs/handle_ruby_root_in_source_locations branch July 15, 2022 20:15
@shopify-shipit shopify-shipit bot temporarily deployed to production July 19, 2022 17:59 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to 0-9-stable August 22, 2022 21:36 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants