Skip to content
This repository has been archived by the owner on Apr 1, 2020. It is now read-only.

[WIP] Support clangd as an LSP client #757

Merged
merged 6 commits into from
Oct 9, 2017

Conversation

TalAmuyal
Copy link
Collaborator

This is still a work in progress.

clangd works with the changes in this PR.
However, there are some issues:

Completion completes too much (probably clangd-s fault)

For instance, when writing namesp, the first completion option is namespace name.
The name part is a place holder for the name of the namespace.
The issue here, is that selecting that option for completion adds the word name.
I suspect that this is due to a misuse of the label LSP field on the server side.

Stdout used for debugging

clangd outputs a copy of the input and output as logs into stdout.
The format is more or less HTTP-HEADER <-- request-msg reply-msg -->.
As a temporary hack, a new flag is introduced that instructs which behavior should be used with the stderr of the specific LSP server (either log it as error or as debug).

Which brings me the the next issue: IDK TypeScript, so there is a run-time error with this flag.
It is always evaluated as undefined so I or'ed it with true until I figure out what's wrong (hints will be appreciated).

@bryphe
Copy link
Member

bryphe commented Oct 5, 2017

Wow, excellent! Thanks for testing out the clangd integration, and opening the PR. I was just working on some LSP stuff so it's helpful to know about the flag you needed to add to get it working.

For instance, when writing namesp, the first completion option is namespace name.

Interesting - would it be possible to see the the value we get for the completion? If you build One with npm run build-debug and put a breakpoint here:
https://github.com/bryphe/oni/blob/33c0350cab8a6661c6600d09bb8a2f36ba65137e/browser/src/Plugins/Api/LanguageClient/LanguageClient.ts#L278

It would be helpful to see the value of items in this case.

Which brings me the the next issue: IDK TypeScript, so there is a run-time error with this flag.

Hmm, the flag looks ok (and the code using it), but I'm wondering how it gets passed in? It looks like there may be a file missing (the lib\index.js that registers the language client). Could you please add and push that up? I can definitely help you troubleshoot.

const command = Oni.configuration.getValue("cpp.langServerCommand", "clangd")

const serverOptions = {
command,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the stdErrAsLog issue - it looks like it's picked up as part of these options, so you'll need to include it as part of this object:

const serverOptions = {
   command,
   stdErrAsLog: true,
}

I think that should address the issue you had where it wasn't getting set / defined in the language client code

@bryphe
Copy link
Member

bryphe commented Oct 5, 2017

Just added a comment on passing the stdErrAsLog variable through... let me know if it's still not working. Once that's set, I'll bring this in!

@TalAmuyal
Copy link
Collaborator Author

Re-based on master, now can't build Oni :(
#765

@TalAmuyal
Copy link
Collaborator Author

Since #765 is not always in the way, I could resume :D

The flag now works as expected, thanks!

Regarding the completion issue, I took a deeper look at it and the issue is on Oni's side.

The protocol states that label should be used unless insertText is present.

The first entry in the response that clangd sends is: {"label":"namespace name = namespace","sortText":"00040namespace","filterText":"namespace","insertText":"namespace"}

So I made a few changes and now it works.

Please review the changes

@bryphe
Copy link
Member

bryphe commented Oct 9, 2017

Looks great @TalAmuyal ! Thanks for debugging into the completion issue and fixing the root cause 👍

Tested it against another language provider (the omnisharp/C# one), and it worked great. Bringing this in now.

@bryphe bryphe merged commit 85f3cff into onivim:master Oct 9, 2017
@bryphe
Copy link
Member

bryphe commented Oct 9, 2017

Thanks for the contribution, @TalAmuyal ! I'll create a new release today including this change.

@TalAmuyal TalAmuyal mentioned this pull request Oct 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants