Skip to content

Support Sorbet typed: false files for completion requests #1245

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.

Sign up for GitHub

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

Merged
andyw8 merged 7 commits into from
Dec 18, 2023
Merged

Support Sorbet typed: false files for completion requests #1245

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
659b07d
enable completion on typed: false files
SeanKG Dec 8, 2023
5c6364c
Add tests for completion.rb
SeanKG Dec 11, 2023
4d3c576
use prism to look for magic comments
SeanKG Dec 12, 2023
de8f237
rename method and clean up tests
SeanKG Dec 12, 2023
134bad2
use asser/refute_predicate
SeanKG Dec 14, 2023
2f933d2
Add a test for other comments / strings
SeanKG Dec 14, 2023
7545635
Remove completion on constants
SeanKG Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/ruby_lsp/document.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,16 @@ def locate(node, char_position, node_types: [])
[closest, parent, nesting.map { |n| n.constant_path.location.slice }]
end

sig { returns(T::Boolean) }
def sigil_is_true_or_higher?
[/# typed: true/, /# typed: strict/, /# typed: strong/].any? { |pattern| pattern.match?(@source) }
end

sig { returns(T::Boolean) }
def typechecker_enabled?
DependencyDetector.instance.typechecker && sigil_is_true_or_higher?
end

class Scanner
extend T::Sig

Expand Down
2 changes: 1 addition & 1 deletion lib/ruby_lsp/executor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ def completion(uri, position)
return unless target

dispatcher = Prism::Dispatcher.new
listener = Requests::Completion.new(@index, nesting, dispatcher)
listener = Requests::Completion.new(@index, nesting, dispatcher, document.typechecker_enabled?)
dispatcher.dispatch_once(target)
listener.response
end
Expand Down
10 changes: 6 additions & 4 deletions lib/ruby_lsp/requests/completion.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@ class Completion < Listener
index: RubyIndexer::Index,
nesting: T::Array[String],
dispatcher: Prism::Dispatcher,
typechecker_enabled: T::Boolean,
).void
end
def initialize(index, nesting, dispatcher)
def initialize(index, nesting, dispatcher, typechecker_enabled)
super(dispatcher)
@_response = T.let([], ResponseType)
@index = index
@nesting = nesting
@typechecker_enabled = typechecker_enabled

dispatcher.register(
self,
Expand All @@ -63,7 +65,7 @@ def on_string_node_enter(node)
# Handle completion on regular constant references (e.g. `Bar`)
sig { params(node: Prism::ConstantReadNode).void }
def on_constant_read_node_enter(node)
return if DependencyDetector.instance.typechecker
return if @typechecker_enabled
Copy link
Member

@vinistock vinistock Dec 14, 2023

Choose a reason for hiding this comment

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

Seeing these changes for constants made me wonder why Sorbet wasn't providing hover or completion for them on typed: false files. It can accurately report diagnostics for them, so I thought it should just work.

After talking to the team, hover now works for constants in typed: false files and I opened a PR for completion. Definition already worked for constants.

If the PR is approved, we can maintain the same check we had before, since Sorbet will always be able to handle completion for constants.

Copy link
Contributor Author

@SeanKG SeanKG Dec 15, 2023

Choose a reason for hiding this comment

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

Oh nice yeah I mean if sorbet can cover this then that works! I had sorta considered that but it was way easier for me to understand how to change this code base.

Will we get 1:1 support for typed: false from the sorbet LS as this one though?

I know constants will be covered but I think you all are working on more support for methods in here as well right? Still seems like it could be valuable to know at the document level what's going on with typechecking.

Obviously this is kind of an edge case - It's on my roadmap to go get us past typed: false but it may take some time still. We need to dedicate some time to supporting that push properly and have a few other priorities before we can take the time to do that. We kinda yolo'd into it on first pass and it was messy / got in the way of devs quite a bit so we had to regress for a time. Hence why we have sorbet installed, but are largely typed: false for now.

Anywhoo context aside I still think this could be valuable but I'm also happy to wait for a concrete use-case if the current one gets covered by sorbet!

Copy link
Member

@vinistock vinistock Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, this change still makes perfect sense for methods. Sorbet does not even attempt to typecheck typed: false files, so we can definitely fallback method hover, completion, signature help and definition to the Ruby LSP.

For constants though, we can fully delegate to Sorbet. It's always able to discover and handle constants properly (meta-programming excluded of course).

So let's just revert the changes to the early returns for constant reads and constant path reads and leave it only for call nodes. Then we should be good to go.

Copy link
Contributor Author

@SeanKG SeanKG Dec 18, 2023

Choose a reason for hiding this comment

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

Right yeah that makes sense. I just pushed those changes up now 👍


name = node.slice
candidates = @index.prefix_search(name, @nesting)
Expand All @@ -82,7 +84,7 @@ def on_constant_read_node_enter(node)
# Handle completion on namespaced constant references (e.g. `Foo::Bar`)
sig { params(node: Prism::ConstantPathNode).void }
def on_constant_path_node_enter(node)
return if DependencyDetector.instance.typechecker
return if @typechecker_enabled

name = node.slice

Expand Down Expand Up @@ -125,7 +127,7 @@ def on_constant_path_node_enter(node)

sig { params(node: Prism::CallNode).void }
def on_call_node_enter(node)
return if DependencyDetector.instance.typechecker
return if @typechecker_enabled
return unless self_receiver?(node)

name = node.message
Expand Down
48 changes: 44 additions & 4 deletions test/requests/completion_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ def setup
@uri = URI("file:///fake.rb")
@store = RubyLsp::Store.new
@executor = RubyLsp::Executor.new(@store, @message_queue)
stub_no_typechecker
end

def teardown
Expand Down Expand Up @@ -338,7 +337,6 @@ class A
end

def test_completion_for_aliased_constants
stub_no_typechecker
document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri)
module A
module B
Expand Down Expand Up @@ -370,7 +368,6 @@ module Other
end

