Add Support for Roslyn LSP (Csharp Language Server)

[Digni]

This adds support for the (newer) Roslyn LSP from Microsoft (Microsoft.CodeAnalysis.LanguageServer).

The current Implementation is using a wrapper provided by: https://github.com/SofusA/csharp-language-server.
This is because the Roslyn LSP expects one of the following notifications after "initialize":

{
  "method": "solution/open",
  "params": {
    "solution" : "filepathUri_to_solution" (.sln, .slnx)
  }
}

{
  "method": "project/open",
  "params": {
    "projects" : [ "filePathUri_to_Project" ,  ....] (list of .csproj files)
  }
}

The csharp-language-server reacts on the initialize notification to get the working directory and then scans it for solution and project files.
If it finds a solution file it picks the first one it finds, this could lead to some unintended reference issues if there are more than one solution file in the working directory.
The VsCode Extension handles this by asking the user which solution should be opened.

I would like to implement something similar and run the roslyn lsp directly (dotnet Microsoft.CodeAnalysis.LanguageServer.dll).
But for that to work I need to be able to send these notifications to the lsp server (and if possible ask the user which solution he wants to open) and I currently don't know how this would be possible with the extension api.

Maybe someone could help me with that.

Update

Added Workspace Configuration Options for Roslyn lsp.
Server expects the following format:

{language}|{grouping}.name for language specific options or {grouping}.{name} for general options.

Example

"csharp|inlay_hints.csharp_enable_inlay_hints_for_implicit_object_creation": true,
"csharp|inlay_hints.csharp_enable_inlay_hints_for_implicit_variable_types": true,

In the settings json I made it nested:

  "lsp": {
    "roslyn": {
      "settings": {
        "csharp|inlay_hints": {
          "csharp_enable_inlay_hints_for_implicit_object_creation": true,
          "csharp_enable_inlay_hints_for_implicit_variable_types": true,
          "csharp_enable_inlay_hints_for_lambda_parameter_types": true,
          "csharp_enable_inlay_hints_for_types": true,
          "dotnet_enable_inlay_hints_for_indexer_parameters": true,
          "dotnet_enable_inlay_hints_for_literal_parameters": true,
          "dotnet_enable_inlay_hints_for_object_creation_parameters": true,
          "dotnet_enable_inlay_hints_for_other_parameters": true,
          "dotnet_enable_inlay_hints_for_parameters": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_differ_only_by_suffix": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_match_argument_name": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_match_method_intent": true
        }
      }
    }
  }

This will get translated to the valid format described above.
Which leads to something like this (configured inlay_hints):

[Vlaaaaaaad]

CC @fminkowski, @dibarbet, @CyrusNajmabadi, and @maxbrunsfeld since they were involved in the initial implementation of the Zed C# extension and thus might or might not have thoughts

[Digni]

Seems like I made that PR to early... I did try Completion, Code Action etc.
But I missed a really crucial part.
The diagnostics are not working, even with that hack of @SofusA. (Force Diagnostics Pull)
I did try the hack that is used in nvim:

    capabilities = {
        textDocument = {
            -- HACK: Doesn't show any diagnostics if we do not set this to true
            diagnostic = {
                dynamicRegistration = true,
            },
        },
    },

Via intercepting the notification and adding it, but that did not work.

So I think here we need the support from the Zed Team.

[SofusA]

I also get no diagnostics. A simple way to reproduce is overriding the path for omnisharp:

"lsp": {
    "omnisharp": {
      "binary": {
        "path": "csharp-language-server"
      }
    }
  }

Do you know if zed supports pull diagnostics?

[Digni]

I do not, but my guess at the moment seems no.

Just tried dotrush to see if the diagnostics are working with that, but its the same. No Diagnostics

[SofusA]

ruby configuration states that pull diagnostics are not supported, under setting up ruby-lsp

[Digni]

You are right, here is the issue tracking that: zed-industries/zed#13107
Roslyn switched to only pull diagnostics some time ago, so it seems we need to wait on that for working rolyn support.

