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

Specify input text in completion items #648

Open
sumneko opened this issue Dec 28, 2018 · 29 comments
Open

Specify input text in completion items #648

sumneko opened this issue Dec 28, 2018 · 29 comments
Labels
clarification completion feature-request Request for new features or functionality
Milestone

Comments

@sumneko
Copy link

sumneko commented Dec 28, 2018

image

As you see, client thinks input text is co, but server knows the really input text is core.co.
So I hope the server can specify input text in the response of Completion Request.

@puremourning
Copy link

+1

In ycmd's protocol, the server tells the client the column at which completion should start (as Vim and can't support arbitrary text edits for completion). For the YCM LSP implementation, our LSP client has to try and guess, as the behaviour of overlapping text is pretty much unspecified in the protocol:

The insertText is subject to interpretation by the client side.
* Some tools might not take the string literally. For example
* VS Code when code complete is requested in this example con<cursor position>
* and a completion item with an insertText of console is provided it
* will only insert sole.

@rcjsuen
Copy link
Contributor

rcjsuen commented Dec 28, 2018

Interesting. Just realized this is actually noticeable in the example given for microsoft/vscode-docker#277 although not really related to the fix itself.

@joaotavora
Copy link

Just realized this is closely related to #651. In that issue I provide an alternative way to fix this, where the server says what characters of the completion it is actually matching.

@mickaelistria
Copy link

I don't think it's a protocol issue; as it seems to me that for other clients (such as Eclipse IDE for instance) core.co would be highlighted as match. The client already has everything necessary to improve this, and I don't think the protocol is to blame here, it seems more like a plain bug in VSCode that should better detect the string overlap between proposed edit and current code.

@KamasamaK
Copy link
Contributor

KamasamaK commented Jan 5, 2019

You can specify a textEdit in completion items, and its range should be be sufficient to determine "input text". This can be necessary when including word boundaries in completion items. I too do not believe this is an issue with the protocol.

@joaotavora
Copy link

joaotavora commented Jan 6, 2019

@KamasamaK, @mickaelistria how would the server communicate that the string it is matching is not contiguous? Say if the user has typed co.let to match core.completion. Is that possible with text edits in completion responses too?

@mickaelistria
Copy link

Is that possible with text edits in completion responses too?

At least Eclipse IDE support for LSP is able to do it without any protocol extension. Try Eclipse Wild Web Developer and open a .json file which has a json schema containing something like last.name and you'll see la.na shows last.name as proposal and highlight la.na in the proposal.

@joaotavora
Copy link

Try Eclipse Wild Web Developer and open a .json file

I'm unable to do that now. Can you perhaps post a small gif recording of that interaction here? I am also interested in the traffic of LSP messages that enable this, if you can capture them. Eclipse JDT server is known for doing some out-of-spec things. But if it's within the current spec, I'd love to know how.

@mickaelistria
Copy link

I'm unable to do that now. Can you perhaps post a small gif recording of that interaction here?

I don't think a git would be more descriptive than "it just works" here.

I am also interested in the traffic of LSP messages that enable this, if you can capture them.

it's a completion request/completion response, just like VSCode or any LSP client. The Language Server that's used is the JSon language server from VSCode.

Eclipse JDT server is known for doing some out-of-spec things

Eclipse JDT is not involved here, it's only the client part that implements substring detection better than VSCode apparently; but again, only relying on things that are already available.

But if it's within the current spec, I'd love to know how.

https://git.eclipse.org/c/lsp4e/lsp4e.git/tree/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSIncompleteCompletionProposal.java#n209

@joaotavora
Copy link

I don't think a git would be more descriptive than "it just works" here.

You suggested I install in my system a large piece of software with lots of depedencies, isn't it simpler just to record a simple gif so we can ensure we're talking about the same thing.

But OK, if that's too hard, here's a GIF from an IDE that does what I'm looking for:

image

Can you say that your Eclipse LSP server supports this kind of completion?

it's a completion request/completion response, just like VSCode or any LSP client. The Language Server that's used is the JSon language server from VSCode.

Can you guarantee that that server is not using any out-of-spec fields?

https://git.eclipse.org/c/lsp4e/lsp4e.git/tree/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSIncompleteCompletionProposal.java#n209

Don't make me go read your Java code (I'm not telling you to go read Emacs-lisp either), there are many abstractions and data types outside that function that I need to understand before I can make sense out of the function. LSP is language-agnostic: if you're arguing LSP is sufficient here please show the LSP traffic for your last.name example.

