-
-
Notifications
You must be signed in to change notification settings - Fork 222
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
Auto completion does not work in clojure-lsp only mode (no repl connection). #996
Comments
Thanks for the detailed report! @ericdallo see anything odd here? I see that the |
In that first response that is a lot of duplicates which I'm guessing is something to be fixed. |
Yep, it's a bug on clojure-lsp, probably a missing distinct or a better na filter, I can check that soon |
I'm not sure if that would fix the issue of completions not showing up in the editor, but maybe we can reassess that after the duplicate fix. |
Oh sorry, I missed the second example. |
We'll have to dig into it. |
According lsp spec, "data" field in completionItem
I can see two problems from coc.nvim lsp trace, same as from vscode calve.
|
The data field could be anything and we pass some data that we can use on resolveItem to resolve item documentation field, I don't find anything wrong here, we could improve data field content though |
@ericdallo, do you agree if the "data" field could be anything, which means it'd be opaque to lsp client. My reading of spec is that should be only used as some kind of lookup key on lsp server for "lazily loading expensive completionItem". |
Actually, the client just get the |
Then the problem is that I don't see completeItem/resolve call after completion response. Also, why another roundtrip to server when "data" field had everything? https://microsoft.github.io/language-server-protocol/specification#textDocument_completion
|
It's not a must call
It doesn't have the documentation field, which could be a huge field increasing the json size. |
textDocument/completion response does have "doc" field in "data" json as shown in trace log. Here is cut and paste one of them
|
I see, yet it's the raw data not pretty/ready to display on client (missing markdown), but maybe we could probably parse on |
Yes, that's what I meant. If lsp client wants to keep a lightweight completionItemList cache (you seems saying emacs one does) as that may be refreshed on every keystroke, it makes sense not loading the document/detail of the whole completeItem[]. Maybe putting a hash lookup key in data field only (do you know where "_hasheq": 0 coming from, is it a clojure-lsp thing?), then completeItem/resovle lazily request "huge" markdown document field on completionItem selection. This is what LSP spec 3.16 says. By default the request can only delay the computation of the detail and documentation properties. Basically, textDocument/complete response can looks like this (so that the completionItemList cache can be lightweight on lsp client).
completeItem/resolve request/response should look like these (only when a complete item is selected from UI dropdown menu).
|
@ericdallo, completion still doesn't work. Response looks alright. I'm not sure textDocument/documnetHighlight in the middle interfere the completion or not.
|
@fonghou the fix was made on clojure-lsp master, we still need to release a version ( probably tomorrow) and calva needs to bump to use that version of clojure-lsp |
Oh, just saw you are using the master fix, sorry. |
I'm not sure why it's not working. I'll look into this soon. |
I've been looking into this and found something, though still don't know the real issue. I downgraded to Calva version 2.0.139 which used an older version of clojure-lsp, and with that the completions work (show up in the editor). The response of that version looks like this:
The response of the latest clojure-lsp, that doesn't work (show in the editor) looks like this:
I'm not sure yet what's causing it to not show in the editor. |
Yeah, we have a more complete completion response now, showing documentation, args, and completion item kind still conforming the LSP spec, the response of both looks correct to me :/ |
I think this may be a bug in the vscode-langaugeclient. When I add middleware for the completion request/response and log the response, I see this in the debug console:
|
@ericdallo It appears that VS Code wants the alias to be included in the completion item labels. I hardcoded completion items returned to be the following: [
{
"label": "str/test-fn",
"detail": "test detail",
"data": "clojure.string/blank?"
},
{
"label": "str/capitalize",
"detail": "clojure.string",
"data": "clojure.string/capitalize"
},
{
"label": "str/ends-with?",
"detail": "clojure.string",
"data": "clojure.string/ends-with?"
}
] You can see this gives me completions in the editor now: I think if the alias (like If it works as it currently is with other clients, as I suspect it does, and you have a reason not to go back to including the alias in the label, then I wonder if there's something in our language definition / parsing stuff that we need to modify so that VS Code is looking only to match the text after |
Oh, good catch! I can take a look at this tomorrow |
Pinging @PEZ. Do you know of what might make VS Code match on the completions that don't include the namespace, like other editors seem to do? Is there something we can change in Calva that might do that? Seems like it sees I don't know which solution is better anyway, though. 🤷♂️ |
@bpringe we have completion on root too, to complete function/var/alias names, those don't have an alias on completion items, could you confirm that does work on Calva? |
@bpringe I made the change and it worked on emacs too, could you test it if fix it for Calva? |
@bpringe I notice that we don't get completions from unrepl either. We probably should deal with that the same way as we do with documentation and navigation lookup. |
Guessing you mean nrepl? But in any case, I'm not following. 🤔 I'll test that out @ericdallo. |
That works @ericdallo! 🎉 I also verified that nrepl returns completions with the alias as well, though I'm not sure if this is just because of how we're calling the I think this is good to leave as-is for now (with the merge for re-including aliases), but maybe later we can try to make Calva not require the alias in completions. I wonder if this is something to do with the wordPattern we have set, or something like that (so it wants to match on |
Nice, I'll release soon |
Released @bpringe ! |
@bpringe, @ericdallo Thank you for working on this. The fix also works for coc-nvim. Since it's a fork of vscode-languageclient, but no using calva code, I think this issue being fixed is not calva specific. Fyi, one inconsistency I noticed. "detail" field from unaliased items shows namespace not signature, but aliased ones contain signatures instead.
|
Nice @fonghou, will take a look, but I think we only include the arglists if its present on clj-kondo analysis, otherwise fallback to namespace |
Latest Calva version and latest clojure-lsp release 2021.01.28-03.03.16.
request and response from above screenshot. response looks strange same {"label": "log", "kind": 9} shows up multiple times.
request and response at the point of above screenshot. Response does contain useful info, but nothing displays in GUI completion menu. Not sure {"_hasheq": 0} in all data items could be the issue.
The text was updated successfully, but these errors were encountered: