First Draft of Common Language Server Protocol Framework (CLaSP)

[ryanbrandenburg]

Roslyn has a good framework for hosting Language servers in a way that enables quick. Since other projects (Razor being the first) would like to use this system also I've done some work here to separate the parts likely to be common to any LSP implementor from those parts specific to Roslyns LanguageServers.

Goals:

1. Create a new package "CommonLanguageServerProtocol.Framework" where this code can live. Nobody should have to load any parts of Roslyn in order to host their LS, so a hard separation here is desirable.
2. Lots of abstraction. We want to enable other LS's to do what makes sense for their scenarios, so even if we provide a "default" implementation in CLaSP Framework we want to enable devs to replace it.
3. Roslyn as the first consumer. Since Roslyn already uses this system and only has to adapt somewhat to the new abstractions they're a logical first customer. Semantic Tokens worked when I tested things locally and I got no errors I'm sure there will be more work here once I run the tests but I wanted to get things in front of people ASAP.

Still to do:

1. Clean up code. There's a lot of class's and interfaces sharing files along with other code clean-up crimes, these will of course be cleaned up before merging.
2. Onboard Razor. We want to make sure that all these abstractions work for Razor as proof that no/not-too-many "Roslyn-isms" slipped through that other consumers might not want.
3. NuGet packages shipping.
4. Tests. Need to be both created for ClaSP itself and modified existing ones to make them pass in the new world.

Huge thanks to @dibarbet and @davidwengier for writing the Queueing, Services and Workspace's systems so I could rip them off.

[ryanbrandenburg]

Policy will probably dictate that this package begin with a reserved prefix like Microsoft.

[tmat]

It's not clear to me what code are we trying to reuse between Roslyn and Razor LSP server implementations. Can you list what you envision sharing?

[ryanbrandenburg]

Aka, if protocol changes does that mean we need to release anew CLaSP? Can it be decoupled? Does it matter?

If all the protocol changes is the inclusion of new endpoints or the changing of existing params I think we're decoupled enough to avoid needing updates. The areas (that I can think of) where that is NOT true are pre-set methods like Initialize (which we had talked about splitting out anyway) and ClientCapabilities. Hypothetically if we implement serialization correctly (haven't worked on this yet since it was working out of the box so far for the Roslyn scenarios I tested) we should be able to serialize ClientCapabilities to a descendent as long as the change is additive.

If y'all spot any other coupling let me know.

[NTaylorMullen]

Hypothetically if we implement serialization correctly (haven't worked on this yet since it was working out of the box so far for the Roslyn scenarios I tested) we should be able to serialize ClientCapabilities to a descendent as long as the change is additive.

I see, it definitely feels odd to have a dependency on the protocol types purely for a base "InitializeParams" object. I imagine the next step would be someone who depended on CLaSP would then re-define their own version and would then need to binding redirect the protocol assemblies to be the correct version, all potential friction points if multiple people are shipping CLaSP simultaneously.

Let me see if I can write down the example that comes to mind: imagine Roslyn + Razor are on CLaSP and CLaSP itself depends on VS.Protocol. Lets say that looks something like this:

        Roslyn
      /            \
CLaSP    VS.Protocol 2.0.0
   |
VS.Protocol 1.0.0

and

        Razor
      /            \
CLaSP    VS.Protocol 3.0.0
   |
VS.Protocol 1.0.0

Even though both Roslyn & Razor depend on the same version of CLaSP they both now need to binding redirect VS.Protocol v1.0.0 -> v3.0.0 and v2.0.0 if they want to interact with CLaSP's initialize params object; however, because CLaSP depends on the protocol neither Roslyn or Razor can binding redirect 1.0.0 to their corresponding version because that'd then break the other partner team.

I thinkkkkkkkk this is a problem or did I mis-represent something?

[CyrusNajmabadi]

It would be great to have a meeting that does a high level design review of what's going on here. Thanks!

[ryanbrandenburg]

I see, it definitely feels odd to have a dependency on the protocol types purely for a base "InitializeParams" object. I imagine the next step would be someone who depended on CLaSP would then re-define their own version and would then need to binding redirect the protocol assemblies to be the correct version, all potential friction points if multiple people are shipping CLaSP simultaneously.

...

I thinkkkkkkkk this is a problem or did I mis-represent something?

@NTaylorMullen and I talked about this offline. The determination was that using VS.Protocol was not actually desirable in this case because of licensing issues (before we even consider binding re-directs).

[ryanbrandenburg]

Lots of updates, equivalent for the surface area PR coming soon.

[333fred]

This introduces a new set of public APIs for the LSP. I'm super excited to see this, but we also need to take it through a public API review. At first glance, this is going to require multiple dedicated sessions to review. @ryanbrandenburg and @dibarbet, can we please sync on getting this new API reviewed?

[ryanbrandenburg]

Is there anything in particular we should be looking at? GitHub breaks when I try to get it to show me commits since my last review...

It's hard to narrow down since I've mostly been responding to PR feedback and Analyzer Warnings for the last few days. Maybe just focus in on the actual CLaSP API's and maybe any areas of Roslyn that you're particularly familiar with?

[davidwengier]

Boy does github suck for long reviews. I think "Add single comment" might work better, but as it is, I have no idea what question this is answering, for example:

CC @davidwengier who IIRC was an advocate for the handlers.

I definitely was, but at the time I was coming from the idea of this being a standard, but opinionated, implementation of a language server that is easy to build on. I think that impression was wrong, and this is more foundational and unopinionated, so I'm fine with whatever works best

[ryanbrandenburg]

@333fred I'm removing the "Needs API review" tag after our previous conversation about putting this on a non-public feed for now. Let me know if that's not appropriate.