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

roslyn-lsp --stdio option (or --pipe with a client-created pipe) #72871

Closed
zoriya opened this issue Apr 4, 2024 · 11 comments · Fixed by #76351
Closed

roslyn-lsp --stdio option (or --pipe with a client-created pipe) #72871

zoriya opened this issue Apr 4, 2024 · 11 comments · Fixed by #76351
Assignees
Labels
Area-IDE Feature Request help wanted The issue is "up for grabs" - add a comment if you are interested in working on it LSP issues related to the roslyn language server protocol implementation
Milestone

Comments

@zoriya
Copy link

zoriya commented Apr 4, 2024

Summary

Using roslyn-ls with an editor that is not vscode is hard because roslyn-ls creates its own pipe to communicate. I would expect communication channel to be defined by the client, with cmd line args like recommended on the spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#implementationConsiderations (having --stdio or --pipe /tmp/roslyn.sock).

Background and Motivation

I'm trying to add roslyn-ls to neovim's builtin lsp client, you can find the whole discussion there: neovim/nvim-lspconfig#2657

Proposed Feature

Support --stdio (which was the default before #69918, it seems changing fixed a bug, but I don't understand how)
Or support --pipe <pipe-path> to allow the client to specify its own pipe before creating the server.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 4, 2024
@sharwell sharwell added the Need Design Review The end user experience design needs to be reviewed and approved. label Apr 5, 2024
@sharwell sharwell removed the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 5, 2024
@sharwell sharwell added this to the Backlog milestone Apr 5, 2024
@sharwell sharwell added the LSP issues related to the roslyn language server protocol implementation label Apr 5, 2024
@CyrusNajmabadi CyrusNajmabadi removed the Need Design Review The end user experience design needs to be reviewed and approved. label Oct 26, 2024
@CyrusNajmabadi CyrusNajmabadi moved this from In Queue to Complete in IDE: Design review Oct 26, 2024
@CyrusNajmabadi
Copy link
Member

@dibarbet Can you either assign to a milestone, or close out as not planned if we are sticking with the current approach.

@CyrusNajmabadi CyrusNajmabadi added the Need More Info The issue needs more information to proceed. label Nov 26, 2024
@dibarbet
Copy link
Member

dibarbet commented Nov 27, 2024

Support --stdio (which was the default before #69918, it seems changing fixed a bug, but I don't understand how)

3rd party analyzers / code actions / etc sometimes write to stdout and corrupt the output. However I understand that sometimes a simple stdout option would be useful. I would accept a contribution here to add an --stdio flag which it could use instead of a named pipe if a client prefers that.

or --pipe /tmp/roslyn.sock)

We were not entirely convinced that nodejs' implementation of creating a named pipe on Windows passed the correct security flags (only readable/writable to current user, max number of connections, etc). We found it more expedient and verifiable to create the named pipe on the .NET side and send it back to the client to connect to.

I don't think we would want to add support for a --pipe flag as we already support named pipes (with a different mechanism of getting the name).

@dotnet-policy-service dotnet-policy-service bot added untriaged Issues and PRs which have not yet been triaged by a lead and removed Need More Info The issue needs more information to proceed. labels Nov 27, 2024
@dibarbet dibarbet added the help wanted The issue is "up for grabs" - add a comment if you are interested in working on it label Nov 27, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 27, 2024
@616b2f
Copy link
Contributor

616b2f commented Nov 27, 2024

Support --stdio (which was the default before #69918, it seems changing fixed a bug, but I don't understand how)

3rd party analyzers / code actions / etc sometimes write to stdout and corrupt the output. However I understand that sometimes a simple stdout option would be useful. I would accept a contribution here to add an --stdio flag which it could use instead of a named pipe if a client prefers that.

This sounds reasonable to me.

I don't think we would want to add support for a --pipe flag as we already support named pipes (with a different mechanism of getting the name).

Can we reconsider this? Because actually the upcoming LSP spec 3.18 is pretty clear about how the server should implement that:

pipe: use pipes (Windows) or socket files (Linux, Mac) as the communication channel. The pipe / socket file name is passed as the next arg or with --pipe=.

See: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.18/specification/#implementationConsiderations

While I understand the reason you described, I don't think all clients have to be forced into an off spec implementation to circumvent the shortcomings of one specific client platform.

Having the pipe parameter optional could satisfy both requirements with minimal effort.

What do you think? If you are okay with that I could create a PR with the changes.

@dibarbet
Copy link
Member

dibarbet commented Dec 5, 2024

Had some discussions with others on the team - we'd be OK with accepting an optional --pipe option as well as it is defined in the spec.

@616b2f
Copy link
Contributor

616b2f commented Dec 6, 2024

@dibarbet this are great news, thank you!

I would like to take this one and will prepare a PR in the next few days.

@captain0xff
Copy link

Do you also have any plans to add the --stdio option? A lot of editors' lsp clients don't support named pipes yet and also the spec recommends having a --stdio option so can we have both the --pipe and --stdio option?

@CyrusNajmabadi
Copy link
Member

@captain0xff We accept PRs :)

@captain0xff
Copy link

Sure, I was just confirming if a PR with the --stdio option will be accepted after the --pipe option is added as this issue only requests for one of them.

@CyrusNajmabadi
Copy link
Member

Yup. See #72871 (comment)

@JoeRobich
Copy link
Member

3rd party analyzers / code actions / etc sometimes write to stdout and corrupt the output.

One thought is that we would use the stream returned from Console.OpenStandardOutput() for messaging and reassign Console.Out using Console.SetOut() to redirect Console.WriteLine() calls to a different stream.

@seblj
Copy link

seblj commented Dec 14, 2024

I added a new issue about stdio support: #76436 to more easily track potential progress on this :)

I am considering looking into this myself, but if someone else beats me to it, then it would be great if that issue could be used to communicate about it :)

PS: sorry if I didn't create the issue correctly, but I didn't know which category it belonged to, so I just opened an empty one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Feature Request help wanted The issue is "up for grabs" - add a comment if you are interested in working on it LSP issues related to the roslyn language server protocol implementation
Projects
Status: Complete
Development

Successfully merging a pull request may close this issue.

8 participants