WIP - Get more detailed completions by default #167
Conversation
|
Pivoting here and making this not-default until performance is more acceptable |
8d3cb17 to
f0e08a5
Compare
| @@ -32,6 +34,10 @@ if (program.tsserverLogFile && !program.tsserverLogVerbosity) { | |||
| program.tsserverLogVerbosity = 'normal' | |||
| } | |||
|
|
|||
| if (!program.detailedCompletionsLimit) { | |||
| program.detailedCompletionsLimit = 500 | |||
There was a problem hiding this comment.
This is IMO risky as completions are not re-requested when you are narrowing your completion results. So if we exclude something initially, it will be unavailable even if the user narrows down his query.
Due to that, all those optimizations should be optional IMO. Better to be correct than shaving some milliseconds.
There was a problem hiding this comment.
It's not just a matter of shaving milliseconds, the LSP server in neovim has been having issues handling large payloads, leading to freezes and OOM issues; see this issue.
But I agree that arbitrarily limiting results is risky, people need to trust that they are seeing all available completion items.
Maybe a good middle ground would be to visually indicate to the user that the list has been truncated? ie. adding a <RESULTS TRUNCATED> item to the completion list; I know this sucks but I'm not sure there is a better way.
There was a problem hiding this comment.
For servers that are likely to return a lot of completions it's OK to limit but then also use the isIncomplete flag so that completions are re-requested on query change.
So that could be the solution here too - add a flag that enables the use of isIncomplete option and limiting the results. But that would likely require more effort to implement.
There was a problem hiding this comment.
Good points and thanks for the feedback!
To be clear, this limit doesn't change the total number of candidates completed: it only makes those candidates less detailed. I'm finding it's needed (and I probably need to make the default smaller) because of a compounding issue with as-you-type completions. Sometimes a completion that is more broad (ie user types co) will return after a more specific query (ie user continues typing console.).
I need to do a little more research into how this is handled on other servers. I know our cancellation story is pretty rough right now, but I don't know that it would handle this specific situation.
There was a problem hiding this comment.
I can't see how we can come up with a good solution for this issue. Even if we would use the isIncomplete flag and limit number of completions, that could result in client missing some completion that he wanted to use. It would likely come on typing another characters but for some (maybe all) editors the completion popup might already be dismissed since there were no matching completions.
Also, with the isIncomplete flag we would trade the initial heavy completions with a need to request completions on every typed character which would make things worse for editors that currently don't struggle with large number of completions.
So IMO this PR should be closed.
Resolves #160
Currently debugging why we require limiting the number of detailed completions. By limiting the number of detailed completions, we're somehow speeding up completion on even relatively small completion candidates. This lines up reports that Neovim is getting spammed with tons of completion responses that didn't seem necessary by @pyrho
Finally, I need to add a command line flag that disables detailed completions