feat(lsp): support using new language server implementations

jokeyrhyme commented 1 year ago

relates to #117
adds a new "implementation" setting: extension, nu --lsp, or lsp
- "extension" is the default, and uses the existing TypeScript->nu --ide-... implementation embedded in this extension
- nu --lsp will use this implementation (if available): https://github.com/nushell/nushell/pull/10723
- uses the "nushellExecutablePath" setting
- nuls will use the implementation in this repository (if available): https://github.com/jokeyrhyme/nuls
- language server selection is live, i.e. changing "implementation" causes the selected language server to start (and all others to stop)

jokeyrhyme commented 1 year ago

using this branch, I can confirm that this bug is in nuls and not helix, so I'm going to have to sit down and fix it: https://github.com/jokeyrhyme/nuls/issues/22

fdncred commented 1 year ago

Let's make this draft while it is still WIP.

fdncred commented 1 year ago

ok, I'm seeing a few things:

I figured out that you have to quit out and restart to use a different lsp.
It looks like hover help isn't rendered correctly.

I'm guessing this is some markdown formatting issues with backticks and such. I remember JT fighting this originally.

We also already know that goto-definition doesn't quite work right yet.
It looks like the include-path isn't being sent correctly, at least it looks different in the logging window. This is how those u{1e} separators are rendered like using the extension lsp
Hovers seem to be getting the right information.

It's generally a pretty good experience considering where it's at in the development cycle. Good job!

jokeyrhyme commented 1 year ago

@fdncred I assume those are notes on nuls and not this PR on the extension? Anything further to be done in this PR?

fdncred commented 1 year ago

Yes, those are notes on nuls when I tried it out in this PR.

I think there should be some type of restart on this PR when the lsp changes.

jokeyrhyme commented 1 year ago

Okay, I've made "implementation" a live setting

It's working suspiciously well, haha

fdncred commented 1 year ago

My test of this produced some errors. Not sure what any of this really means.

fdncred commented 1 year ago

Something weird is going on. If I delete the node_modules folders from the root, server, and client folders. Then do a npm install and start debugging with F5 in vscode. I get all these errors seen above in the vscode where i have the source code and these popups in vscode that was launched with F5.

fdncred commented 1 year ago

I will say though, the switching between nuls and extension is working well. We just need to narrow down on what's giving these other errors.

jokeyrhyme commented 1 year ago

The current behaviour tries to stop all the servers and then start the desired one, and maybe those errors are from trying to stop a server that was never running

Just a theory, but I'll play with it and see

Thanks for checking!

jokeyrhyme commented 1 year ago

I'm handling the Promises from client.start() and client.stop() more thoroughly to try to address those error messages

That said, I believe them to be pretty harmless, and you probably won't encounter them at all unless you try to use an implementation that cannot be started, i.e. you don't have nuls installed, or you don't have the nu --lsp branch installed

fdncred commented 1 year ago

Thanks! I'll give this a try later this afternoon. Just so you know, I had nuls and (obviously) extension installed when I got those errors. I did not have the PR with nu --lsp installed, but I never configured my system to use it. Hopefully we don't see these errors anymore so we can land this PR.

fdncred commented 1 year ago

Sorry - Ran out of time today. I hope to look at this tomorrow.

jokeyrhyme commented 1 year ago

No worries

Not urgent

And I've had less time to work in this area in the last week, too :)

fdncred commented 1 year ago

ok, finally got to this. This looks better but I'm not sure we can ship with this. People will be filing issues because they think something isn't working. I wonder if there's a better way to handle this like if whatever the configured lsp is, like extension, don't try and stop the configured one on startup. Just stop the other ones maybe?

There's also 3 language server log menu items now. Not sure what that's about. They all have similarly formatted information. Looks like they're all coming from the "extension" lsp. And they all update as you mouse over things.

jokeyrhyme commented 1 year ago

Okay, seeing as there's no real harm in having .stop() fail, we'll silently log these errors instead of displaying them

And I've given each LanguageClient its own unique identifier and label, so that the drop-down in the Output pane is less confusing

fdncred commented 1 year ago

I like what you did with the errors but it's still showing multiple Nushell Language Servers on startup for extension.

Once I switch to nuls I see two with different names, which is probably ok. Although I'm not sure myself why they couldn't all share the same log window.

If I switch back to extension in the config, i have the same two log windows for (extension) again.

jokeyrhyme commented 1 year ago

Hmmmm, I don't know why I can't reproduce any of this on my machine

It's been a pretty poor feedback loop so far

I've been able to verify that different implementations are powering IDE features as expected, but not everything else you've experienced

