Skip to content

Commit

Permalink
Use location link for namespace and method definition
Browse files Browse the repository at this point in the history
  • Loading branch information
vinistock committed Jun 18, 2024
1 parent 04e47b3 commit 58c694f
Show file tree
Hide file tree
Showing 8 changed files with 75 additions and 46 deletions.
2 changes: 2 additions & 0 deletions lib/ruby_indexer/lib/ruby_indexer/entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ class Visibility < T::Enum
sig { returns(RubyIndexer::Location) }
attr_reader :location

alias_method :name_location, :location

sig { returns(T::Array[String]) }
attr_reader :comments

Expand Down
5 changes: 4 additions & 1 deletion lib/ruby_lsp/addon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,10 @@ def create_semantic_highlighting_listener(response_builder, dispatcher); end
# Creates a new Definition listener. This method is invoked on every Definition request
sig do
overridable.params(
response_builder: ResponseBuilders::CollectionResponseBuilder[Interface::Location],
response_builder: ResponseBuilders::CollectionResponseBuilder[T.any(
Interface::Location,
Interface::LocationLink,
)],
uri: URI::Generic,
node_context: NodeContext,
dispatcher: Prism::Dispatcher,
Expand Down
27 changes: 12 additions & 15 deletions lib/ruby_lsp/listeners/definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ class Definition

sig do
params(
response_builder: ResponseBuilders::CollectionResponseBuilder[Interface::Location],
response_builder: ResponseBuilders::CollectionResponseBuilder[T.any(
Interface::Location,
Interface::LocationLink,
)],
global_state: GlobalState,
uri: URI::Generic,
node_context: NodeContext,
Expand Down Expand Up @@ -158,16 +161,13 @@ def handle_method_definition(message, receiver_type)
return unless methods

methods.each do |target_method|
location = target_method.location
file_path = target_method.file_path
next if @typechecker_enabled && not_in_dependencies?(file_path)

@response_builder << Interface::Location.new(
uri: URI::Generic.from_path(path: file_path).to_s,
range: Interface::Range.new(
start: Interface::Position.new(line: location.start_line - 1, character: location.start_column),
end: Interface::Position.new(line: location.end_line - 1, character: location.end_column),
),
@response_builder << Interface::LocationLink.new(
target_uri: URI::Generic.from_path(path: file_path).to_s,
target_range: range_from_location(target_method.location),
target_selection_range: range_from_location(target_method.name_location),
)
end
end
Expand Down Expand Up @@ -218,19 +218,16 @@ def find_in_index(value)
return if first_entry.private? && first_entry.name != "#{@node_context.fully_qualified_name}::#{value}"

entries.each do |entry|
location = entry.location
# If the project has Sorbet, then we only want to handle go to definition for constants defined in gems, as an
# additional behavior on top of jumping to RBIs. Sorbet can already handle go to definition for all constants
# in the project, even if the files are typed false
file_path = entry.file_path
next if @typechecker_enabled && not_in_dependencies?(file_path)

@response_builder << Interface::Location.new(
uri: URI::Generic.from_path(path: file_path).to_s,
range: Interface::Range.new(
start: Interface::Position.new(line: location.start_line - 1, character: location.start_column),
end: Interface::Position.new(line: location.end_line - 1, character: location.end_column),
),
@response_builder << Interface::LocationLink.new(
target_uri: URI::Generic.from_path(path: file_path).to_s,
target_range: range_from_location(entry.location),
target_selection_range: range_from_location(entry.name_location),
)
end
end
Expand Down
6 changes: 3 additions & 3 deletions lib/ruby_lsp/requests/definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class Definition < Request
def initialize(document, global_state, position, dispatcher, typechecker_enabled)
super()
@response_builder = T.let(
ResponseBuilders::CollectionResponseBuilder[Interface::Location].new,
ResponseBuilders::CollectionResponseBuilder[Interface::Location],
ResponseBuilders::CollectionResponseBuilder[T.any(Interface::Location, Interface::LocationLink)].new,
ResponseBuilders::CollectionResponseBuilder[T.any(Interface::Location, Interface::LocationLink)],
)
@dispatcher = dispatcher

Expand Down Expand Up @@ -104,7 +104,7 @@ def initialize(document, global_state, position, dispatcher, typechecker_enabled
@target = T.let(target, T.nilable(Prism::Node))
end

sig { override.returns(T::Array[Interface::Location]) }
sig { override.returns(T::Array[T.any(Interface::Location, Interface::LocationLink)]) }
def perform
@dispatcher.dispatch_once(@target) if @target
@response_builder.response
Expand Down
14 changes: 12 additions & 2 deletions test/expectations/definition/class_reference.exp.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
{
"result": [
{
"uri": "file:///fixtures/class_reference_target.rb",
"range": {
"targetUri": "file:///fixtures/class_reference_target.rb",
"targetSelectionRange": {
"start": {
"line": 4,
"character": 8
},
"end": {
"line": 4,
"character": 20
}
},
"targetRange": {
"start": {
"line": 4,
"character": 2
Expand Down
14 changes: 12 additions & 2 deletions test/expectations/definition/constant_reference.exp.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
{
"result": [
{
"uri": "file:///fixtures/constant_reference_target.rb",
"range": {
"targetUri": "file:///fixtures/constant_reference_target.rb",
"targetSelectionRange": {
"start": {
"line": 3,
"character": 7
},
"end": {
"line": 3,
"character": 10
}
},
"targetRange": {
"start": {
"line": 3,
"character": 0
Expand Down
47 changes: 27 additions & 20 deletions test/requests/definition_expectations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,15 @@ def run_expectations(source)
when Array
response.each do |location|
attributes = T.let(location.attributes, T.untyped)
fake_path = T.let(attributes[:uri].split("/").last(2).join("/"), String)
location.instance_variable_set(:@attributes, attributes.merge("uri" => "file:///#{fake_path}"))

case location
when RubyLsp::Interface::LocationLink
fake_path = T.let(attributes[:targetUri].split("/").last(2).join("/"), String)
location.instance_variable_set(:@attributes, attributes.merge("targetUri" => "file:///#{fake_path}"))
else
fake_path = T.let(attributes[:uri].split("/").last(2).join("/"), String)
location.instance_variable_set(:@attributes, attributes.merge("uri" => "file:///#{fake_path}"))
end
end
end

Expand Down Expand Up @@ -103,7 +110,7 @@ class Baz
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { line: 7, character: 0 } },
)
range = server.pop_response.response[0].attributes[:range].attributes
range = server.pop_response.response[0].attributes[:targetRange].attributes
range_hash = { start: range[:start].to_hash, end: range[:end].to_hash }
assert_equal({ start: { line: 0, character: 0 }, end: { line: 5, character: 3 } }, range_hash)

Expand All @@ -113,7 +120,7 @@ class Baz
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { line: 7, character: 5 } },
)
range = server.pop_response.response[0].attributes[:range].attributes
range = server.pop_response.response[0].attributes[:targetRange].attributes
range_hash = { start: range[:start].to_hash, end: range[:end].to_hash }
assert_equal({ start: { line: 1, character: 2 }, end: { line: 4, character: 5 } }, range_hash)

Expand All @@ -123,7 +130,7 @@ class Baz
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { line: 7, character: 10 } },
)
range = server.pop_response.response[0].attributes[:range].attributes
range = server.pop_response.response[0].attributes[:targetRange].attributes
range_hash = { start: range[:start].to_hash, end: range[:end].to_hash }
assert_equal({ start: { line: 2, character: 4 }, end: { line: 3, character: 7 } }, range_hash)
end
Expand Down Expand Up @@ -172,7 +179,7 @@ class A
response = server.pop_response

assert_instance_of(RubyLsp::Result, response)
assert_equal(uri.to_s, response.response.first.attributes[:uri])
assert_equal(uri.to_s, response.response.first.attributes[:targetUri])
end
end

Expand Down Expand Up @@ -222,7 +229,7 @@ def test_definition_addons
response = server.pop_response.response

assert_equal(2, response.size)
assert_match("class_reference_target.rb", response[0].uri)
assert_match("class_reference_target.rb", response[0].target_uri)
assert_match("generated_by_addon.rb", response[1].uri)
end
ensure
Expand All @@ -249,7 +256,7 @@ def foo; end
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { character: 4, line: 4 } },
)
assert_equal(uri.to_s, server.pop_response.response.first.attributes[:uri])
assert_equal(uri.to_s, server.pop_response.response.first.attributes[:targetUri])
end
end

Expand Down Expand Up @@ -295,8 +302,8 @@ def foo; end
)

first_definition, second_definition = server.pop_response.response
assert_equal(uri.to_s, first_definition.attributes[:uri])
assert_equal(second_uri.to_s, second_definition.attributes[:uri])
assert_equal(uri.to_s, first_definition.attributes[:targetUri])
assert_equal(second_uri.to_s, second_definition.attributes[:targetUri])
end
end

Expand All @@ -319,7 +326,7 @@ def foo; end
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { character: 9, line: 4 } },
)
assert_equal(uri.to_s, server.pop_response.response.first.attributes[:uri])
assert_equal(uri.to_s, server.pop_response.response.first.attributes[:targetUri])
end
end

Expand Down Expand Up @@ -367,7 +374,7 @@ def bar

assert_equal(1, response.size)

range = response[0].attributes[:range].attributes
range = response[0].attributes[:targetRange].attributes
range_hash = { start: range[:start].to_hash, end: range[:end].to_hash }
assert_equal({ start: { line: 3, character: 2 }, end: { line: 3, character: 14 } }, range_hash)
end
Expand Down Expand Up @@ -398,11 +405,11 @@ def foo; end

assert_equal(2, response.size)

range = response[0].attributes[:range].attributes
range = response[0].attributes[:targetRange].attributes
range_hash = { start: range[:start].to_hash, end: range[:end].to_hash }
assert_equal({ start: { line: 3, character: 2 }, end: { line: 3, character: 14 } }, range_hash)

range = response[1].attributes[:range].attributes
range = response[1].attributes[:targetRange].attributes
range_hash = { start: range[:start].to_hash, end: range[:end].to_hash }
assert_equal({ start: { line: 7, character: 2 }, end: { line: 7, character: 14 } }, range_hash)
end
Expand Down Expand Up @@ -450,14 +457,14 @@ def argument; end
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { character: 12, line: 6 } },
)
assert_equal(3, server.pop_response.response.first.range.start.line)
assert_equal(3, server.pop_response.response.first.target_range.start.line)

server.process_message(
id: 1,
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { character: 4, line: 6 } },
)
assert_equal(1, server.pop_response.response.first.range.start.line)
assert_equal(1, server.pop_response.response.first.target_range.start.line)
end
end

Expand All @@ -481,8 +488,8 @@ def baz
params: { textDocument: { uri: uri }, position: { character: 11, line: 6 } },
)
response = server.pop_response.response.first
assert_equal(1, response.range.start.line)
assert_equal(1, response.range.end.line)
assert_equal(1, response.target_range.start.line)
assert_equal(1, response.target_range.end.line)
end
end

Expand Down Expand Up @@ -551,15 +558,15 @@ def method3
params: { textDocument: { uri: uri }, position: { character: 6, line: 13 } },
)
response = server.pop_response.response.first
assert_equal(2, response.range.start.line)
assert_equal(2, response.target_range.start.line)

server.process_message(
id: 1,
method: "textDocument/definition",
params: { textDocument: { uri: uri }, position: { character: 6, line: 14 } },
)
response = server.pop_response.response.first
assert_equal(6, response.range.start.line)
assert_equal(6, response.target_range.start.line)
end
end

Expand Down
6 changes: 3 additions & 3 deletions vscode/src/test/suite/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
DocumentHighlightKind,
Hover,
WorkDoneProgress,
Location,
SemanticTokens,
DocumentLink,
WorkspaceSymbol,
Expand All @@ -22,6 +21,7 @@ import {
SelectionRange,
CodeAction,
TextDocumentFilter,
LocationLink,
} from "vscode-languageclient/node";
import { after, afterEach, before } from "mocha";

Expand Down Expand Up @@ -334,7 +334,7 @@ suite("Client", () => {
text,
},
});
const response: Location[] = await client.sendRequest(
const response: LocationLink[] = await client.sendRequest(
"textDocument/definition",
{
textDocument: {
Expand All @@ -345,7 +345,7 @@ suite("Client", () => {
);

assert.strictEqual(response.length, 1);
assert.match(response[0].uri, /server\.rb/);
assert.match(response[0].targetUri, /server\.rb/);
}).timeout(20000);

test("semantic highlighting", async () => {
Expand Down

0 comments on commit 58c694f

Please sign in to comment.