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

[Alpha] Language Server Protocol #969

Merged
merged 51 commits into from
Jan 9, 2018
Merged

[Alpha] Language Server Protocol #969

merged 51 commits into from
Jan 9, 2018

Conversation

david-driscoll
Copy link
Member

@david-driscoll david-driscoll commented Sep 25, 2017

Epic: #968

This is the first step in getting LSP support integrated into OmniSharp.

Lsp packages have been pushed to nuget here and here.

This starts consuming them, the branch lsp may be long lived, or we may merge into master a few times, we haven't discussed this yet.

For testing I was using the test extension from the csharp-lsp repository, and simplying making sure that OmniSharp.exe was invoked with -lsp or --languageserver and it will negotiate with language server mode automatically. It would be great to get others feedback using this against language server 2.0 vs 3.0 (that vscode implements).

Note: not all endpoints currently map 1:1 to LSP, more information on lsp can be found here and the C# implementation here

Parts done:

  • Wired in language server protocol to the default host
  • Endpoints configured so far :
    • /gotodefinition
    • /findsymbols
    • /updatebuffer
    • /changebuffer
    • /codecheck
    • /filesChanged
    • /formatAfterKeystroke
    • /formatRange
    • /codeformat
    • /highlight
    • /autocomplete
    • /findimplementations
    • /findusages
    • /gotofile
    • /gotoregion
    • /navigateup
    • /navigatedown
    • /typelookup
    • /getcodeactions
    • /runcodeaction
    • /rename
    • /signatureHelp
    • /currentfilemembersastree
    • /currentfilemembersasflat
    • /gettestcontext
    • /metadata
    • /packagesource
    • /packagesearch
    • /packageversion
    • /projects
    • /project
    • /fixusings
    • /checkalivestatus
    • /checkreadystatus
    • /stopserver
    • /open
    • /close
    • /diagnostics
    • /v2/getcodeactions
    • /v2/runcodeaction
    • /v2/getteststartinfo
    • /v2/runtest
    • /v2/debugtest/getstartinfo
    • /v2/debugtest/launch
    • /v2/debugtest/stop

@david-driscoll
Copy link
Member Author

cc @mickaelistria

@david-driscoll
Copy link
Member Author

Also a reference implementation of the current working mappings can be found in the existing OmniSharp-node-client repo by looking at /languageserver/server.ts

[ImportingConstructor]
public HoverHandler(IEnumerable<IRequestHandler> handlers, DocumentSelector documentSelector)
{
_definitionHandler = handlers.OfType<IRequestHandler<TypeLookupRequest, TypeLookupResponse>>().Single();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is multiple languages supported? E.g. in the Cake PR we register a second handler of this type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came to that realization last night, and added a comment about changing that around. I'll be thinking about that today and probably figure out a decent solution fairly soon.

The note wasn't near that code however

// TODO: Get these with metadata so we can attach languages
// This will thne let us build up a better document filter, and add handles foreach type of handler
// This will mean that we will have a strategy to create handlers from the interface type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjorkstromm
Copy link
Member

@david-driscoll, EPIC! Do you accept PRs to this branch if I could find some time for helping out with endpoint mapping?

@david-driscoll
Copy link
Member Author

@mholo65 absolutely, please feel free to branch from there and create a pull request into it. I'm going to look at testing cake support if I get a chance here.

@david-driscoll
Copy link
Member Author

@mholo65 I've made a temporary branch that integrates things https://github.com/david-driscoll/Omnisharp-roslyn/tree/temp/cake

… correctly. Tested with cake inegration and found an issue if the filename is null, and avoid that scenario
@bjorkstromm
Copy link
Member

Thanks @david-driscoll. What client do you use to test LSP? Could you provide some steps on how to get it up and running?

@david-driscoll
Copy link
Member Author

I was using https://github.com/OmniSharp/csharp-language-server-protocol/tree/master/vscode-testextension . Again this was just an informal test to ensure things were working as expected. I did encounter a few bugs in the lsp server so I fixed those.

To get it running, the only changes you need to make are to point it at a version of OmniSharp that has lsp support, like temp/cake above. reference point

@bjorkstromm
Copy link
Member

@david-driscoll ok, I'm working on the CompletionHandler now.

@DustinCampbell
Copy link
Contributor

@david-driscoll: you'll want to merge the latest master changes into lsp in order to get fixes for the OSX build.

@david-driscoll
Copy link
Member Author

Going to put this through as is tomorrow, if there aren't any objections. We'll continue to iterate and add handlers over the next few weeks.

@DustinCampbell
Copy link
Contributor

Going to put this through as is tomorrow, if there aren't any objections. We'll continue to iterate and add handlers over the next few weeks.

Could you wait until Monday? I'd like to get a new OmniSharp build out with a couple of bug fixes before Monday so we can use it in a C# for VS Code release.

@david-driscoll
Copy link
Member Author

Yeah that's fine. 👍

@DustinCampbell
Copy link
Contributor

FYI: All is good. This can be merged now to simplify development.

<ProjectReference Include="..\OmniSharp.DotNet\OmniSharp.DotNet.csproj" />
<ProjectReference Include="..\OmniSharp.DotNetTest\OmniSharp.DotNetTest.csproj" />
<ProjectReference Include="..\OmniSharp.MSBuild\OmniSharp.MSBuild.csproj" />
<ProjectReference Include="..\OmniSharp.Script\OmniSharp.Script.csproj" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference OmniSharp.Cake!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Not ! (Clumsy me on phone)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling some of those references can go away, because we don't specifically get types for a given project system, we just use the metadata from MEF.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however I will validate that in a day or two.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d think it will not load unless it’s a compile time dependency. I might be wrong though :) https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Host/CompositionHostBuilder.cs#L158

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a dependency via OmniSharp.Stdio and we bootstrap Lsp through Stdio.
https://github.com/OmniSharp/omnisharp-roslyn/blob/lsp/src/OmniSharp.Stdio/Program.cs#L36

We need to make a unified bootstrapper or rename stdio in a future change.

@aignas
Copy link

aignas commented Dec 16, 2017

Hello, I am interested in testing this with https://github.com/autozimu/LanguageClient-neovim as an alternative to https://github.com/OmniSharp/omnisharp-vim. I am using ArchLinux and I am having trouble building it, but I had great success using the precompiled binaries via the AUR package.

What is the status of this?

@david-driscoll
Copy link
Member Author

Standard issue with mac build failing, forcing the merge to get this in so consumes can test.

@david-driscoll david-driscoll merged commit f0a3ac1 into master Jan 9, 2018
@filipw
Copy link
Member

filipw commented Jan 11, 2018

Standard issue with mac build failing, forcing the merge to get this in so consumes can test.

🎉🎉🎉

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

Successfully merging this pull request may close these issues.

7 participants