[RFC] Switch C# completer to LSP #1752
Conversation
bstaletic
changed the title
I got CodeActions working.
It should be there!
This should only need an updated monisharp, via
Fixed as well.
Hmm... still true. not sure what I'm doing wrong. |
We can now communicate with the bundled server, but it won't initialize on my machine. This is where my knowledge of C# mess ends and I will be needing help from here on out. |
|
All tests are passing on non-Winblows. |
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, 52 of 55 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
ycmd/completers/cs/cs_completer.py line 88 at r4 (raw file):
def GetProjectRootFiles( self ): return [ '*.csproj' ]
I think we should look for solution files (*.sln) not c-sharp project files?
or at least, both ?
I think it's more normal to find the solution file in the project's true root, and that is what I think omnisharp will read (at least it historically did)
ycmd/completers/cs/cs_completer.py line 117 at r4 (raw file):
if not hover_value: raise RuntimeError( 'No documentation available.' ) markdown_stripped = ''.join( hover_value.split( '```' ) )[ 7: ]
this reads like magic. can we comment it?
ycmd/tests/clangd/subcommands_test.py line 320 at r4 (raw file):
'resolve': True, 'command': has_entries( { 'command': has_entries( { 'command': 'clangd.applyTweak' } ) } )
am I reading this right, we now have command.command.command ?
ycmd/tests/cs/signature_help_test.py line 202 at r4 (raw file):
@IsolatedYcmd( { 'disable_signature_help': True } )
why did we lose this test?
ycmd/tests/rust/subcommands_test.py line 67 at r4 (raw file):
# throws an exception and the easiest way to do that is to throw from # within the FlagsForFile function. app.post_json( test.get( 'route', '/run_completer_command' ),
this looks... wrong ? 'run_completer_command' doesn't know who to handle "FileReadyToParse".
what's going on?
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
ycmd/completers/cs/cs_completer.py line 88 at r4 (raw file):
Previously, puremourning (Ben Jackson) wrote…
I think we should look for solution files (*.sln) not c-sharp project files?
or at least, both ?I think it's more normal to find the solution file in the project's true root, and that is what I think omnisharp will read (at least it historically did)
Hmm... I'm not going to push back a lot, but Omnisharp-Roslyn definitely does read *.csproj. Editing testy.csproj allowed omnisharp to actually find the standard library.
When I created a tiny project to test my local setup, I've done this:
dotnet new console # created *.csproj
dotnet new sln # created *.sln
dotnet sln add *.csproj # adds csproj to the sln file
ycmd/completers/cs/cs_completer.py line 117 at r4 (raw file):
Previously, puremourning (Ben Jackson) wrote…
this reads like magic. can we comment it?
For posterity, the response looks like this
```csharp
type info
```
docstring
So the idea was to drop the first and the third line, to get rid of the silly markdown stuff.
ycmd/tests/clangd/subcommands_test.py line 320 at r4 (raw file):
Previously, puremourning (Ben Jackson) wrote…
am I reading this right, we now have command.command.command ?
Yes. It is not clean, but it avoids changing the ycmd API.
The first command should actually be called code_action.
Now that we receive incomplete code actions, it is not enough to marshal just the code_action['command'] part of it to YCM and back.
We're now marshaling entire code_action objects. But!
That code_action object is still located at fixits_response[ 'fixits' ][ 0 ][ 'command' ] so that clients don't need to change.
ycmd/tests/cs/signature_help_test.py line 202 at r4 (raw file):
Previously, puremourning (Ben Jackson) wrote…
why did we lose this test?
Completion tests are flaky.
The basic completion test triggers completion at the same location.
I can bring it back, but we need to do something about flaky completion tests.
ycmd/tests/rust/subcommands_test.py line 67 at r4 (raw file):
Previously, puremourning (Ben Jackson) wrote…
this looks... wrong ? 'run_completer_command' doesn't know who to handle "FileReadyToParse".
what's going on?
Yeah, that's a very odd comment in a rust/subcommands_test.py!
My bet is... yup, copy paste error.
bstaletic@Gallifrey ycmd (git)-[cs-lsp]-% rg 'within the Flags' ycmd/tests --vimgrep -B3
ycmd/tests/go/subcommands_test.py-59- # Because we aren't testing this command, we *always* ignore errors. This
ycmd/tests/go/subcommands_test.py-60- # is mainly because we (may) want to test scenarios where the completer
ycmd/tests/go/subcommands_test.py-61- # throws an exception and the easiest way to do that is to throw from
ycmd/tests/go/subcommands_test.py:62:5: # within the FlagsForFile function.
--
ycmd/tests/rust/subcommands_test.py-63- # Because we aren't testing this command, we *always* ignore errors. This
ycmd/tests/rust/subcommands_test.py-64- # is mainly because we (may) want to test scenarios where the completer
ycmd/tests/rust/subcommands_test.py-65- # throws an exception and the easiest way to do that is to throw from
ycmd/tests/rust/subcommands_test.py:66:5: # within the FlagsForFile function.
--
ycmd/tests/tern/subcommands_test.py-45- # Because we aren't testing this command, we *always* ignore errors. This
ycmd/tests/tern/subcommands_test.py-46- # is mainly because we (may) want to test scenarios where the completer
ycmd/tests/tern/subcommands_test.py-47- # throws an exception and the easiest way to do that is to throw from
ycmd/tests/tern/subcommands_test.py:48:5: # within the FlagsForFile function.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
ycmd/completers/cs/cs_completer.py line 117 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
For posterity, the response looks like this
```csharp
type info
```docstring
So the idea was to drop the first and the third line, to get rid of the silly markdown stuff.
Done.
ycmd/tests/clangd/subcommands_test.py line 320 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Yes. It is not clean, but it avoids changing the ycmd API.
The firstcommandshould actually be calledcode_action.Now that we receive incomplete code actions, it is not enough to marshal just the
code_action['command']part of it to YCM and back.
We're now marshaling entirecode_actionobjects. But!
Thatcode_actionobject is still located atfixits_response[ 'fixits' ][ 0 ][ 'command' ]so that clients don't need to change.
The first command is a LSP CodeAction.
The second command is a LSP Command.
The third command is a string.
ycmd/tests/rust/subcommands_test.py line 67 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Yeah, that's a very odd comment in a rust/subcommands_test.py!
My bet is... yup, copy paste error.bstaletic@Gallifrey ycmd (git)-[cs-lsp]-% rg 'within the Flags' ycmd/tests --vimgrep -B3 ycmd/tests/go/subcommands_test.py-59- # Because we aren't testing this command, we *always* ignore errors. This ycmd/tests/go/subcommands_test.py-60- # is mainly because we (may) want to test scenarios where the completer ycmd/tests/go/subcommands_test.py-61- # throws an exception and the easiest way to do that is to throw from ycmd/tests/go/subcommands_test.py:62:5: # within the FlagsForFile function. -- ycmd/tests/rust/subcommands_test.py-63- # Because we aren't testing this command, we *always* ignore errors. This ycmd/tests/rust/subcommands_test.py-64- # is mainly because we (may) want to test scenarios where the completer ycmd/tests/rust/subcommands_test.py-65- # throws an exception and the easiest way to do that is to throw from ycmd/tests/rust/subcommands_test.py:66:5: # within the FlagsForFile function. -- ycmd/tests/tern/subcommands_test.py-45- # Because we aren't testing this command, we *always* ignore errors. This ycmd/tests/tern/subcommands_test.py-46- # is mainly because we (may) want to test scenarios where the completer ycmd/tests/tern/subcommands_test.py-47- # throws an exception and the easiest way to do that is to throw from ycmd/tests/tern/subcommands_test.py:48:5: # within the FlagsForFile function.
Should I get rid of those comments in this pull request?
There was a problem hiding this comment.
Trying to get my win blows environment working to test it out and see why.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
bstaletic
force-pushed
the
cs-lsp
branch
4 times, most recently
from
4e0c3d2
to
a23558f
Compare
Currently missing pieces: - GetDoc just does not exist in LSP mode. - CodeActions are ill-formed and hit a million asserts in ycmd. - Currently only works with user_option roslyn_binary_path. - GetProjectRootFiles returns an empty list, so users need to cd to their desired project root.
- Update to 1.39.12 - Make `mono` optional
LSP OmniSharp-Roslyn uses a different request than we previously have for this purposes. This results in some questionable formatting, but there's no way we can fix that without breaking other stuff.
|
The only thing missing here is testing this branch on Windows. |
|
@bstaletic Once again you caught me on vacation, I'll give it a spin Sunday evening or Monday 🙂 |
|
@dkaszews No worries and apologies for interrupting your vacation again! |
Very much not finished. Needs a lot more testing.
Currently missing pieces:
Omnisharp does not seem to be picking up docstrings in LSP mode.CodeActions are ill-formed and hit a million asserts in ycmd. Starting with this one.Currently only works with user_option roslyn_binary_path.GetProjectRootFiles returns an empty list, so users need to cd to their desired project root.I could not get omnisharp to log anything at all.Diagostics work, rename works. I have not tried the rest tonight, but I remember them working.
@dkaszews This should be enough to try it out a bit.
This change is