@joaotavora
Copy link

please show the LSP traffic for your last.name example.

Or if you prefer, just explain in plain english the relevant parts of the LSP requests and responses that support this.

@sumneko
Copy link
Author

sumneko commented Jan 7, 2019

You can specify a textEdit in completion items, and its range should be be sufficient to determine "input text". This can be necessary when including word boundaries in completion items. I too do not believe this is an issue with the protocol.

Thank you, it works.

@kdvolder
Copy link

kdvolder commented Jan 7, 2019

You can specify a textEdit in completion items, and its range should be be sufficient to determine

This is just a 'happy' coincidence but there really isn't any fundamental reason why this should be the input text.

Say I type 'foo' and the completion is 'foobar'. It could be represented equivalently by a server as

  • "insert 'bar' after 'foo'.
  • "replace 'foo' with 'foobar'.

Both these edits will result in the same end result. However the range in the first completion is empty (so input text is empty) and the range in the second one is 'foo'.

So just to add my '+1' and explanation why I think it is best to have the server explitly indicate the 'input text'.

In my opinion the arguments on not doing / needing this information explicitly basically boil down saying that the client can 'guess' the input text from available information. (E.g. the range if the main edit, as vscode does or some fancy text matching as eclipse does).

And it is true this works fine in most cases, however

  1. it remains a heuristic. The client is only guessing what part of the document the server took to be as 'input text' and it is certainly possible the client's heuristics gets it wrong.

  2. the heuristics are or can be non trivial and so are a burden to implement

  3. the heuristics are not spec-ed and so not consistently implemented the same on different clients.

Re point 1), besides the 'foobar' example above, I have another real example of this. Our language server intenionally included some spaces in front of the input text in the main edit range. The purpose of this was to 'de-dent' a proposal (by deleting some spaces in front of the input text). However the spaces really are not part of the 'input text' (just stuff in front of the input text that we want to delete). These proposals threw off vscode's 'guess' of the input text (it included the spaces) which in turn messed up sorting of proposals.

See: microsoft/vscode#26096 for details.

Vscode resolved this issue by adding some special case rules around (not) including spaces at the start of the input text.

So to summarise: My point is, sure... you can 'guess' the input text correctly in most/many cases. But why guess at all? When it would typically be very easy for a server to explicitly indicate what section of the document it actually used as 'input text'.

@dbaeumer
Copy link
Member

I do agree that this needs to be clarified. However assuming that we find a solution for the unintended problem I think specifying this via the range of a text edit or via an additional property has the same expressiveness.

@dbaeumer dbaeumer added discussion feature-request Request for new features or functionality clarification labels Jan 21, 2019
@dbaeumer dbaeumer added this to the On Deck milestone Jan 21, 2019
@kdvolder
Copy link

kdvolder commented Jan 21, 2019

I think specifying this via the range of a text edit or via an additional property has the same expressiveness

Only if you assume the range of the text edit matches the input text. This is not guaranteed and you cannot always make it so. This is because the 'text edit range' and the 'input text' are different things that have different meaning.

The range of the text edit is the text you want to replace, whereas the 'input text' is the text you use for filtering matches. Now, yes, it is true these are usually the same, but that is really just a coincidence. It all depends on what effect you want to have when a completion is applied, whether you can make the edit range be equal to the text you range you used for filtering completions.

@astoff
Copy link

astoff commented Jan 23, 2020

Notice another consequence of this issue: If the user wants to complete

require 'core.fo|'

(with the cursor indicated by |) and the server returns core.foobar as a candidate, the editor will filter it out, since the presumed input text, fo, doesn't match core.foobar.

(This isn't the only issue that arises from client-side filtering, as I tried to explain in #898)

@joaotavora
Copy link

@astoff don't you have a typo there? Or am I missing something? Also, where is the user's cursor in that moment?

@astoff
Copy link

astoff commented Jan 23, 2020

@joaotavora Should be correct now :-)

@kdvolder
Copy link

In general I am in agreement, however I'm not sure this example @astoff is a good one (or perhaps I am misunderstanding it).

the presumed input text, fo, doesn't match core.foobar.

Hmm... but fo does match with core.foobar since fo is a substring of 'core.foobar'. At least in vscode, I think that counts as a match.

@dgutov
Copy link

dgutov commented Jan 23, 2020

@kdvolder Suppose it uses fuzzy matching, OK.

When the editor inserts the completion, the text will become core.core.foobar.

@kdvolder
Copy link

When the editor inserts the completion, the text will become core.core.foobar.

Not if you specify the edit range correctly to indicate that the completion should replace core.fo -> core.foobar.

So, while in general I am really a proponent of making the 'input text range' explicit and separate from the completion's 'edit range'... it is actually quite hard to find good compelling examples for this.

@astoff
Copy link

astoff commented Jan 23, 2020

but fo does match with core.foobar since fo is a substring of 'core.foobar'. At least in vscode

Fine. Is there any genuine reason to do this kind of substring matching, or is it just a workaround for cases like the above?

If it's a workaround, then it has a simple solution: provide the completion prefix/bounds/subject in the response.

If it is a genuine feature, that's OK. But other editors may have a different idea. For instance, if you allow substring matches, sorting becomes more complicated (what sorts higher as a completion of foo: afoo or foobar?)

When the editor inserts the completion, the text will become core.core.foobar.

This means that, in the current state of affairs, the textEdit field of a completion item is essentially mandatory to ensure correct operation.

it is actually quite hard to find good compelling examples for this

If the server could convey to the client that no filtering should happen, then I agree that the only reason to give the completion subject is cosmetic — so the editor can place a popup box right below the completion subject, highlight the match etc. If there is client-side filtering, the prefix information is essential.

@kdvolder
Copy link

kdvolder commented Jan 23, 2020

Fine. Is there any genuine reason to do this kind of substring matching

I don't know about you but I like the fuzzy matches. I can type some memorable part of the string and not bother typing the sometimes very long common prefix that is shared by a lot of other things in the same package/namespace.

This means that, in the current state of affairs, the textEdit field of a completion item is essentially mandatory to ensure correct operation.

Agreed. And our servers always include them precisely because just using insertText alone just seems too ambiguous to me as to what that means (both in terms of the 'effect' that happens when the completion is invoked, as well as it's 'filtering' semantics).

So in that sense, I should change my statement that its not a good example. I think now... it is a good example... if we explicitly assume the completion uses simple 'insertText' rather than a textEdit with a range.

@dgutov
Copy link

dgutov commented Jan 23, 2020

I think fuzzy matching is good, but we can't rely on it (there is a certain fraction of users who avoid it).

@astoff
Copy link

astoff commented Mar 4, 2020

So, while in general I am really a proponent of making the 'input text range' explicit and separate from the completion's 'edit range'... it is actually quite hard to find good compelling examples for this.

@kdvolder I think this #898 (comment) is a good compelling example. Do you agree?

In short: foo.ba is getting completed to foo["bar bar"]. The "input text" does not include the dot, but the "edit range" does

@kdvolder
Copy link

kdvolder commented Mar 5, 2020

@astoff Yes I agree. It is in some ways also quite similar to our own examples which I have presented a long while ago (sorry no link... its burried somewhere in the long threads of multiple similar issues raised around this, so its kind of hard to find it now). But in short our example is for a yaml editor and it is similar to the 'delete the .' example... in that it ours wants to delete some spaces in front of the 'input text' to make sure the completion is inserted with a correct indentation.

I think in both cases what we have is a desire to adjust some 'punctuation' characters near the 'input text', to ensure 'correct' syntactic structure after the expansion. But these characters are not considered as part of the 'input text'.

@XeroOl
Copy link

XeroOl commented May 30, 2024

I want to let y'all know that I'm encountering this issue in the real world, working on fennel-ls. Emacs+Eglot decides the completion range differently than other editors, which causes them to filter out all the completion candidates.

The textEdit workaround doesn't seem to fix the problem (Eglot understands the edit, but it still filters out the completions because it doesn't think they match), and we can't adjust Emacs' fennel-mode to agree with fennel-ls, because changing fennel-mode also affects its non-LSP code indentation behavior.

I'm handling it right now by detecting eglot and providing different completions for it, but the promise of LSP is to avoid client-specific behavior.

@joaotavora
Copy link

@XeroOl

;; Eglot does completions differently than every other client I've seen so far, in that it considers foo.bar to be one "symbol".

FWIW foo.bar being one or two symbols is something in control of fennel-mode probably, so you should direct your complain there, tell them about syntax tables.

OTOH it's true that LSP simply doesn't have a way to tell what the thing at point is.

@XeroOl
Copy link

XeroOl commented May 30, 2024

@joaotavora That's the problem. There's nothing to complain about in fennel-mode, because foo.bar is technically one symbol in Fennel (symbols can have . in them in this language).
Fennel-mode & Eglot & Emacs are all working as intended, which is why I think the problem is with the protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification completion feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests