Add partial completion and rename functionality in LSP/VSCode #1143
Conversation
thpani
force-pushed
the
th/vsc-complete
branch
2 times, most recently
from
db2498a to
77462be
Compare
| const declIds = findDeclByNameAtPos(identifier, position, sourceFile, parsedData) | ||
|
|
||
| // FIXME: use analysisOutputByDocument | ||
| const [_, aop] = analyzeModules(table, modules) |
There was a problem hiding this comment.
Have you tried using analysisOutputByDocument? I'm not sure exactly what the consequences of the delay might be, but we might be fine with it.
We might also be fine with running the analysis when users type .. The reason for the delay is that we can't run it every time a user types anything, since that becomes laggy pretty quickly.
There was a problem hiding this comment.
We could try to trigger the async job here and avoid running it twice, but I'm not sure it's worth the trouble. The real improvement will be with incremental parsing.
There was a problem hiding this comment.
Have you tried using analysisOutputByDocument? I'm not sure exactly what the consequences of the delay might be, but we might be fine with it.
Yeah, I tried that, but in 99% of cases the analysis output and the parsed data are out of sync by then (i.e., using disjoint sets of IDs).
I guess the delay is fine for hover, because an analysis will be triggered until you move your mouse, but for triggering completion, the LSP request wins the race against the timeout.
There was a problem hiding this comment.
Maybe we should make analysisOutputByDocument a Promise? Then I could wait for that.
There was a problem hiding this comment.
Maybe we should make
analysisOutputByDocumenta Promise? Then I could wait for that.
I'm not sure if that's a good idea because everything else is fine with outdated information - it's better to show the latest information than make the user wait, I think.
Perhaps we could have another version of analysisOutputByDocument that is a promise and use it only for this scenario.
There was a problem hiding this comment.
I tried to combine this with a promise, but I couldn't find a way to use the same analysis action both with a timeout and as a promise.
I'm off next week, maybe you have an idea here, otherwise I'll take another stab at it when I'm back!
There was a problem hiding this comment.
I gave this a shot and I think I got something pretty OK, please take a look and see if you agree: ea330e0
There was a problem hiding this comment.
This looks great! Thanks for taking a shot at it! 🙏
Co-authored-by: Gabriela Moreira <[email protected]>
Output of my 🥧 week:
Add partial support for the completion and rename LSP endpoints in the VSCode plugin.
The logic for completion is a bit funky, because in most cases, when the endpoint is called, we don't have parsable source code. Hopefully we can alleviate this in the future by having an incremental parser.
The other thing I noted was that analysis as currently delayed by some timeout after parsing. While this works for hovering, it gives me mismatching IDs in the parser result and the analysis output when completion is called.
Thus, I'm currently calling analysis synchronously in
completeIdentifier– I'm sure you have suggestions how to improve this :)CHANGELOG.mdfor any new functionalityREADME.mdupdated for any listed functionality