There are 3 separate LanguageClient instances, which is why there should be at least 3 separate log sections, but it's possible that restarting one creates a fresh entry, which isn't ideal

There might be a way to recycle a single instance between them all

Or perhaps we're in strange territory for Code, perhaps one extension wasn't supposed to connect to multiple language servers like this

fdncred commented 1 year ago

I really think the key is to remove your node_modules folders and do a npm install. once i did that, weirdness was allowed. 😆

Just to be clear, we'll only have 1 language server in the final build. So, figuring out how to make it work with 3 is not really a super high priority. Eventually we'll just delete all the code and just have one. Having said that, if it's something that can be figured out quickly, it would be nice to be able to continue to switch between all of them while we're still testing. If it's a massive pain, we can just live with it until we reduce to one.

fdncred commented 1 year ago

@schrieveslaach Now that we landed your PR https://github.com/nushell/nushell/pull/10723, I've started playing here with it to see how things are going. Good news is that since @jokeyrhyme already programmed this PR to make your lsp avaiable, I can see some things working like this hover.

Hover

But command hover's don't quite format right like this one

and this one

Goto Def

hard to show but when i tested it and it seems to be working great so far

Completions

I'm not sure how to get completions to work. What I was doing was typing str <ctrl-space>. Not sure the right voodoo to get them to work here.

All in all, it's a good start.

I can also see the process running, and when I quit vscode, the nu --lsp process is ended.

schrieveslaach commented 1 year ago

@fdncred, thanks for the feedback and great to see that it is also working in Vscode (I'm using Neovim).

I'm not sure how to get completions to work. What I was doing was typing str . Not sure the right voodoo to get them to work here.

I observed similar behavior in my editor of choice and that it seems to work only in certain conditions. Here is an example of my setup. I type something into the command line, start Nushell's open in editor feature, then my Neovim starts and start the LSP automatically, I can use some completions and then I quit the editor:

output

However, when the document changed completions stop working unless I was the document. And here the textDocument/didChange implementation is required. I'll start working on it in the next days.

schrieveslaach commented 1 year ago

@fdncred, speaking of “to show”: if you want I could provide some GIF recording of Nushell in Neovim if you want to include it in the release blog post.

fdncred commented 1 year ago

@fdncred, speaking of “to show”: if you want I could provide some GIF recording of Nushell in Neovim if you want to include it in the release blog post.

ya, definitely. Feel free to put them in this PR https://github.com/nushell/nushell.github.io/pull/1114 with a write up like "We've started building a nushell Language Server". What most people are looking for at this point is (1) how to configure it in helix, vim, nvim and (2) which features are currently supported.

amtoine commented 1 year ago

@schrieveslaach i know this is not really the place for this, but could you share your Neovim setup? i cannot get the completions to work :confused:

schrieveslaach commented 1 year ago

@schrieveslaach i know this is not really the place for this, but could you share your Neovim setup? i cannot get the completions to work 😕

@amtoine, at the moment it is very simple. I start an LSP for the current buffer when file type nu.

function StartNuLS()
lua << EOF
local id = vim.lsp.start({
   name = "Nushell's LSP",
   cmd = {
      "/pat/to/nushell-repo/target/debug/nu",
      "--lsp"
   },
   on_attach = M.on_attach
})
vim.lsp.buf_attach_client(0, id)
EOF
endfunction

augroup NuSetup
   autocmd!
   autocmd FileType nu call StartNuLS()
augroup end

I was thinking of providing a PR to lspconfig after Nushell's next release which does it then for you. Maybe I'll also try to add file type nu to Neovim directly.

amtoine commented 1 year ago

@schrieveslaach that would be great, i don't know how to plug that snippet into my config, i get errors all over the place :cry:

AucaCoyan commented 11 months ago

Just to get some directions, after this release of nu is done, and #11284 + 11320 are merged: what is missing to this PR?

I would like a list of things that --lsp must be capable of before landing it 🏁

fdncred commented 11 months ago

This PR only needs to support 2 lsps now. nu --lsp and nu --ide-* and not nuls. So, it needs to be refactored to only do those 2. You should also read these threads to see what I mentioned was missing or not working right before. Some of it has to do with logging.

jokeyrhyme commented 11 months ago

We probably want to determine what the destiny of this PR is

I agree we should remove the nuls code path

But is it desirable to ship/merge this PR otherwise (assuming other rough areas are polished)?

Or, should we keep this as a PR until nu --lsp has eclipsed nu --ide-..., at which point we can delete that code path from the PR, too?

fdncred commented 11 months ago

I'd like to have the functionality of switching between the two lsps until nu-lsp overtakes nu --ide- in functionality and stability. This PR isn't far from landing. We just need to work on the things mentioned, logging, making sure to default to nu --ide-, etc.

AucaCoyan commented 11 months ago

@fdncred Oh, my bad, sorry if I was disrespectful. I lost something in the translation 😅

My understanding is that the problems mentioned in this PR are solved in other PRs (either merged, or ready to merge when the moment comes)

markdown formatting for hover solved in 11253
go to definition solved in 10723
hover was reorganized to match the help command 11320

and, the remaining issues:

It looks like the include-path isn't being sent correctly, at least it looks different in the logging window.

I don't know how to reproduce the problem, can you give me a little script or sample to follow? 💯 . That is a note for nuls, is --lsp working properly?

red error-sy logs here

I had some of those errors too, but it was something of VS Code playing badly with the extension. I decided to try the extension host with all the extensions disabled and it didn't failure once. Still have that weird bug that sometimes the LSP has more than one instance running

And that's about it. I see nothing else to be missing. That's why I asked before for "what could be done to land this PR?" (in a poor manner) Again, sorry for the rudeness! 😇

schrieveslaach commented 11 months ago

I think input for @AucaCoyan last comment would be also input for me on what I could work next in the LSP area. I'm a bit lost there at the moment because I just using it to edit my Nushell REPL input and don't often work with a lot of scripts.

fdncred commented 11 months ago

Looking at this PR, there are a few things that might need to change before landing. I've tried to re-read everything and add a checkbox for it below just to make sure we've covered everything. Some of these items may be done and we can just check them off. Others will need some work prior to landing this PR.

- [x] I'd like it to support nu --ide-* and nu --lsp as the two options for LSPs.
- [ ] I don't want to have to quit out to get switching LSPs to work.
- [ ] When running from VSCode server (debug mode) it shouldn't have any errors.
- [ ] Test deleting node_modules and then npm install to see if odd errors in vscode happen.
- [ ] When switching LSPs, maybe it would be nice to show a popup that we switched.
- [ ] When switching LSPs, we shouldn't get "nushell language server error".
- [ ] We should have 1 "Nushell Language Server" log shared between LSPs, not multiple. As a fallback, I could also live with separate "Nushell Language Server (extension)" and "Nushell Language Server (nu-lsp)" logs.
- [ ] We should initially have pretty verbose logging so we can make sure things are launching and running correctly. So, start with logging most everything in typescript so that we can see what's happening in the log window as it happens.
- [ ] Make sure functionality works on each LSP if implemented.
  - Extension
  - - [ ] Hover built-ins
  - - [ ] Hover custom commands
  - - [ ] Hover variables
  - - [ ] Hover rendered correctly
  - - [ ] GoTo Def
  - - [ ] Completions
  - - [ ] Diagnostics (errors/red squigglies)
  - - [ ] Inlays
  - - [ ] Configuration is accepted when changed (including include paths)
  - nu --lsp ( we know some of these are not implemented yet. that won't stop this pr from landing)
  - - [ ] Hover built-ins
  - - [ ] Hover custom commands
  - - [ ] Hover variables
  - - [ ] Hover rendered correctly
  - - [ ] GoTo Def
  - - [ ] Completions
  - - [ ] Diagnostics (errors/red squigglies)
  - - [ ] Inlays
  - - [ ] Configuration is accepted when changed (including include paths)

AucaCoyan commented 11 months ago

Thank you very much for creating a list with everything! 🙏🏼

I'm wondering on how to tackle the list. I think we should resolve changing servers and debugging + errors first. But since this is a PR, only jokeyrhyme and maintainers of this repo can make changes. Is it ok to you to make PRs into your repo jokeyrhyme/vscode-nushell-lang jokeyrhyme? Another possible way is to close this pr and make a meta issue. Both ways are good for me 💻

fdncred commented 11 months ago

or @AucaCoyan we can close this PR and you can copy the code out of it and make a new PR with you being the owner. i'm not sure if there's a way to give you permissions to change this PR without going through jokeyrhyme's repo.

jokeyrhyme commented 11 months ago

I've invited everyone here to collaborate on my fork, but have no ill feelings if you wish to fork my fork into another repository :)

fdncred commented 11 months ago

Thanks @jokeyrhyme, we appreciate your help!

AucaCoyan commented 11 months ago

Thank you jokey! I tested it and updated main of your repo successfully 😄

We'll be seeing around each other here and in the fork 💪🏼

nushell / vscode-nushell-lang