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

CompletionItemProvider#resolve not always called #26622

Closed
kdvolder opened this issue May 14, 2017 · 8 comments
Closed

CompletionItemProvider#resolve not always called #26622

kdvolder opened this issue May 14, 2017 · 8 comments
Assignees
Labels
suggest IntelliSense, Auto Complete under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@kdvolder
Copy link

kdvolder commented May 14, 2017

  • VSCode Version:
$ code --version
1.12.1
f6868fce3eeb16663840eb82123369dec6077a9b
  • OS Version: Ubuntu 14.04 LTS

Steps to Reproduce:

  1. Install the extension from this vsix file: https://s3-us-west-1.amazonaws.com/s3-test.spring.io/sts4/vscode-extensions/snapshots/vscode-concourse-0.0.6-RC.1.vsix
  2. Create a file called 'pipeline.yml' and put this text in it:
resources:
- name: demo-repo
  type: git
  source:
    uri: git@github.com/kdvolder/demo-repo
    <*>
  1. The <*> marks the cursor/caret position. So delete it and place cursor there.

  2. Press CTRL-SPACE to invoke content assist.

Now, in order to see the problem. Only use the mouse to invoke the last proposal in the list. I.e Do not use the cursor keys, also Do no type to narrow the proposals. Instead, scroll-down (using mouse or scrollwheel) to the last proposal its text is ← ← - name. Then click the proposal with the mouse to invoke it.

=> Observe: The completion simply inserts the label text. So the editor contents becomes this:

resources:
- name: demo-repo
  type: git
  source:
    uri: git@github.com/kdvolder/demo-repo
    ← ← - name

This is wrong. Compare this to the intended/correct behaviour which you can get by using cursor keys (instead of mouse) to move down and select the proposal, then press ENTER to invoke it. In this case the editor contents becomes this:

resources:
- name: demo-repo
  type: git
  source:
    uri: git@github.com/kdvolder/demo-repo
- name: 

I speculate that the reason for the discrepancy is as follows:

  1. our completions must be resolved to get the doc-string as well as fill in the edit.
  2. When we use cursor navigation the doc-strings are revealed in the UI, and this forces the details, including the edits to be filled in.
  3. When we directly click on a completion, then its doc string is never revealed and the item is therefore not being resolved. Thus, the item being invoked doesn't have a textEdit and Vscode just defaults to inserting the label text at the cursor instead.
@jrieken jrieken added suggest IntelliSense, Auto Complete bug Issue identified by VS Code Team member as probable bug labels Jul 24, 2017
@jrieken jrieken changed the title Lazy resolved completions (from LSP server) not always resolved properly CompletionItemProvider#resolve not always called Jul 24, 2017
@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label Jul 24, 2017
@jrieken
Copy link
Member

jrieken commented Jul 24, 2017

Thus, the item being invoked doesn't have a textEdit and Vscode just defaults to inserting the label text at the cursor instead.

The text edit, better the range and insertText, is needed early on before filtering and scoring happens because they play a role in how that works. Otherwise sorting would change when going up and down in the list of suggestions. Because of that it is unlikely that we simple allow to change these things after the fact (that we do is maybe the actual bug)

@kdvolder
Copy link
Author

It is not at all clear from the language-server specification that the text edits aren't supposed to be lazy resolved. By my reading of it, I was assuming that lazy resolution of both the edits and the doc strings would be something one might do 'lazyly'.

Now, I went back to the spec and I have to admit this is more based on my own intuition and the fact that both computing complex set of edits and doc strings could be something 'expensive'. It is not really somthing spec says literally. In fact, the spec doesn't seem to say what qualifies as 'additional information' that is computed by lazy resolution.
Per my reading I interpreted it to include 'edits' as well as 'doc string' because, intuitively one would expect that the edits are not needed / used until a particular completion is selected and applied by the user.

Yes, I understand your comment about sorting, but that is really not that intuitive at all, especially given the fact that language server spec also includes a sort string in the item (which now, as I understand, is, for the most part, being ignored by vscode, in most cases).

I think there's a bit of a dilemma here that

  • I think its reasonable to expect/support lazy resolition of complex edits
  • the particular way in which vscode uses the edit in determining sorting makes this somewhat impossible or pointless (i.e. if you have to resolve every item before sorting, then its pointless to make them lazy).

I do think this 'dilemma' is probably more of an idiosyncracy in vsode's implementation/interpretation of the spec. I would guess that other language server client implementation, probably don't use the edit text in this way. Call it an educated guess, I have not confirmed this, but simply relying on the sortText for sorting is the more straighforward way to interpret the spec.

@jrieken jrieken closed this as completed Nov 20, 2017
@jrieken jrieken reopened this Nov 20, 2017
@jrieken jrieken removed the bug Issue identified by VS Code Team member as probable bug label Nov 20, 2017
@jrieken jrieken assigned dbaeumer and unassigned jrieken Nov 20, 2017
@jrieken
Copy link
Member

jrieken commented Nov 20, 2017

Assigining to @dbaeumer for the LSP discussion. We have no plan on changing how items are sorted and since that depends on the text edit that cannot be lazy in VS Code.

@dbaeumer
Copy link
Member

@jrieken can you enlighten me why the sorting depends on text edits. I can easily argue that we need the label, sort text, ...

@dbaeumer
Copy link
Member

... and shouldn't we document that in the vscode.d.ts as well. It is not just LSP being underspecified :-)

@dbaeumer dbaeumer added this to the November 2017 milestone Nov 21, 2017
jrieken added a commit that referenced this issue Nov 21, 2017
@kdvolder
Copy link
Author

can you enlighten me why the sorting depends on text edits

@jrieken can correct me if I'm wrong, but I think this is because vscode is trying to sort based on 'relevance' (and mostly ignores the sortText). The relevance is determined by a matching algorithm that takes as input the characters that make up the 'query' versus the completion item. Basically, it wants to put the things that are a better match to what you typed at the front of the list. But, to do that, it uses the replacement region of its text edit to determine the 'query' you typed.

We had lengthy discussion about this in the past: #26096

@dbaeumer
Copy link
Member

I clarified this in the spec and made only documentation and detail lazy resolvable. If we think that other clients can handle more lazy attributes or want to change this I propose we introduce a capability that describes which properties can be lazy. However I first would wait for someone asking for this.

@dbaeumer
Copy link
Member

Closing. Changes happened in vscode-languageserver-node and language-server-protocol.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
suggest IntelliSense, Auto Complete under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants