-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
C# Support: Add treesitter and OmniSharp LSP support #6908
Conversation
We require contributors to sign our Contributor License Agreement, and we don't have @fminkowski on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this language! I added a note to the highlights file about incorrect annotations. Could you also include an outline.scm
file? This is what other tree sitter projects might call a tags
file, and powers Zed's outline view (cmd-shift-O
).
Also, could you include a screenshot comparing this configuration's rendering of a C# file in Zed to the same file in your preferred editor?
(#eq? @variable.builtin "_")) | ||
|
||
(method_declaration | ||
name: (identifier) @function.method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use function.method
, and several other highlights in this file. for the list of annotations we do support, see: https://github.com/zed-industries/zed/blob/main/crates/theme/src/styles/syntax.rs#L16-L68
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll update these, the other highlights, and add the outline.scm
. It looks like the other highlights in the repo do use function.method
though. Maybe those need updated too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made these updates and added reference screenshots comparing to VS Code. Let me know if you need anything else.
It is not recommended to use OmniSharp anymore now that the Roslyn LSP is out. OmniSharp is far inferior. |
This is the OmniSharp-Roslyn LSP. |
Yes, OmniSharp is outdated. |
This is using the same version that the C# dev kit in VS Code uses when the |
I apologize for derailing this topic, it appears I was misinformed. |
No problem! Glad we got it figured out. |
Hi, O# maintainer and roslyn team member here. This is not correct. There are 2 extensions at play here: the C# extension, and DevKit. The C# extension is the entirely-open-source bit (except the debugger), and has 2 available servers. By default, it uses the Roslyn LSP server, but you can switch it back to use O# if you so choose (in either framework or modern variants, which is what the useModernNet switch governs). Note that when you do so, vscode doesn't actually use O# in LSP mode, it uses it in a custom protocol that predates and inspired LSP. Both of these servers can be freely used anywhere you choose, including Zed, as they are both MIT licensed. On the other hand, DevKit is a closed-source extension that adds additional functionality to the C# experience. It forces the use of the Roslyn LSP and is not licensed for use outside of vscode. As I said, you are free to use either the Roslyn LSP or O#. If I were making a new editor support, I would probably go with the pure Roslyn solution, but it's up to you, and I don't want to imply that O# is a bad choice, because it's not. But I do expect that the pure Roslyn LSP will be more actively maintained and improved at this point. |
Thanks for the clarification, @333fred . How much extra work, in your estimation, would be involved in converting or changing the current PR to use the pure Roslyn solution? Just a ballpark figure, a rough guess. Would it range from trivial to a complete overhaul, essentially starting from scratch and being a completely different project/PR?
|
Thanks for further clarifying @333fred! The Do we have to create a project, pull in the As @solrevdev said, a ballpark estimate of what this would take will be really helpful. Along with any documentation that you think will help. |
This is the package source for the LSP: https://dev.azure.com/azure-public/vside/_artifacts/feed/msft_consumption/NuGet/Microsoft.CodeAnalysis.LanguageServer. I wouldn't expect that setup would be particularly more complex than O#, https://github.com/dotnet/vscode-csharp/blob/c5d0e9e98c0b925a4f74ae7c73a8d4b37f98005c/src/lsptoolshost/roslynLanguageServer.ts is how vscode does it. Feel free to reference with appropriate accredation. Note that we ship each platform with its own vsix, we acquire specific packages during build here: https://github.com/dotnet/vscode-csharp/blob/c5d0e9e98c0b925a4f74ae7c73a8d4b37f98005c/tasks/offlinePackagingTasks.ts#L169. Definitely agree that this is a bit confusing. When we moved to the native Roslyn LSP, we wanted to make sure that people who were happy with O# didn't have the rug ripped out from under them, or that people who wanted to use their own other LSP could do so if they chose. Depending on which route you go, either @dibarbet (pure Roslyn LSP) or @JoeRobich (O# LSP) are great resources to reach out to if you run into problems. |
Thanks @333fred. It does not look as straightforward to integrate the Roslyn LSP as it is the OmniSharp LSP. I think using the Roslyn LSP will take some more research. I would like to move forward with OmniSharp at this time since it seems to fit with the current architecture - fetch an executable and spin it up. A lot of other editors are still using OmniSharp LSP too - nvim, helix, etc. Open to feedback on this, but I will not be pursuing Roslyn LSP any further with this PR. |
Yeah, omnisharp gets a +1 from me for now. It does the job in sublime text for me. Thanks for adding C# @fminkowski |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zed currently doesn't use a query for folds. It looks like some folds.scm
files got added by mistake for Zig and Svelte in other PRs. Could you remove this one?
Nice work @fminkowski, thanks for the PR. I left one comment. |
This looks close to be done, so it would be great to add a new docs entry to the https://github.com/zed-industries/zed/tree/main/docs/src/languages list before merging. |
I don't want to push back against such a decision in general, but I will say that the roslyn LSP will be pretty much the same. Here's an example from nvim: https://github.com/jmederosalvarado/roslyn.nvim. |
4ff5b43
to
b0e80bb
Compare
b0e80bb
to
53df8ed
Compare
To add on top of what Fred was saying, as one of the primary owners of the Roslyn LSP impl, i would strongly recommend going with our LSP and going the LSP route, vs the omnisharp route. A few of core reasons:
And so on and so forth. |
@CyrusNajmabadi I'm definitely up for researching using the Roslyn LSP. Where can I find documentation how how to use it, build it, where it lives, etc? I need something to point me in the right direction. Right now, all I have are links to how others are using it. With OmniSharp I was able to quickly and easily use it. But with the Roslyn LSP it has not been obvious. Can you point me in the right direction? I am open to doing some follow up work to move over to the Roslyn LSP. But right now, I don't know what that requires. Whereas with OmniSharp, it was very straightforward. I look forward to your help! |
Hi: You can see our language server here: https://github.com/dotnet/roslyn/tree/main/src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer It's just an exe that launches that then has a normal server that speaks the LSP protocol over a named pipe. The server launches, and sends the named pipe name back to the client to then connect to. @davidbarbet could you link to the client side code that then interfaces with this? |
Unfortunately we don't have great existing docs on how to use it - however, it shouldn't be a lot more complicated than O# to setup, so I'll try and outline the general steps.
There are likely to be a few rough edges here, but feel free to ping me on github (or email me directly) if you have questions or suggestions on how we can improve this. |
@dibarbet This is awesome! Thanks so much for sharing these details. If @mikayla-maki and @maxbrunsfeld are ok with with the PR in its current state, we can move forward with this PR so Zed has C# syntax highlighting and at least OmniSharp support for now. And in the meantime, I can start working on getting the Roslyn LSP integrated. But with the details provided above, I think it will be possible, but may take a bit of fiddling. |
@dibarbet I am now able to run the Roslyn LSP thanks to your instructions. I now have a clearer understanding of how the Roslyn LSP works and what may need to be done to integrate it with Zed. Unfortunately, I don't think it is very straightforward to add at the moment, since it doesn't utilize stdin. Looking through the Zed source code, it looks like that is the only supported pipe right now. Maybe one of the maintainers can provide more insight on this? So, I still think we should proceed with OmniSharp and I will continue researching what it will take to get Roslyn support added. |
I'm ok with merging the OmniSharp version. Then, if you're interested @fminkowski, you could open a PR that makes Zed compatible with Roslyn LSP, and switches to using that. Do we need to talk to the LSP over a TCP socket instead of stdin? @dibarbet Thanks so much for the rundown. |
@maxbrunsfeld Sounds good. Yes, it looks like it utilizes a domain socket for IPC. So it will require some additional changes to support. With the details that @dibarbet provided above I think I can get it to work, but it will take some time and it will require more feedback. So, let's move forward with this as is and I'll work on support for Roslyn LSP. |
Nice work @fminkowski. This will ship in Zed 0.121. |
@fminkowski @maxbrunsfeld Sorry for the noobie question... but how do I use this? I'm new to Zed and don't see any docs on C# support or how to set up the Omnisharp LSP. I'm using Zed 0.132.2 (@maxbrunsfeld said this will ship in Zed 0.121) and it seems like out of the box it doesn't have C# support. Either I didn't configure something, or it's broken. I don't see C# in the list of languages in the language selector, and it doesn't seem to get chosen when I open any .cs files. There's no syntax highlighting, etc. Am I doing something wrong, or is this a bug? If it's the latter lmk and I'll open a separate issue for this. Thanks for your hard work everyone! I'll be happy to contribute docs if needed once I understand how to fix this. |
@davidnagli See if the extension is installed by running |
This PR adds the C# tree-sitter grammar. It also adds OmniSharp-Roslyn for LSP support.
Resolves issue #5299
Release Notes:
VSCode
Zed