search

neoclide / coc-tsserver

Tsserver extension for coc.nvim that provide rich features like VSCode for javascript & typescript
MIT License
1.05k stars 68 forks source link

Resolves insertText of completion against spec #305

Closed rchl closed 3 years ago

rchl commented 3 years ago

I've been investigating the code related to the javascript.suggest.completeFunctionCalls setting and noticed that it updates the insertText property of the completion from the completionItem/resolve request.

Here is the relevant code: https://github.com/neoclide/coc-tsserver/blob/66ae279b1a3441ad5ca77d47934f99f039cd1e0b/src/server/features/completionItemProvider.ts#L376-L391

The LSP spec has this note regarding the completionItem/resolve handling:

All other properties (usually sortText, filterText, insertText and textEdit) must be provided in the textDocument/completion response and must not be changed during resolve.

https://microsoft.github.io/language-server-protocol/specification#textDocument_completion

I'm just wondering what is your take on this because I was looking into implementing this within typescript-language-server but according to the spec this shouldn't work (and it likely won't in ST, for example).

chemzqm commented 3 years ago

It's same behavior as typescript-language-features https://github.com/microsoft/vscode/blob/main/extensions/typescript-language-features/src/languageFeatures/completions.ts#L223, because SymbolDisplayPart only exists with CompletionDetailsResponse.

From LSP specfication:

This is done using the completionItem#resolveSupport client capability which lists all properties that can be filled in during a ‘completionItem/resolve’ request

you can add textEdit field to completionItem#resolveSupport client capability and avoid insertText which is deprecated.

rchl commented 3 years ago

It's same behavior as typescript-language-features https://github.com/microsoft/vscode/blob/main/extensions/typescript-language-features/src/languageFeatures/completions.ts#L223, because SymbolDisplayPart only exists with CompletionDetailsResponse.

Yes, I understand it's mostly the same code that VSCode uses but I'm talking from the perspective of the LSP spec which VScode doesn't have to follow since they made their implementation not use LSP in the first place.

From LSP specfication:

This is done using the completionItem#resolveSupport client capability which lists all properties that can be filled in during a ‘completionItem/resolve’ request

you can add textEdit field to completionItem#resolveSupport client capability and avoid insertText which is deprecated.

Hmm, now that I'm re-reading my quoted sentence, I guess it doesn't disallow having insertText (or textEdit) in resolveSupport. It just gives example on what would normally not be in resolveSupport, but can be. It's not very clear, I would say.

chemzqm commented 3 years ago

Yes, it says usually, not disallow.