[SofusA]

Thanks to @MoaidHathot for pointing this out. It looks like utilizing csharp-language-server outside of Visual Studio Code goes against the Dev Kit license making this usage illegal in Zed.

The tool only utilises code from Roslyn which is licensed under MIT Licence. It does do use anything from C# DevKit.

C# DevKit utilises the same underlying language server from Roslyn, Microsoft.CodeAnalysis.LanguageServer, but csharp-language-server has nothing to do with C# DevKit.

[Digni]

@SofusA was quicker to respond. And that is true, the server uses the Microsoft.CodeAnalysis.LanguageServer. This can be found here: https://github.com/dotnet/roslyn.
And that is MIT License, so no DevKit is used.

[dylhack]

Ah okay looks like a misinterpretation of csharp-language-server and the DevKit license. Love it so far! Great work.

[Digni]

Well they are though:
https://dev.azure.com/azure-public/vside/_artifacts/feed/vs-impl/NuGet/Microsoft.CodeAnalysis.LanguageServer.osx-arm64/overview/5.0.0-1.25302.10
https://dev.azure.com/azure-public/vside/_artifacts/feed/vs-impl/NuGet/Microsoft.CodeAnalysis.LanguageServer.linux-x64/overview/5.0.0-1.25302.10
https://dev.azure.com/azure-public/vside/_artifacts/feed/vs-impl/NuGet/Microsoft.CodeAnalysis.LanguageServer.win-x64/overview/5.0.0-1.25302.10

They are all MIT Licensed, as can be seen in the nuget Feed.

[julionav]

@Digni any chance that this can be run alongside omnisharp? to have the diagnostics. Right now the omnisharp ls doesn't work when decompiling sources (go to definition)

[Digni]

@Digni any chance that this can be run alongside omnisharp? to have the diagnostics. Right now the omnisharp ls doesn't work when decompiling sources (go to definition)

Yeah, Zed supports running several lsp's for one language.
https://zed.dev/docs/configuring-languages

  "languages": {
    "CSharp": {
      "language_servers": ["roslyn", "omnisharp", "..."]
    }
  }

