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

Migrate C# completer to OmniSharp-Roslyn #1082

Closed
micbou opened this issue Aug 13, 2018 · 16 comments
Closed

Migrate C# completer to OmniSharp-Roslyn #1082

micbou opened this issue Aug 13, 2018 · 16 comments

Comments

@micbou
Copy link
Collaborator

micbou commented Aug 13, 2018

The C# completer currently relies on OmniSharpServer which is not maintained anymore. OmniSharp-Roslyn should be used instead. PR #885 implements this but there are some issues with it:

  • keep support for OmniSharpServer: we don't want to maintain two backends for one language;
  • add support for Cygwin: something else we don't want to maintain;
  • do not use LSP which could simplify things (or maybe not, we are talking about LSP after all);
  • seem abandoned.
@bstaletic
Copy link
Collaborator

The problem with Roslyn is that it doesn't support FixIts and that's one of the blockers.

@micbou
Copy link
Collaborator Author

micbou commented Aug 13, 2018

I completely missed that. I suppose we need to wait for PR OmniSharp/omnisharp-roslyn#1076 then.

@bstaletic
Copy link
Collaborator

Woah! Finaly... however, they didn't respond to @mispencer's question about if they plan to support FixIts when he asked. :/

@insidewhy
Copy link

insidewhy commented Mar 13, 2019

The version of OmniSharp that works with YouCompleteMe is so ancient as to be useless for a lot of people.

Are fixits really needed? I don't have a clue what they are. If not, are they as useful as not being on an ancient version of C#?

Given that many other extensions manage to use OmniSharp-roslyn I'm thinking there's got to be a more pragmatic solution here.

@bstaletic
Copy link
Collaborator

Are fixits really needed? I don't have a clue what they are. If not, are they as useful as not being on an ancient version of C#?

There is a :YcmCompleter FixIt command that can fix some syntax errors, like a missing semicolon.

Since OmniSharp/omnisharp-roslyn#1076 doesn't seem like it will be merged any time soon, I'm starting to think that FixIts are an acceptable loss at this point.

Just a note, the LSP interface for Roslyn was in a terrible state last time I checked ad if it is still the case, we shouldn't use it.

@insidewhy
Copy link

@bstaletic The current OmniSharp server isn't using LSP either, so wouldn't it be okay to migrate to the roslyn version without double-migrating also to LSP? Feels like a simpler more incremental change? When it comes to completion via LSP there's a lot of really great solutions, I think the reason a lot of people still use YCM is precisely because it has such great support for so many langauges, many not yet covered by LSP.

@bstaletic
Copy link
Collaborator

The current OmniSharp server can't use LSP. Under the, likely wrong, assumption that OmniSharp-Roslyn has a good LSP implementation, Roslyn support could be made very simple.

This is a working implementation of a PHP LSP server.

This is the LSP discussion for Roslyn: OmniSharp/omnisharp-roslyn#968 so maybe it is worth a try to avoid writing a complex completer.

@insidewhy
Copy link

Yep I've been experimenting with OmniSharp-Roslyn's LSP implementation, a few things work well but many things don't work at all. The upgrade path from Omnisharp to Omnisharp-roslyn isn't much work, I think it'd be the only thing capable of delivering really great code completion to YCM (and vim/neovim) without too much effort. C# code completion might remain out of the realm of possibility for vim users for some time unless we make a pragmatic stop-gap solution with what we have.

@insidewhy
Copy link

Anyway this is something I'm happy to work on, I totally understand if you don't want it in the mainline YCM repository. If I have to maintain a fork for a short time to get decent C# completion without having to use VSCodeVim then I'm also happy to do that. Hopefully the LSP support will progress quickly enough that whatever happens can be trashed fairly quickly. For me at least, it'd be worth my effort so that I can use my favourite editor with the Unity project I'm working with for the next few months.

@puremourning
Copy link
Member

I suspect that losing FixIts is not the end of the world. @mispencer maintained the C# support, so I assume their opinion counts too, but provided the server is maintained and stable and the tests are adequate, i would be happy to use roslyn on the basis that is what's actively developed and supported by MS, right ?

Does anyone know what VSCode uses for non-windows c# support? When i looked at it for vimspector, it looked like they use mono + proprietary licensed MS stuff where the license basically says "can't be used outside of VScode". I take it they are not using omnisharp ?

@bstaletic
Copy link
Collaborator

Roslyn is what is maintained and supported. To summarize my thoughts:

  • FixIts aren't worth keeping legacy server supported and thus making YCM's C# completer useless for the ever growing number of users.
  • Cygwin support would add a ton of complexity and supporting it means actually making all completers work with it, plus running another appveyor job. Without one of the core maintainers using and actively supporting cygwin, I don't think it is worth the trouble.
  • Roslyn LSP support is not the way to go.
  • While at this point there is a case of two servers for a single language, I'd still want to avoid that as much as possible.

@mispencer
Copy link
Contributor

If there is interest, I can rework and resubmit my earlier PR without the Cygwin support or legacy Omnisharp support.

I don't personally use Cygwin anymore (like many people I suspect, I have migrated to WSL). So I am ok with removing the Cygwin logic from my earlier PR. I am also ok with removing the legacy Omnisharp support entirely at this point. Unlike two years ago, I don't need support for legacy code.

Last I checked (which was quite awhile ago), VS Code is using OmniSharp though the Omnisharp-vscode extension.

@bstaletic
Copy link
Collaborator

I'd be completely fine with migrating to Roslyn without cygwin support and with legacy server being dropped completely.

@Valloric
Copy link
Member

Valloric commented Mar 16, 2019

I'd be willing to lose plenty of existing features to update to the latest Roslyn parser. As has been stated here, our existing C# support is pretty out of date at this point.

Code completion comes first, and we need to do that feature well.

@mispencer
Copy link
Contributor

I created #1209 for this.

@bstaletic
Copy link
Collaborator

Done in #1274.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants