feat(lsp): support using new language server implementations

[jokeyrhyme]

• relates to Standalone LSP Server in Rust #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): Integrated Language Server nushell#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]

• 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: go-to-definition sometimes lands on the wrong line (but correct file, at least) jokeyrhyme/nuls#22

[fdncred]

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

[jokeyrhyme]

Bah, this is the only deliberate change here

The rest is auto-prettier, which I can revert if necessary

[fdncred]

If you're using vscode to edit these files, one thing that I've just learned is you can do ctrl/cmd-shift-p and choose File:Save without formatting and it stops things like prettier or other formatters from running on save.

[fdncred]

ok, I'm seeing a few things:

1. I figured out that you have to quit out and restart to use a different lsp.

2. 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.

3. We also already know that goto-definition doesn't quite work right yet.

4. 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
   

5. 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]

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

[fdncred]

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]

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

It's working suspiciously well, haha

[fdncred]

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

[fdncred]

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]

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]

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]

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]

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]

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

[jokeyrhyme]

No worries

Not urgent

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

[fdncred]

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.

[schrieveslaach]

@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:

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]

@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]

@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 nushell/nushell.github.io#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]

@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 😕

[schrieveslaach]

@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]

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

[AucaCoyan]

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]

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]

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]

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]

@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]

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]

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.

1. • I'd like it to support nu --ide-* and nu --lsp as the two options for LSPs.
2. • I don't want to have to quit out to get switching LSPs to work.
3. • When running from VSCode server (debug mode) it shouldn't have any errors.
4. • Test deleting node_modules and then npm install to see if odd errors in vscode happen.
5. • When switching LSPs, maybe it would be nice to show a popup that we switched.
6. • When switching LSPs, we shouldn't get "nushell language server error".
7. • 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.
8. • 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.
9. • 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]

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]

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]

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]

Thanks @jokeyrhyme, we appreciate your help!

[AucaCoyan]

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 💪🏼