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

Code completion improvements #237

Merged
merged 9 commits into from
May 20, 2020
Merged

Code completion improvements #237

merged 9 commits into from
May 20, 2020

Conversation

skovhus
Copy link
Collaborator

@skovhus skovhus commented May 19, 2020

Fixes #236

This PR enables code completion improvements:

  • completion support for parameter expansions
  • lots of words are now parsed correctly like else (we had a bug in the handling around tree-sitter)
  • reduces some unnecessary complexities

Screenshots (before and after)

Screenshot 2020-05-20 10 42 28Screenshot 2020-05-20 10 42 34

Screenshot 2020-05-20 10 43 07Screenshot 2020-05-20 10 42 42

@skovhus skovhus marked this pull request as draft May 19, 2020 19:30
The helper was originally introduced to make the detection of a word at a given point
more accurate (this is used for completion, hovering, etc) and fixes some issues with the tree sitter grammar.

It turns out this is really not needed and introduces a bunch of issues.

Related to #192
@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #237 into master will decrease coverage by 0.35%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   73.92%   73.57%   -0.36%     
==========================================
  Files          18       18              
  Lines         560      545      -15     
  Branches       90       87       -3     
==========================================
- Hits          414      401      -13     
+ Misses        125      124       -1     
+ Partials       21       20       -1     
Impacted Files Coverage Δ
server/src/util/tree-sitter.ts 100.00% <ø> (+4.76%) ⬆️
server/src/server.ts 62.42% <90.00%> (+0.16%) ⬆️
server/src/analyser.ts 83.43% <100.00%> (+0.61%) ⬆️
server/src/util/sh.ts 84.00% <100.00%> (+1.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e441064...e6f759b. Read the comment docs.

@skovhus skovhus force-pushed the word-at-point-improvements branch from 62b101d to 01a692c Compare May 19, 2020 19:34
currentUri,
})

if (shouldCompleteOnVariables) {
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 an optimization to avoid creating the massive allCompletions array below?
If so, this if statement only returns when the user type:

echo $
echo ${
echo "$
echo "${

and if the user types anything after that, the value of shouldCompleteOnVariables will be false.
Is this desired behavior, or do we want shouldCompleteOnVariables to be true when the user has something like:

start="some variable"
# user begins to type 'st':
# and hopes to autocomplete 'start'
echo ${st

# shouldCompleteOnVariables is false though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for reading through this carefully.

is this an optimization to avoid creating the massive allCompletions array below?

This is actually not an optimization. The reason for the early exit is that we only want to suggest parameters/variables here if the word that we complete on is $ or ${.

and if the user types anything after that, the value of shouldCompleteOnVariables will be false.

From my experience, this is not how language server protocol clients work. They get completion for a word (e.g. $) once, meaning they do not continuously call the onCompletion while the user types. This makes our life a bit easier here, as we do not need to adjust while the user types out a word.

@@ -13,6 +13,8 @@ import { BashCompletionItem, CompletionItemDataType } from './types'
import { uniqueBasedOnHash } from './util/array'
import { getShellDocumentation } from './util/sh'

const PARAMETER_EXPANSION_PREFIXES = new Set(['$', '${', '${#'])

Copy link
Contributor

Choose a reason for hiding this comment

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

this variable is used to test if:

    const shouldCompleteOnVariables = word
      ? PARAMETER_EXPANSION_PREFIXES.has(word)
      : false

This check sets shouldCompleteOnVariables to true only for the first two, and only if the user hasnt typed anything else. I've done some tests, and I can't find a case where ${# gets detected.

@nikita-skobov
Copy link
Contributor

The PR looks very nice! I tested it out and it works well for most cases that I've tried.

I made a few comments about the variables that it shows for completion, and how basically, it will show you variables if you type: $ or ${ but if you start typing anything after that, it will show you all variables, builtins, reserved words, etc.

I think this issue is something with tree sitter like you said in #236

When you type ${a then the const word = this.getWordAtPoint word becomes a, instead of ${a. but maybe this is desired?

In either case, I think it looks good. 👍

@skovhus skovhus marked this pull request as ready for review May 20, 2020 09:11
@skovhus
Copy link
Collaborator Author

skovhus commented May 20, 2020

I made a few comments about the variables that it shows for completion, and how basically, it will show you variables if you type: $ or ${ but if you start typing anything after that, it will show you all variables, builtins, reserved words, etc.

I don't think this is the case for most use cases. As I commented above, the client of an LSP will only call once a word is started (e.g $), and not while the user types (e.g $MY_VARIA).

But if you go to an existing line with an unfinished word (e.g $MY_VARIA) and auto completes, then we do not currently know that we are in a parameter expansion, hence we will autocomplete on not only parameters (MY_VARIABLE), but also functions, built-ins, etc. This should be a future extension, where we based on the grammar, would see if the current word inside a parameter expansion.

I documented this in e6f759b

When you type ${a then the const word = this.getWordAtPoint word becomes a, instead of ${a. but maybe this is desired?

Yes, form the parser's side, a is a word inside the parameter expansion. So this is expected behavior.

@skovhus
Copy link
Collaborator Author

skovhus commented May 20, 2020

@rcjsuen @nikita-skobov thanks for the review. Much appreciated! 👏

@skovhus skovhus merged commit c23778c into master May 20, 2020
@skovhus skovhus deleted the word-at-point-improvements branch May 20, 2020 09:23
akurtakov added a commit to akurtakov/shellwax that referenced this pull request May 27, 2020
Language server changelog:
1.16.1

    Fix brace expansion bug
(bash-lsp/bash-language-server#240)
    Do not crash if bash is not installed
(bash-lsp/bash-language-server#242)

1.16.0

    Improved completion handler for parameter expansions
(bash-lsp/bash-language-server#237)

1.15.0

    Use comments above symbols for documentation
(bash-lsp/bash-language-server#234,
bash-lsp/bash-language-server#235)

1.14.0

    onHover and onCompletion documentation improvements
(bash-lsp/bash-language-server#230)
    support 0/1 as values for HIGHLIGHT_PARSING_ERRORS
(bash-lsp/bash-language-server#231)

1.13.1

    Gracefully handle glob failures
(bash-lsp/bash-language-server#224,
bash-lsp/bash-language-server#226)
    Maintenance
(bash-lsp/bash-language-server#222,
bash-lsp/bash-language-server#225)


Signed-off-by: Alexander Kurtakov <[email protected]>
akurtakov added a commit to eclipse-shellwax/shellwax that referenced this pull request May 27, 2020
Language server changelog:
1.16.1

    Fix brace expansion bug
(bash-lsp/bash-language-server#240)
    Do not crash if bash is not installed
(bash-lsp/bash-language-server#242)

1.16.0

    Improved completion handler for parameter expansions
(bash-lsp/bash-language-server#237)

1.15.0

    Use comments above symbols for documentation
(bash-lsp/bash-language-server#234,
bash-lsp/bash-language-server#235)

1.14.0

    onHover and onCompletion documentation improvements
(bash-lsp/bash-language-server#230)
    support 0/1 as values for HIGHLIGHT_PARSING_ERRORS
(bash-lsp/bash-language-server#231)

1.13.1

    Gracefully handle glob failures
(bash-lsp/bash-language-server#224,
bash-lsp/bash-language-server#226)
    Maintenance
(bash-lsp/bash-language-server#222,
bash-lsp/bash-language-server#225)


Signed-off-by: Alexander Kurtakov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

code completion of variables in quotes
3 participants