Skip to content
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.

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

Add autocomplete for classes, modules and constants #957

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

vinistock
Copy link
Member

Motivation

Closes #61

Add completion for classes, modules and constants.

Implementation

I'll explain the changes per file

  1. Executor: when a completion request is triggered, we receive the position for the character immediately after the letter the user just typed. So I started checking if the previous character (the one just inserted) is an uppercase character and then we go down the path of completion for classes, modules and constants. Otherwise, we do the previous path completion. Also started passing the nesting to the completion request and adapted our capabilities
  2. Completion: started handling const and const_path_ref to provide completion for both namespaced and regular constants. These events use the prefix_search added in Add entries prefix tree #955 and then build the completions. I also extracted the logic to build markdown content from hover into Common, so that we can display the exact same documentation on both hover and completion (the documentation shows up on the side when selecting a completion)

Other than that, I just renamed the request to be more generic, using Completion instead of PathCompletion.

Automated Tests

Added tests for both const and const path refs.

Manual Tests

  1. Open a codebase that does not use Sorbet (e.g.: rails/rails)
  2. Configure the Ruby LSP to use this branch "rubyLsp.branch": "vs/add_autocomplete"
  3. Type some uppercase letter somewhere
  4. Verify you get completion options
  5. Verify you can see the documentation properly when switching options (notice that in VS Code the documentation might be hidden. You need to click the little right arrow on top of a completion item to show docs)

@vinistock vinistock added the enhancement New feature or request label Aug 30, 2023
@vinistock vinistock added this to the 2023-Q3 milestone Aug 30, 2023
@vinistock vinistock self-assigned this Aug 30, 2023
@vinistock vinistock requested a review from a team as a code owner August 30, 2023 19:49
@vinistock vinistock requested review from andyw8 and st0012 August 30, 2023 19:49
@github-actions
Copy link
Contributor

Benchmark results in seconds (slowest at top)

          textDocument/diagnostic average: 0.042066 std_dev: 0.010297
          textDocument/definition average: 0.00485 std_dev: 0.002825
      textDocument/selectionRange average: 0.004171 std_dev: 0.000585
   textDocument/documentHighlight average: 0.002627 std_dev: 0.000196
        textDocument/documentLink average: 0.002564 std_dev: 0.000205
 textDocument/semanticTokens/full average: 0.002548 std_dev: 0.000222
            textDocument/codeLens average: 0.002537 std_dev: 0.000192
      textDocument/documentSymbol average: 0.002511 std_dev: 0.000152
        textDocument/foldingRange average: 0.002507 std_dev: 0.000218
textDocument/semanticTokens/range average: 0.001688 std_dev: 0.000124
               codeAction/resolve average: 0.00148 std_dev: 0.00013
           textDocument/inlayHint average: 0.00143 std_dev: 0.00012
               textDocument/hover average: 0.001416 std_dev: 0.000103
          textDocument/completion average: 0.001364 std_dev: 7.8e-05
          textDocument/formatting average: 0.000874 std_dev: 0.000296
    textDocument/onTypeFormatting average: 0.000873 std_dev: 8.3e-05
          textDocument/codeAction average: 0.000844 std_dev: 0.0001


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion faster by 99.331 %
            textDocument/codeLens unchanged
          textDocument/definition unchanged


================================================================================
Missing benchmarks:

RubyLsp::Requests::ShowSyntaxTree

# When the user types in the first letter of a constant name, we actually receive the position of the next
# immediate character. We check to see if the character is uppercase and then remove the offset to try to locate
# the node, as it could not be a constant
target_node_types = if ("A".."Z").cover?(document.source[char_position - 1])
Copy link
Contributor

Choose a reason for hiding this comment

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

What about constants beginning with letters from other alphabets, e.g. Á? 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just _FOO

Copy link
Contributor

Choose a reason for hiding this comment

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

_FOO would just be an odd-looking local variable:

irb(main):001:0> _FOO = 1
=> 1
irb(main):002:0> defined? _FOO
=> "local-variable"

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL!

Copy link
Member Author

Choose a reason for hiding this comment

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

How do we get all upper case characters including the ones with accents in Ruby? I never seen this done before. And we need to return the list of trigger characters to the editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe with /[[:upper:]]/, if that's consistent with what Ruby considers for constants:

irb(main):003> "Á".match?(/[[:upper:]]/)
=> true

Copy link
Contributor

Choose a reason for hiding this comment

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

(if you can find the lower and upper bounds, you can use that for the input)

Copy link
Contributor

Choose a reason for hiding this comment

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

With a little help from ChatGPT 😁

# Define the range of Unicode characters
unicode_range = 0..0x10FFFF

# Initialize an array to store the uppercase characters
uppercase_chars = []

# Iterate over the unicode range
unicode_range.each do |i|
  char = [i].pack('U*')
  if char =~ /[[:upper:]]/
    uppercase_chars << char
  end
rescue # to avoid 'invalid byte sequence in UTF-8' for some values
  nil 
end.compact

# Output the uppercase characters
puts uppercase_chars

(1951 results)

Copy link
Member Author

Choose a reason for hiding this comment

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

I created #990 to explore this in more detail. For now, let's ship without unicode support.

Copy link
Member

Choose a reason for hiding this comment

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

It is usually best to ask Ruby to compile a snippet with the given input and see if Ruby treats it as a constant:

pry> RubyVM::InstructionSequence.compile("Ã = 42", nil, nil, 0, false).to_a.dig(-1, -2, 0) == :setconstant
=> true
pry> RubyVM::InstructionSequence.compile("_FOO = 42", nil, nil, 0, false).to_a.dig(-1, -2, 0) == :setconstant
=> false
pry> RubyVM::InstructionSequence.compile("FOO = 42", nil, nil, 0, false).to_a.dig(-1, -2, 0) == :setconstant
=> true
pry> RubyVM::InstructionSequence.compile("Ҷ = 42", nil, nil, 0, false).to_a.dig(-1, -2, 0) == :setconstant
=> true

# When the user types in the first letter of a constant name, we actually receive the position of the next
# immediate character. We check to see if the character is uppercase and then remove the offset to try to locate
# the node, as it could not be a constant
target_node_types = if ("A".."Z").cover?(document.source[char_position - 1])
Copy link
Contributor

Choose a reason for hiding this comment

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

Or just _FOO

lib/ruby_lsp/executor.rb Outdated Show resolved Hide resolved
test/requests/completion_test.rb Show resolved Hide resolved
Constant::CompletionItemKind::REFERENCE
end

insertion_text = first_entry.name.dup
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this dup necessary? Is name mutable?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're mutating it on the lines below. If we don't dup it, we end up mutating the index entry itself.

We were actually bit by this in the past

a = first_entry.name # => Foo::Bar
a.delete_prefix!("Foo::")
first_entry.name # => Bar

@vinistock vinistock force-pushed the vs/add_entries_prefix_tree branch from 0647342 to 357a126 Compare September 1, 2023 17:59
Base automatically changed from vs/add_entries_prefix_tree to main September 1, 2023 18:29
@vinistock vinistock requested review from Morriar and andyw8 September 1, 2023 19:47
@vinistock vinistock merged commit 721dbda into main Sep 8, 2023
25 of 29 checks passed
@vinistock vinistock deleted the vs/add_autocomplete branch September 8, 2023 18:44
vinistock pushed a commit that referenced this pull request Feb 28, 2024
The example config as show would result in one config key being overwritten by the other so I thought it would be better to simplify the example to show one instance of the `rubyLsp.bundleGemfile` key and adding a note about relative and absolute path support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add completion for classes and modules
4 participants