From 659b07d8ebc1f70e616c56c47f8e8a7170d9fa05 Mon Sep 17 00:00:00 2001 From: Sean Graves Date: Fri, 8 Dec 2023 14:40:47 -0500 Subject: [PATCH 1/7] enable completion on typed: false files Co-authored-by: Joshua Scharf --- lib/ruby_lsp/document.rb | 10 +++++++ lib/ruby_lsp/executor.rb | 2 +- lib/ruby_lsp/requests/completion.rb | 10 ++++--- test/ruby_document_test.rb | 46 +++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 5 deletions(-) diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index efd6ecdb7e..d0710a4961 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -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 diff --git a/lib/ruby_lsp/executor.rb b/lib/ruby_lsp/executor.rb index 84ce34208c..a7ef6918ed 100644 --- a/lib/ruby_lsp/executor.rb +++ b/lib/ruby_lsp/executor.rb @@ -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 diff --git a/lib/ruby_lsp/requests/completion.rb b/lib/ruby_lsp/requests/completion.rb index 5391ebc697..ad0fe7f9cc 100644 --- a/lib/ruby_lsp/requests/completion.rb +++ b/lib/ruby_lsp/requests/completion.rb @@ -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, @@ -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 name = node.slice candidates = @index.prefix_search(name, @nesting) @@ -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 @@ -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 diff --git a/test/ruby_document_test.rb b/test/ruby_document_test.rb index b8bbc89670..ab0cdb915a 100644 --- a/test/ruby_document_test.rb +++ b/test/ruby_document_test.rb @@ -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) From 5c6364cd424ddcb92b5cb11ef035f47377e0a2ad Mon Sep 17 00:00:00 2001 From: Sean Graves Date: Mon, 11 Dec 2023 15:52:40 -0500 Subject: [PATCH 2/7] Add tests for completion.rb Co-authored-by: Joshua Scharf --- test/requests/completion_test.rb | 48 +++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/test/requests/completion_test.rb b/test/requests/completion_test.rb index f48980fd65..1211a98ee8 100644 --- a/test/requests/completion_test.rb +++ b/test/requests/completion_test.rb @@ -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 @@ -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 @@ -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 @@ -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 @@ -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: {}) From 4d3c5766502f93381a4de86d23fb8d86429cb861 Mon Sep 17 00:00:00 2001 From: Sean Graves Date: Tue, 12 Dec 2023 13:53:21 -0500 Subject: [PATCH 3/7] use prism to look for magic comments --- lib/ruby_lsp/document.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index d0710a4961..0203816179 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -179,7 +179,9 @@ def locate(node, char_position, node_types: []) sig { returns(T::Boolean) } def sigil_is_true_or_higher? - [/# typed: true/, /# typed: strict/, /# typed: strong/].any? { |pattern| pattern.match?(@source) } + parse_result.magic_comments.any? do |comment| + comment.key == "typed" && ["true", "strict", "strong"].include?(comment.value) + end end sig { returns(T::Boolean) } From de8f2379198d1c648b2718372c21165b8b895296 Mon Sep 17 00:00:00 2001 From: Sean Graves Date: Tue, 12 Dec 2023 14:56:37 -0500 Subject: [PATCH 4/7] rename method and clean up tests --- lib/ruby_lsp/document.rb | 4 ++-- test/ruby_document_test.rb | 24 ++++++------------------ 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index 0203816179..76cd679a94 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -178,7 +178,7 @@ def locate(node, char_position, node_types: []) end sig { returns(T::Boolean) } - def sigil_is_true_or_higher? + def sorbet_sigil_is_true_or_higher parse_result.magic_comments.any? do |comment| comment.key == "typed" && ["true", "strict", "strong"].include?(comment.value) end @@ -186,7 +186,7 @@ def sigil_is_true_or_higher? sig { returns(T::Boolean) } def typechecker_enabled? - DependencyDetector.instance.typechecker && sigil_is_true_or_higher? + DependencyDetector.instance.typechecker && sorbet_sigil_is_true_or_higher end class Scanner diff --git a/test/ruby_document_test.rb b/test/ruby_document_test.rb index ab0cdb915a..3a6204ecb2 100644 --- a/test/ruby_document_test.rb +++ b/test/ruby_document_test.rb @@ -542,44 +542,32 @@ def test_no_sigil version: 1, uri: URI("file:///foo/bar.rb"), ) - value = false - - assert_equal(value, document.sigil_is_true_or_higher?) + refute(document.sorbet_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?) + refute(document.sorbet_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?) + refute(document.sorbet_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?) + assert(document.sorbet_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?) + assert(document.sorbet_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?) + assert(document.sorbet_sigil_is_true_or_higher) end private From 134bad278566b4f9b650f0af5b76edfd6bc54625 Mon Sep 17 00:00:00 2001 From: Sean Graves Date: Thu, 14 Dec 2023 14:02:00 -0500 Subject: [PATCH 5/7] use asser/refute_predicate --- test/ruby_document_test.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/ruby_document_test.rb b/test/ruby_document_test.rb index 3a6204ecb2..668964d2f8 100644 --- a/test/ruby_document_test.rb +++ b/test/ruby_document_test.rb @@ -542,32 +542,32 @@ def test_no_sigil version: 1, uri: URI("file:///foo/bar.rb"), ) - refute(document.sorbet_sigil_is_true_or_higher) + refute_predicate(document, :sorbet_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")) - refute(document.sorbet_sigil_is_true_or_higher) + refute_predicate(document, :sorbet_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")) - refute(document.sorbet_sigil_is_true_or_higher) + refute_predicate(document, :sorbet_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")) - assert(document.sorbet_sigil_is_true_or_higher) + assert_predicate(document, :sorbet_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")) - assert(document.sorbet_sigil_is_true_or_higher) + assert_predicate(document, :sorbet_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")) - assert(document.sorbet_sigil_is_true_or_higher) + assert_predicate(document, :sorbet_sigil_is_true_or_higher) end private From 2f933d21a955fda9d5ea9780895409f7a06764f6 Mon Sep 17 00:00:00 2001 From: Sean Graves Date: Thu, 14 Dec 2023 14:10:44 -0500 Subject: [PATCH 6/7] Add a test for other comments / strings --- test/ruby_document_test.rb | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/ruby_document_test.rb b/test/ruby_document_test.rb index 668964d2f8..ab9a8b14ff 100644 --- a/test/ruby_document_test.rb +++ b/test/ruby_document_test.rb @@ -570,6 +570,31 @@ def test_sigil_strong assert_predicate(document, :sorbet_sigil_is_true_or_higher) end + def test_sorbet_sigil_only_in_magic_comment + document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: URI("file:///foo/bar.rb")) + # typed: false + + def foo + some_string = "# typed: true" + end + + # Shouldn't be tricked by the following comment: + # ``` + # # typed: strict + # + # def main; end + # ``` + def bar; end + + def baz + <<-CODE + # typed: strong + CODE + end + RUBY + refute_predicate(document, :sorbet_sigil_is_true_or_higher) + end + private def assert_error_edit(actual, error_range) From 75456351545f4cbc29c070aa731bd9e688651044 Mon Sep 17 00:00:00 2001 From: Sean Graves Date: Mon, 18 Dec 2023 14:45:32 -0500 Subject: [PATCH 7/7] Remove completion on constants --- lib/ruby_lsp/requests/completion.rb | 4 ++-- test/requests/completion_test.rb | 30 ++++++++++++++++++++++------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/lib/ruby_lsp/requests/completion.rb b/lib/ruby_lsp/requests/completion.rb index ad0fe7f9cc..a9a619afc4 100644 --- a/lib/ruby_lsp/requests/completion.rb +++ b/lib/ruby_lsp/requests/completion.rb @@ -65,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 @typechecker_enabled + return if DependencyDetector.instance.typechecker name = node.slice candidates = @index.prefix_search(name, @nesting) @@ -84,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 @typechecker_enabled + return if DependencyDetector.instance.typechecker name = node.slice diff --git a/test/requests/completion_test.rb b/test/requests/completion_test.rb index 1211a98ee8..c6e2995d6a 100644 --- a/test/requests/completion_test.rb +++ b/test/requests/completion_test.rb @@ -174,6 +174,7 @@ def test_completion_is_not_triggered_if_argument_is_not_a_string end def test_completion_for_constants + stub_no_typechecker document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri) class Foo end @@ -195,6 +196,7 @@ class Foo end def test_completion_for_constant_paths + stub_no_typechecker document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri) class Bar end @@ -234,6 +236,7 @@ module Foo end def test_completion_conflicting_constants + stub_no_typechecker document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri) module Foo class Qux; end @@ -263,6 +266,7 @@ class Qux; end end def test_completion_for_top_level_constants_inside_nesting + stub_no_typechecker document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri) class Bar end @@ -291,6 +295,7 @@ module Foo end def test_completion_private_constants_inside_the_same_namespace + stub_no_typechecker document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri) class A CONST = 1 @@ -337,6 +342,7 @@ 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 @@ -368,6 +374,7 @@ 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 @@ -400,6 +407,7 @@ 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 @@ -542,12 +550,16 @@ def test_with_typed_false document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri) # typed: false class Foo - end + def complete_me + end - F + def you + comp + end + end RUBY - end_position = { line: 4, character: 1 } + end_position = { line: 6, character: 8 } @store.set(uri: @uri, source: document.source, version: 1) index = @executor.instance_variable_get(:@index) @@ -557,19 +569,23 @@ class Foo method: "textDocument/completion", params: { textDocument: { uri: @uri.to_s }, position: end_position }, ) - assert_equal(["Foo"], result.map(&:label)) + assert_equal(["complete_me"], result.map(&:label)) end def test_with_typed_true document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: @uri) # typed: true class Foo - end + def complete_me + end - F + def you + comp + end + end RUBY - end_position = { line: 4, character: 1 } + end_position = { line: 6, character: 8 } @store.set(uri: @uri, source: document.source, version: 1) index = @executor.instance_variable_get(:@index)