dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.12k stars 4.04k forks source link

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

Open zoriya opened 8 months ago

zoriya commented 8 months ago

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: https://github.com/neovim/nvim-lspconfig/issues/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.

CyrusNajmabadi commented 6 days ago

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

dibarbet commented 5 days ago

Support --stdio (which was the default before https://github.com/dotnet/roslyn/pull/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).

616b2f commented 4 days ago

Support --stdio (which was the default before https://github.com/dotnet/roslyn/pull/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.