From e597de15fce1c72fbad0302bfbaa31728874f9a9 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 20 Jun 2024 14:23:09 -0400 Subject: [PATCH] Address PR feedback --- DESIGN_AND_ROADMAP.md | 16 ++++++++-------- lib/ruby_lsp/listeners/completion.rb | 5 +++-- lib/ruby_lsp/listeners/definition.rb | 2 +- lib/ruby_lsp/listeners/hover.rb | 5 ++--- lib/ruby_lsp/listeners/signature_help.rb | 5 ++--- lib/ruby_lsp/requests/completion_resolve.rb | 9 +++++---- lib/ruby_lsp/utils.rb | 1 + test/requests/completion_resolve_test.rb | 4 ++-- test/requests/hover_expectations_test.rb | 2 +- test/requests/signature_help_test.rb | 2 +- test/type_inferrer_test.rb | 4 ++-- 11 files changed, 28 insertions(+), 27 deletions(-) diff --git a/DESIGN_AND_ROADMAP.md b/DESIGN_AND_ROADMAP.md index b4bc4e3eb..ec6951aaf 100644 --- a/DESIGN_AND_ROADMAP.md +++ b/DESIGN_AND_ROADMAP.md @@ -221,7 +221,7 @@ Interested in contributing? Check out the issues tagged with [help-wanted] or [g ## Guessed types -Guessed types is an experimental features where the Ruby LSP attempts to identify the type of a receiver based on its +Guessed types is an experimental feature where the Ruby LSP attempts to identify the type of a receiver based on its identifier name. For example: ```ruby @@ -232,13 +232,13 @@ user.name @post.like! ``` -IMPORTANT: The goal of this experiment is to understand if we can get better accuracy for the code that you already -have. The hypothesis is that a reasonable amount of code already uses patterns like the ones in the example and, in -those cases, we can achieve nicer results. - -However, identifiers are not the ideal medium for proper type annotations. It would not be possible to express anything -complex, such as unions, intersections or generics. Additionally, it is very trivial to fool the type guessing by simply -naming a variable with a type name that doesn't match its actual type. +> [!IMPORTANT] The goal of this experiment is to understand if we can get better accuracy for the code that you already +> have. The hypothesis is that a reasonable amount of code already uses patterns like the ones in the example and, in +> those cases, we can achieve nicer results. +> +> However, identifiers are not the ideal medium for proper type annotations. It would not be possible to express anything +> complex, such as unions, intersections or generics. Additionally, it is very trivial to fool the type guessing by simply +> naming a variable with a type name that doesn't match its actual type. ```ruby pathname = something_that_returns_an_integer diff --git a/lib/ruby_lsp/listeners/completion.rb b/lib/ruby_lsp/listeners/completion.rb index 814cec608..7bbeb9c9c 100644 --- a/lib/ruby_lsp/listeners/completion.rb +++ b/lib/ruby_lsp/listeners/completion.rb @@ -366,10 +366,11 @@ def complete_methods(node, name) return unless range - guessed_type = type.is_a?(TypeInferrer::GuessedType) + guessed_type = type.name @index.method_completion_candidates(method_name, type.name).each do |entry| entry_name = entry.name + owner_name = entry.owner&.name label_details = Interface::CompletionItemLabelDetails.new( description: entry.file_name, @@ -382,7 +383,7 @@ def complete_methods(node, name) text_edit: Interface::TextEdit.new(range: range, new_text: entry_name), kind: Constant::CompletionItemKind::METHOD, data: { - owner_name: entry.owner&.name, + owner_name: owner_name, guessed_type: guessed_type, }, ) diff --git a/lib/ruby_lsp/listeners/definition.rb b/lib/ruby_lsp/listeners/definition.rb index a2fc640a8..e7a201058 100644 --- a/lib/ruby_lsp/listeners/definition.rb +++ b/lib/ruby_lsp/listeners/definition.rb @@ -65,7 +65,7 @@ def on_call_node_enter(node) # Until we can properly infer the receiver type in erb files (maybe with ruby-lsp-rails), # treating method calls' type as `nil` will allow users to get some completion support first - if @language_id == Document::LanguageId::ERB && inferrer_receiver_type == "Object" + if @language_id == Document::LanguageId::ERB && inferrer_receiver_type&.name == "Object" inferrer_receiver_type = nil end diff --git a/lib/ruby_lsp/listeners/hover.rb b/lib/ruby_lsp/listeners/hover.rb index f208d575c..65457bd81 100644 --- a/lib/ruby_lsp/listeners/hover.rb +++ b/lib/ruby_lsp/listeners/hover.rb @@ -177,9 +177,8 @@ def handle_method_hover(message, inherited_only: false) title = "#{message}#{T.must(methods.first).decorated_parameters}" if type.is_a?(TypeInferrer::GuessedType) - title << " | guessed receiver: #{type.name}" - link = "https://github.com/Shopify/ruby-lsp/blob/main/DESIGN_AND_ROADMAP.md#guessed-types" - @response_builder.push("[Learn more about guessed types](#{link})\n", category: :links) + title << "\n\nGuessed receiver: #{type.name}" + @response_builder.push("[Learn more about guessed types](#{GUESSED_TYPES_URL})\n", category: :links) end categorized_markdown_from_index_entries(title, methods).each do |category, content| diff --git a/lib/ruby_lsp/listeners/signature_help.rb b/lib/ruby_lsp/listeners/signature_help.rb index e97dc3000..7994870e5 100644 --- a/lib/ruby_lsp/listeners/signature_help.rb +++ b/lib/ruby_lsp/listeners/signature_help.rb @@ -64,9 +64,8 @@ def on_call_node_enter(node) title = +"" extra_links = if type.is_a?(TypeInferrer::GuessedType) - title << "guessed receiver: #{type.name}" - link = "https://github.com/Shopify/ruby-lsp/blob/main/DESIGN_AND_ROADMAP.md#guessed-types" - "[Learn more about guessed types](#{link})" + title << "\n\nGuessed receiver: #{type.name}" + "[Learn more about guessed types](#{GUESSED_TYPES_URL})" end signature_help = Interface::SignatureHelp.new( diff --git a/lib/ruby_lsp/requests/completion_resolve.rb b/lib/ruby_lsp/requests/completion_resolve.rb index 20de6d9a8..5dd79ba78 100644 --- a/lib/ruby_lsp/requests/completion_resolve.rb +++ b/lib/ruby_lsp/requests/completion_resolve.rb @@ -65,10 +65,11 @@ def perform label = +"#{label}#{first_entry.decorated_parameters}" end - extra_links = if @item.dig(:data, :guessed_type) - label << " | guessed receiver: #{owner_name}" - link = "https://github.com/Shopify/ruby-lsp/blob/main/DESIGN_AND_ROADMAP.md#guessed-types" - "[Learn more about guessed types](#{link})" + guessed_type = @item.dig(:data, :guessed_type) + + extra_links = if guessed_type + label << "\n\nGuessed receiver: #{guessed_type}" + "[Learn more about guessed types](#{GUESSED_TYPES_URL})" end @item[:documentation] = Interface::MarkupContent.new( diff --git a/lib/ruby_lsp/utils.rb b/lib/ruby_lsp/utils.rb index 8fb6758f1..ecdeea048 100644 --- a/lib/ruby_lsp/utils.rb +++ b/lib/ruby_lsp/utils.rb @@ -25,6 +25,7 @@ module RubyLsp end, String, ) + GUESSED_TYPES_URL = "https://github.com/Shopify/ruby-lsp/blob/main/DESIGN_AND_ROADMAP.md#guessed-types" # A notification to be sent to the client class Message diff --git a/test/requests/completion_resolve_test.rb b/test/requests/completion_resolve_test.rb index 6de982ab5..bace349d2 100644 --- a/test/requests/completion_resolve_test.rb +++ b/test/requests/completion_resolve_test.rb @@ -122,13 +122,13 @@ def foo(a, b, c) existing_item = { label: "foo", kind: RubyLsp::Constant::CompletionItemKind::METHOD, - data: { owner_name: "User", guessed_type: true }, + data: { owner_name: "User", guessed_type: "User" }, } server.process_message(id: 1, method: "completionItem/resolve", params: existing_item) result = server.pop_response.response - assert_match("guessed receiver: User", result[:documentation].value) + assert_match("Guessed receiver: User", result[:documentation].value) assert_match("Learn more about guessed types", result[:documentation].value) end end diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index 71b461d5a..9ab6ed343 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -707,7 +707,7 @@ def name; end ) contents = server.pop_response.response.contents.value - assert_match("guessed receiver: User", contents) + assert_match("Guessed receiver: User", contents) assert_match("Learn more about guessed types", contents) end end diff --git a/test/requests/signature_help_test.rb b/test/requests/signature_help_test.rb index 1a2ae67b0..8f7d1ec99 100644 --- a/test/requests/signature_help_test.rb +++ b/test/requests/signature_help_test.rb @@ -392,7 +392,7 @@ def subscribe!(news_letter) signature = result.signatures.first assert_equal("subscribe!(news_letter)", signature.label) - assert_match("guessed receiver: User", signature.documentation.value) + assert_match("Guessed receiver: User", signature.documentation.value) end end end diff --git a/test/type_inferrer_test.rb b/test/type_inferrer_test.rb index c2a94db75..eb26abaef 100644 --- a/test/type_inferrer_test.rb +++ b/test/type_inferrer_test.rb @@ -234,7 +234,7 @@ def initialize end RUBY - assert_equal("Foo", @type_inferrer.infer_receiver_type(node_context)) + assert_equal("Foo", @type_inferrer.infer_receiver_type(node_context).name) end def test_infer_super_receiver @@ -246,7 +246,7 @@ def initialize(a, b, c) end RUBY - assert_equal("Foo", @type_inferrer.infer_receiver_type(node_context)) + assert_equal("Foo", @type_inferrer.infer_receiver_type(node_context).name) end private