def test_completion_for_aliased_complex_constants
stub_no_typechecker
document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri)
module A
module B
Expand Down Expand Up @@ -403,7 +400,6 @@ module Other
end

def test_completion_uses_shortest_possible_name_for_filter_text
stub_no_typechecker
document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri)
module A
module B
Expand Down Expand Up @@ -542,6 +538,50 @@ def qux
assert_equal(["bar", "bar=(bar)"], result.map { |completion| completion.text_edit.new_text })
end

def test_with_typed_false
document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri)
# typed: false
class Foo
end

F
RUBY

end_position = { line: 4, character: 1 }
@store.set(uri: @uri, source: document.source, version: 1)

index = @executor.instance_variable_get(:@index)
index.index_single(RubyIndexer::IndexablePath.new(nil, @uri.to_standardized_path), document.source)

result = run_request(
method: "textDocument/completion",
params: { textDocument: { uri: @uri.to_s }, position: end_position },
)
assert_equal(["Foo"], result.map(&:label))
end

def test_with_typed_true
document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri)
# typed: true
class Foo
end

F
RUBY

end_position = { line: 4, character: 1 }
@store.set(uri: @uri, source: document.source, version: 1)

index = @executor.instance_variable_get(:@index)
index.index_single(RubyIndexer::IndexablePath.new(nil, @uri.to_standardized_path), document.source)

result = run_request(
method: "textDocument/completion",
params: { textDocument: { uri: @uri.to_s }, position: end_position },
)
assert_empty(result)
end

private

def run_request(method:, params: {})
Expand Down
46 changes: 46 additions & 0 deletions test/ruby_document_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,52 @@ def test_cache_set_and_get
assert_equal(value, document.cache_get("textDocument/semanticHighlighting"))
end

def test_no_sigil
document = RubyLsp::RubyDocument.new(
source: +"# frozen_string_literal: true",
version: 1,
uri: URI("file:///foo/bar.rb"),
)
value = false

assert_equal(value, document.sigil_is_true_or_higher?)
end

def test_sigil_ignore
document = RubyLsp::RubyDocument.new(source: +"# typed: ignored", version: 1, uri: URI("file:///foo/bar.rb"))
value = false

assert_equal(value, document.sigil_is_true_or_higher?)
end

def test_sigil_false
document = RubyLsp::RubyDocument.new(source: +"# typed: false", version: 1, uri: URI("file:///foo/bar.rb"))
value = false

assert_equal(value, document.sigil_is_true_or_higher?)
end

def test_sigil_true
document = RubyLsp::RubyDocument.new(source: +"# typed: true", version: 1, uri: URI("file:///foo/bar.rb"))
value = true

assert_equal(value, document.sigil_is_true_or_higher?)
end

def test_sigil_strict
document = RubyLsp::RubyDocument.new(source: +"# typed: strict", version: 1, uri: URI("file:///foo/bar.rb"))
value = true

assert_equal(value, document.sigil_is_true_or_higher?)
end

def test_sigil_strong
document = RubyLsp::RubyDocument.new(source: +"# typed: strong", version: 1, uri: URI("file:///foo/bar.rb"))
value = true

assert_equal(value, document.sigil_is_true_or_higher?)
end

private

def assert_error_edit(actual, error_range)
Expand Down