Skip to content
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

Integrated Language Server Protocol #10794

Open
5 of 9 tasks
schrieveslaach opened this issue Oct 21, 2023 · 18 comments
Open
5 of 9 tasks

Integrated Language Server Protocol #10794

schrieveslaach opened this issue Oct 21, 2023 · 18 comments
Labels
enhancement New feature or request meta-issue An issue that tracks other issues

Comments

@schrieveslaach
Copy link
Contributor

schrieveslaach commented Oct 21, 2023

Related problem

As described in nushell/vscode-nushell-lang#117 there is a demand to integrate support for the language server protocol (LSP) into Nushell itself instead of creating an additional command line interface (CLI) protocol (that is the current approach with --ide-* CLI options.

Describe the solution you'd like

Based on the initial draft PR #10723 at least following LSP methods should be supported by Nushell:

Describe alternatives you've considered

The alternative would be to keep using the --ide-* in the long run and build an external LSP server.

Additional context and details

No response

@schrieveslaach schrieveslaach added enhancement New feature or request needs-triage An issue that hasn't had any proper look labels Oct 21, 2023
@fdncred
Copy link
Collaborator

fdncred commented Oct 21, 2023

Thanks @schrieveslaach.

The one other thing that I'd like to see added, which I think has to be added on the vscode side is semantic highlighting. Just logging it here to remember.

@amtoine
Copy link
Member

amtoine commented Oct 21, 2023

just to be sure, will #10723 close this issue? 😋

@fdncred
Copy link
Collaborator

fdncred commented Oct 21, 2023

just to be sure, will #10723 close this issue? 😋

Not necessarily. This is the tracking issue. We could land #10723 without everything being completed and then have follow-up PRs to complete the remaining tasks.

@sholderbach sholderbach added meta-issue An issue that tracks other issues and removed needs-triage An issue that hasn't had any proper look labels Oct 23, 2023
@schrieveslaach
Copy link
Contributor Author

@fdncred, textDocument/semanticTokens is now in the list. I put an asterisks at the end because there are multiple requests.

@fdncred
Copy link
Collaborator

fdncred commented Oct 24, 2023

@fdncred, textDocument/semanticTokens is now in the list. I put an asterisks at the end because there are multiple requests.

I'm excited about this!!!!

@fdncred
Copy link
Collaborator

fdncred commented Nov 2, 2023

@schrieveslaach When you get a moment, can you update the tracking items above?

@fdncred
Copy link
Collaborator

fdncred commented Nov 28, 2023

@schrieveslaach I just heard that the work you've done here has already been added to helix and to zed. Here's a screenshot of zed that was posted on discord.
image

@AucaCoyan
Copy link
Contributor

Hi! I did a lot of work on nufmt. I checked out the code for the LSP but sadly I understand very little. I do want to help! LSP give life to my everyday code. Is there any help I can give for you? (particulary for @schrieveslaach ) ❤️

@schrieveslaach
Copy link
Contributor Author

@AucaCoyan, thanks for offering help. In the recent weeks hadn't had much time for working on LSP in Nushell and I wasn't sure what to tackle next. Maybe, on workspace/configuration but I'm uncertain how a workspace should behave for a Nushell project.

But what do you think about integrating nufmt as a Document Formatting Request into Nushell? When Nushell's LSP receives the request, it could look for nufmt binary in the PATH variable and then it could format the content kept in memory and sends back the the text edits that the client has to do as well.

@AucaCoyan
Copy link
Contributor

Great!
Just for confimation: do you see a better approach to have a different binary called nufmt or do you prefer having a library in nu-lsp that formats code?
nufmt it's not close to be ready as a beta yet, it doesn't format a thing. Just checking if you want this or you prefer having everything in one repo for the lsp 👍🏼

@fdncred
Copy link
Collaborator

fdncred commented Dec 3, 2023

we should not integrate nufmt yet. it's not very useful in its current state.

fdncred pushed a commit that referenced this issue Mar 25, 2024
# Description

This commit fills in the completion item kind into the
`textDocument/completion` response so that LSP client can present more
information to the user.

It is an improvement in the context of #10794

# User-Facing Changes

Improved information display in editor's intelli-sense menu


![output](https://github.com/nushell/nushell/assets/16558417/991dc0a9-45d1-4718-8f22-29002d687b93)
@fdncred
Copy link
Collaborator

fdncred commented Mar 31, 2024

@schrieveslaach one thing I think we need to be able to have is the -I include dirs functionality. For vscode, I think that will be a configuration point, but it could also just utilize whatever is in your env.nu NU_LIB_DIRS folders as a starting point.

@schrieveslaach
Copy link
Contributor Author

@fdncred, I'm not so sure if I can follow you. The -I option is evaluated and set put into the engine_state before starting the LSP (in the same fashion as for the IDE command line options). If you are talking about this, I'm lost what is not yet working.

Or are you talking about the rootUri LSP init parameter? Here I'm unsure how a rootUri should be treated be nu's LSP.

@fdncred
Copy link
Collaborator

fdncred commented Apr 7, 2024

@schrieveslaach Sorry, this is what I'm talking about. It may not be exactly as I've stated but this is the symptom.

This is my config.nu loaded in nvim. Note that the lsp is failing to find some files.
image

The reason for this is

  1. I am not specifying any -I parameter to the nu lsp invocation. I'm kind of a nvim noob and I barely got the lsp working.
  2. I'm thinking that the lsp should probably look at NU_LIB_DIRS and apply that automatically. If this were to happen, then I wouldn't have any errors when looking at my config.nu.

In the vscode extension, I get around this by having a config point to manually add Include Dirs.
image

I'm not saying we should have a config or not. I'm not sure but I am saying that whatever is in NU_LIB_DIRS should be used. This may also assume that a persons env.nu is parsed when launching with nu --lsp which I'm not confident of.

@schrieveslaach
Copy link
Contributor Author

If you are testing with nvim-lspconfig changing the CLI parameters is easy:

local lspconfig = require('lspconfig')
lspconfig.nushell.setup({
   cmd = { "nu", "--lsp", "-I", "… your path …", "-I", "… another path" },
   -- … more options …
})

This may also assume that a persons env.nu is parsed when launching with nu --lsp which I'm not confident of.

It doesn't parse standard config files. Seems to me that it should in certain cases. Maybe, if there is no rootUri present. Let me check something.

@schrieveslaach
Copy link
Contributor Author

@fdncred, have a look at #12454 if that solves your issue.

@fdncred
Copy link
Collaborator

fdncred commented Apr 8, 2024

If you are testing with nvim-lspconfig changing the CLI parameters is easy:

This may work but I'm not confident with it because the -I parameter was never meant to be used multiple times when launching nushell. It's meant to be one string with separate folders separated by \x1e, iirc.

I'll test #12454 soon.

@schrieveslaach
Copy link
Contributor Author

Providing -I multiple time was just a guess. Following should do the trick then.

local lspconfig = require('lspconfig')
lspconfig.nushell.setup({
   cmd = { "nu", "--lsp", "-I", "… your path …\x1e… another path" },
   -- … more options …
})

fdncred pushed a commit that referenced this issue Apr 14, 2024
# Description

When starting the LSP server, the configuration file and environment
file are used to configure the LSP engine unless --no-config-file is
provided.

This PR provides an improvement that is related to #10794 

CC: @fdncred
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request meta-issue An issue that tracks other issues
Projects
None yet
Development

No branches or pull requests

5 participants