Skip to content

Commit

Permalink
Use indexed paths to provide jump to require definition (Shopify#975)
Browse files Browse the repository at this point in the history
Now that we have all require paths indexed, we do not need to search the
$LOAD_PATH to find the location of a require. We can just access the
index and jump directly
  • Loading branch information
vinistock authored Sep 7, 2023
1 parent 4888caa commit 71b31db
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 35 deletions.
6 changes: 3 additions & 3 deletions lib/ruby_indexer/lib/ruby_indexer/index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def initialize
@files_to_entries = T.let({}, T::Hash[String, T::Array[Entry]])

# Holds all require paths for every indexed item so that we can provide autocomplete for requires
@require_paths_tree = T.let(PrefixTree[String].new, PrefixTree[String])
@require_paths_tree = T.let(PrefixTree[IndexablePath].new, PrefixTree[IndexablePath])
end

sig { params(indexable: IndexablePath).void }
Expand Down Expand Up @@ -73,7 +73,7 @@ def [](fully_qualified_name)
@entries[fully_qualified_name.delete_prefix("::")]
end

sig { params(query: String).returns(T::Array[String]) }
sig { params(query: String).returns(T::Array[IndexablePath]) }
def search_require_paths(query)
@require_paths_tree.search(query)
end
Expand Down Expand Up @@ -147,7 +147,7 @@ def index_single(indexable_path, source = nil)
visitor.run

require_path = indexable_path.require_path
@require_paths_tree.insert(require_path, require_path) if require_path
@require_paths_tree.insert(require_path, indexable_path) if require_path
rescue Errno::EISDIR
# If `path` is a directory, just ignore it and continue indexing
end
Expand Down
2 changes: 1 addition & 1 deletion lib/ruby_indexer/test/index_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class Foo
end
RUBY

assert_equal(["path/foo", "path/other_foo"], @index.search_require_paths("path"))
assert_equal(["path/foo", "path/other_foo"], @index.search_require_paths("path").map(&:require_path))
end

def test_searching_for_entries_based_on_prefix
Expand Down
39 changes: 14 additions & 25 deletions lib/ruby_lsp/requests/definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,15 @@ def on_command(node)
string = argument.parts.first
return unless string.is_a?(SyntaxTree::TStringContent)

required_file = "#{string.value}.rb"

case message
when "require"
candidate = find_file_in_load_path(required_file)
entry = @index.search_require_paths(string.value).find do |indexable_path|
indexable_path.require_path == string.value
end

if entry
candidate = entry.full_path

if candidate
@response = Interface::Location.new(
uri: URI::Generic.from_path(path: candidate).to_s,
range: Interface::Range.new(
Expand All @@ -83,19 +85,18 @@ def on_command(node)
)
end
when "require_relative"
required_file = "#{string.value}.rb"
path = @uri.to_standardized_path
current_folder = path ? Pathname.new(CGI.unescape(path)).dirname : Dir.pwd
candidate = File.expand_path(File.join(current_folder, required_file))

if candidate
@response = Interface::Location.new(
uri: URI::Generic.from_path(path: candidate).to_s,
range: Interface::Range.new(
start: Interface::Position.new(line: 0, character: 0),
end: Interface::Position.new(line: 0, character: 0),
),
)
end
@response = Interface::Location.new(
uri: URI::Generic.from_path(path: candidate).to_s,
range: Interface::Range.new(
start: Interface::Position.new(line: 0, character: 0),
end: Interface::Position.new(line: 0, character: 0),
),
)
end
end

Expand Down Expand Up @@ -133,18 +134,6 @@ def find_in_index(value)
)
end
end

sig { params(file: String).returns(T.nilable(String)) }
def find_file_in_load_path(file)
return unless file.include?("/")

$LOAD_PATH.each do |p|
found = Dir.glob("**/#{file}", base: p).first
return "#{p}/#{found}" if found
end

nil
end
end
end
end
4 changes: 2 additions & 2 deletions lib/ruby_lsp/requests/path_completion.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ def initialize(index, emitter, message_queue)

sig { params(node: SyntaxTree::TStringContent).void }
def on_tstring_content(node)
@index.search_require_paths(node.value).sort!.each do |path|
@response << build_completion(path, node)
@index.search_require_paths(node.value).map!(&:require_path).sort!.each do |path|
@response << build_completion(T.must(path), node)
end
end

Expand Down
2 changes: 1 addition & 1 deletion test/expectations/definition/requires.exp.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"result": {
"uri": "file:///ruby_lsp/utils.rb",
"uri": "file:///ruby_lsp/event_emitter.rb",
"range": {
"start": {
"line": 0,
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/requires.rb
Original file line number Diff line number Diff line change
@@ -1 +1 @@
require "ruby_lsp/utils"
require "ruby_lsp/event_emitter"
8 changes: 7 additions & 1 deletion test/integration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,13 @@ def test_definition
initialize_lsp(["definition"])
open_file_with("require 'ruby_lsp/utils'")

assert_telemetry("textDocument/didOpen")
read_response("textDocument/didOpen")

# Populate the index
send_request("initialized")
read_response("initialized")
# Read the response for the progress end notification
read_response("$/progress")

response = make_request(
"textDocument/definition",
Expand Down
42 changes: 41 additions & 1 deletion test/requests/definition_expectations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def run_expectations(source)
index = executor.instance_variable_get(:@index)
index.index_single(
RubyIndexer::IndexablePath.new(
nil,
"#{Dir.pwd}/lib",
File.expand_path(
"../../test/fixtures/class_reference_target.rb",
__dir__,
Expand All @@ -34,6 +34,15 @@ def run_expectations(source)
),
),
)
index.index_single(
RubyIndexer::IndexablePath.new(
"#{Dir.pwd}/lib",
File.expand_path(
"../../lib/ruby_lsp/event_emitter.rb",
__dir__,
),
),
)

begin
# We need to pretend that Sorbet is not a dependency or else we can't properly test
Expand Down Expand Up @@ -93,4 +102,35 @@ def test_jumping_to_default_gems
ensure
T.must(message_queue).close
end

def test_jumping_to_default_require_of_a_gem
message_queue = Thread::Queue.new

store = RubyLsp::Store.new
store.set(uri: URI("file:///folder/fake.rb"), source: <<~RUBY, version: 1)
require "bundler"
RUBY

executor = RubyLsp::Executor.new(store, message_queue)

uri = URI::Generic.from_path(path: "#{RbConfig::CONFIG["rubylibdir"]}/bundler.rb")
executor.instance_variable_get(:@index).index_single(
RubyIndexer::IndexablePath.new(RbConfig::CONFIG["rubylibdir"], T.must(uri.to_standardized_path)),
)

Dir.glob("#{RbConfig::CONFIG["rubylibdir"]}/bundler/*.rb").each do |path|
executor.instance_variable_get(:@index).index_single(
RubyIndexer::IndexablePath.new(RbConfig::CONFIG["rubylibdir"], path),
)
end

response = executor.execute({
method: "textDocument/definition",
params: { textDocument: { uri: "file:///folder/fake.rb" }, position: { character: 10, line: 0 } },
}).response

assert_equal(uri.to_s, response.attributes[:uri])
ensure
T.must(message_queue).close
end
end

0 comments on commit 71b31db

Please sign in to comment.