But as written in the ruby extension (https://zed.dev/docs/languages/ruby) they would provide similar functionality. So we probably need to turn off autocompletion, code-actions from omnisharp.

https://github.com/OmniSharp/omnisharp-roslyn/wiki/Configuration-Options

I did not look into that yet.

[AleksaBajat]

@SofusA @Digni It seems that Zed closed this PR. Let me know if I can help out in pursuing this LSP.

[Digni]

I just did a quick check on Zed Preview and diagnostics seems to be working now.

[julionav]

@Digni i tried this but I couldn't make the go to definition with decompiling work. Do you know if that should work? E.g go to the definition of a installed package

[SofusA]

@Digni i tried this but I couldn't make the go to definition with decompiling work. Do you know if that should work? E.g go to the definition of a installed package

I get working decompiling using the the config mentioned in my previous comment.

[Digni]

@Digni i tried this but I couldn't make the go to definition with decompiling work. Do you know if that should work? E.g go to the definition of a installed package

I did just test it and go-to definition works for me with decompilation (using the updated extension).
Sometimes I had some weird behavior though, in this case it seems like the language server did not pick the right solution file, opening a file in the solution (e.g. closest program.cs) you want and restarting the server solved that one for me.

In this case it would be best if we could ask the user which solution to open, but that seems not possible at the moment with the extension api.

@SofusA We could maybe follow the method used by DotRush and pass options along to the language server which contains the path to the solution. The user could provide this in the project specific settings.

                    "roslyn": {
                        // Paths to project or solution files to load
                        "projectOrSolutionFiles": [
                            "/path/to/your/solution.sln"
                        ]
                    }

[SofusA]

Sometimes I had some weird behavior though, in this case it seems like the language server did not pick the right solution file, opening a file in the solution (e.g. closest program.cs) you want and restarting the server solved that one for me.

Hmm this does sound weird. The solution file is searched for from the directory that the csharp-language-server is started from.
In helix, i specify the root of the project with a roots property in the language configuration. Maybe zed has something similar?

@SofusA We could maybe follow the method used by DotRush and pass options along to the language server which contains the path to the solution. The user could provide this in the project specific settings.

I have just added a release which provide arguments for specifying solution or project paths.

[MrSubidubi]

Hey, sorry for the late reply.
I see some discussion above, may I ask what the state is on this? Do you plan on doing more work or is this ready for review with pull diagnostics now merged on Zed's side?

[Digni]

Hey,
I was also a bit swamped the last few weeks. With the update for Pull Diagnostics it works quite well. Only issue is with several solutions in your folder structure, that it could pick the wrong one currently.
A fix could be to provide the solution path to the executable as a parameter, i still wanted to take a look at that.

But it is working as is and we could merge it, while this update is done separately.

[MrSubidubi]

Alright, I'm unfortunately OOO soon and got some other stuff on my list but I'll do my best to get back to you regardless over the next few days/weeks so we can have this merged sooner than later. I'll leave some comments in the code as well, the biggest question I have prior to merging is regarding the LSP settings part:

In the settings json I made it nested:

  "lsp": {
    "roslyn": {
      "settings": {
        "csharp|inlay_hints": {
          "csharp_enable_inlay_hints_for_implicit_object_creation": true,
          "csharp_enable_inlay_hints_for_implicit_variable_types": true,
          "csharp_enable_inlay_hints_for_lambda_parameter_types": true,
          "csharp_enable_inlay_hints_for_types": true,
          "dotnet_enable_inlay_hints_for_indexer_parameters": true,
          "dotnet_enable_inlay_hints_for_literal_parameters": true,
          "dotnet_enable_inlay_hints_for_object_creation_parameters": true,
          "dotnet_enable_inlay_hints_for_other_parameters": true,
          "dotnet_enable_inlay_hints_for_parameters": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_differ_only_by_suffix": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_match_argument_name": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_match_method_intent": true
        }
      }
    }
  }

Not insisting, you have more context here and I might also be missing something, but could we do it any better than this? Especially, if I am to understand this corrrectly, this is needed for inlay hints to work, right? Can we

1. provide better defaults so that most users don't have to edit their settings in the average case for this to work?
2. Make this even easier to configure? Given that we have no JSON schema completion for LSP settings, this seems not too good to configure and we'll definitely need documentation at least, yet I can't help but wonder if we can not just improve it some more in the extension itself. Open to ideas here. If we can't do something like that, I'd at least want to have better defaults so that things just work for most users and having to look up the config is the exception rather than the norm

---

Regarding the

Only issue is with several solutions in your folder structure, that it could pick the wrong one currently.

IIRC we have this issue for some other LSPs currently and I believe there is some ongoing work in the core to better tackle this. Ultimatively, I think this should be easy to do with the extension API. Let's skip it for now and revisit this later once the work is completed there.

[MrSubidubi]

Left some comments, overall looks good, just some style and other minor concerns.

[MrSubidubi]

Just curious - for which operating systems did you test this? Just so I can take a look at the other systems if needed.

[Digni]

Only for mac with arch. I don't have a set up linux system right now.

[MrSubidubi]

Alright, thanks for the information. I'll make sure to test it on the other platforms just to be sure.

[Digni]

Alright, I'm unfortunately OOO soon and got some other stuff on my list but I'll do my best to get back to you regardless over the next few days/weeks so we can have this merged sooner than later. I'll leave some comments in the code as well, the biggest question I have prior to merging is regarding the LSP settings part:

In the settings json I made it nested:

  "lsp": {
    "roslyn": {
      "settings": {
        "csharp|inlay_hints": {
          "csharp_enable_inlay_hints_for_implicit_object_creation": true,
          "csharp_enable_inlay_hints_for_implicit_variable_types": true,
          "csharp_enable_inlay_hints_for_lambda_parameter_types": true,
          "csharp_enable_inlay_hints_for_types": true,
          "dotnet_enable_inlay_hints_for_indexer_parameters": true,
          "dotnet_enable_inlay_hints_for_literal_parameters": true,
          "dotnet_enable_inlay_hints_for_object_creation_parameters": true,
          "dotnet_enable_inlay_hints_for_other_parameters": true,
          "dotnet_enable_inlay_hints_for_parameters": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_differ_only_by_suffix": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_match_argument_name": true,
          "dotnet_suppress_inlay_hints_for_parameters_that_match_method_intent": true
        }
      }
    }
  }

Not insisting, you have more context here and I might also be missing something, but could we do it any better than this? Especially, if I am to understand this corrrectly, this is needed for inlay hints to work, right? Can we

1. provide better defaults so that most users don't have to edit their settings in the average case for this to work?
2. Make this even easier to configure? Given that we have no JSON schema completion for LSP settings, this seems not too good to configure and we'll definitely need documentation at least, yet I can't help but wonder if we can not just improve it some more in the extension itself. Open to ideas here. If we can't do something like that, I'd at least want to have better defaults so that things just work for most users and having to look up the config is the exception rather than the norm

Regarding the

Only issue is with several solutions in your folder structure, that it could pick the wrong one currently.

IIRC we have this issue for some other LSPs currently and I believe there is some ongoing work in the core to better tackle this. Ultimatively, I think this should be easy to do with the extension API. Let's skip it for now and revisit this later once the work is completed there.

That is a valid point, but as commented this is kinda how the Roslyn LSP expects its configuration.
VsCode has a mapping for that, so you see just some options in the settings (e.g a Checkbox) and then it maps it to this format. But there is also no real documentation, except going through the vscode extension, to get all the options to map them. But we can do that i think.

IMO the defaults of roslyn are quite good as is. This is really only for the inline/inlay hints to work.

[MrSubidubi]

But we can do that i think.

If you have the time and are willing to take another look in that regard, I'd highly appreciate it. Ultimately, I'd just like to make it as easy for users as possible. I think it is fine if we increase the maintenance burdon slightly in general if that increases usability a lot - it's obviously Microsoft's fault, but I'd like if we find a way to not directly throw this at users and at least try to make it just a bit better. At least, better documentation both in the official docs and in this repo would already be a good start if we don't find any better solution.

IMO the defaults of roslyn are quite good as is. This is really only for the inline/inlay hints to work.

Failed to explain myself here, I guess, sorry for that - what I meant is that optimally I'd like inlay hints to also work out of the box without any additional configuration.

Also, thanks a lot for following up that quickly! Edit: And sorry for mistyping on the suggestion! 😅

[Digni]

How about using these inlay settings as the default. If the user decides to add a custom config, his config will take precedence.
Otherwise this config is used, while also adding this information to the readme here?

[MrSubidubi]

I think this sounds like a sensible and the most straightforward approach - feel free to go forward with this.

Please also note that I'll now be OOO as mentioned for about two weeks (technically I already am OOO right now, but given your fast follow up, I decided to give a quick reply) so there might be a slight delay in responses from me from now on. That said, if you have time and interest to play around with this some more beyond this approach, feel free to and I'll take a look!

I'll make Roslyn the default for C# once this is in. Especially with official Windows support just around the corner, I wanna make sure that we hit the mark with this once it's merged.

[Digni]

Okay, will see what I can do.
Have a nice time off

[MrSubidubi]

Thank you! 🙂
I'll periodically check in, so if you need anything or think it's ready (perhaps as-is), just let me know.

[MrSubidubi]

Hey, checking back in - how are things looking? Did you find time to play with different approaches or do we just wanna go with the "inlay hints enabled by default" route for now?

I don't wanna bother you too long with this (really thankful for you getting this up and ready in the first place), so if you prefer, let's just get this ready to be merged and follow up with anything we find later.

[AleksaBajat]

@MrSubidubi With the amount of value this provides in comparison to the other C# LSP we should have it shipped and then worry about breaking changes in the next major version of the plugin.