dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.91k stars 785 forks source link

Migrate F# to LSP pull diagnostics #11969

Open tmat opened 3 years ago

tmat commented 3 years ago

Currently F# uses FSharpDocumentDiagnosticAnalyzer, which implements Roslyn's DocumentDiagnosticAnalyzer.

The goal is to replace this with LSP pull diagnostics.

dsyme commented 3 years ago

What is the benefit of doing this? Does it give any advantages?

tmat commented 3 years ago

We are removing DocumentDiagnosticAnalyzer and related legacy diagnostic reporting infrastructure from Roslyn

dsyme commented 3 years ago

We are removing DocumentDiagnosticAnalyzer and related legacy diagnostic reporting infrastructure from Roslyn

Why?

F# spent about 2 years aligning with Roslyn to get win-win, among other things so we didn't have to re-implement an analysis engine. What's the gain here?

tmat commented 3 years ago

Because LSP is the modern architecture and we do not want to maintain alternative legacy implementation that has performance and correctness problems. This is just the first step on the roadmap to LSP. We will be moving all editor-related Roslyn features to LSP and removing the legacy APIs in future. F# is using a lot of these APIs via External Access layer.

dsyme commented 3 years ago

I see. I know very little about LSP, but never heard of it as "the modern architecture" - I thought it was a bit of a mess from what the Ionide folk have told me. Roslyn seems to give us everything we need doesn't it? What advantages does LSP bring?

I just want to understand what positive customer-facing benefits we're going to see here. How do F# customers gain from something like this? It sounds like an awful lot of work but maybe it's straight-forward. Do we have a model implementation of LSP available anywhere, one that gives the quality and perf characteristics beyond Roslyn?

tmat commented 3 years ago

Roslyn already implements many editor features via LSP. We are currently working on switching our diagnostic reporting fully to LSP. In some scenarios like new ASP.NET Razor editor all editor features are running via LSP. TypeScript, XAML, Python and C++ are also moving to LSP.

The main advantage of LSP is client-server separation. This will allow us to move Roslyn services fully to a separate process and reuse the same implementation on all platforms and for all IDEs we support (VS, VS Mac, VS Code).

dsyme commented 3 years ago

I see, so this solves the out-of-proc question, and we can use the Roslyn LSP implementation as a guide. There are big advantages for F# here, yes. This also matters for F# analyzer support (#11057)

Do you think there is any worth in sharing core parts of the code needed to implement LSP on the LSP-server-side?

Please at some point write us a brief how-to-implement-LSP-server guide (e.g. 5 or 10 links to the core of the Roslyn LSP implementation), to help us get more comfortable with the concept. We've never taken any components out-of-process before and we're not very familiar with that kind of programming in this repo.

dsyme commented 3 years ago

Aside: F# does have an LSP implementation in Ionide. For clarity and community cohesion we'd need to be able to understand whether the long term trajectory is:

  1. unite with that
  2. provide an equivalent LSP implementation from this repo (and would that be used in Ionide)
  3. provide a deliberately trimmed-down LSP implementation only suitable for diagnostic analysis in VS.

Sounds like #3 initially - any LSP implementation would be for VS only - and we'd keep language service features in-memory

tmat commented 3 years ago

Do you think there is any worth in sharing core parts of the code needed to implement LSP on the LSP-server-side?

Possibly. We'll definitely try to help F# team to transition as much as we can.

Please at some point write us a brief how-to-implement-LSP-server guide (e.g. 5 or 10 links to the core of the Roslyn LSP implementation), to help us get more comfortable with the concept. We've never taken any components out-of-process before and we're not very familiar with that kind of programming in this repo.

We'll provide guidelines for each LSP feature as we work on the transition.

dsyme commented 3 years ago

Do you expect this to be limited to diagnostics in the initial iteration? I can see the customer-facing case for pulling other things out-of-proc (reduce VS memory usage) but I'm wondering whether you think of those as necessary from the Roslyn perspective.

Also is there anything coupled here, in the sense forcing this at any particular pace?

tmat commented 3 years ago

The diagnostic reporting is the most important for Roslyn at this point. The other features will follow later on. The timeline is hard to predict.

There are various prerequisites involved. E.g. we need to make sure LSP works on VS Mac and that the LSP protocol has enough features that we can achieve 100% parity with current VS functionality. We are working with other partner teams who use Roslyn APIs as well (TypeScript, Unit Testing, XAML). They will also need to make the switch in order for us to be able to remove the old implementation.

dsyme commented 3 years ago

Ah I see. Yes big long term plan, ok.

tmat commented 3 years ago

F# spent about 2 years aligning with Roslyn to get win-win, among other things so we didn't have to re-implement an analysis engine

BTW, moving F# to LSP does not mean F# can't continue using Roslyn Solution, Projects, Documents, etc. to represent F# source code and projects. Those APIs will continue to be available (they will be running purely out-of-proc though).

We will also provide remote services running in Roslyn's process (where the solution is loaded) that F# LSP server implementation will be able to query for information. The server itself may run in Roslyn's process as well.

Specifically, re diagnostics - the main difference between the current solution crawler infrastructure is following. The solution crawler is a stateful service that implements rather complex logic that gathers diagnostics from the solution (and to which F# plugs into using FSharpDocumentDiagnosticAnalyzer) and also many layers of caching to optimize for memory and time. Pull diagnostics on the other hand are stateless on the server side. The client is in control of prioritization of requests for diagnostics and sends pull requests to the server as it sees fit. E.g. it sends requests for latest diagnostics for the current view of the focused open document very frequently, while it requests diagnostics for closed documents less frequently. The server's only job is to reply with a set of diagnostics for specific document(s) in very functional way (with no side-effects).

FSharpDocumentDiagnosticAnalyzer currently calls to IFSharpDocumentDiagnosticAnalyzer implementation. It seems like this implementation would work in the new model as well. It's mostly the infrastructure how the diagnostics flow that would change around it.

dsyme commented 3 years ago

Thank you for the notes! It helps so much

. The client ...sends requests for latest diagnostics for the current view of the focused open document very frequently, while it requests diagnostics for closed documents less frequently.

Would we have to implement this logic on the client-side, or would VS/Roslyn/someone-else implement this?

tmat commented 3 years ago

The client side logic is owned by VS Platform, VS Mac, and VS Code teams. All client code/UI that Roslyn currently implements in VS to report diagnostics is gonna be removed. F# will not need to implement any client side logic/UI. That's one of the advantages of client-server separation of LSP architecture.

dsyme commented 3 years ago

Sounds great, UI was never our strong point in this repo :)

cartermp commented 3 years ago

@tmat could you help with this?

For clarity and community cohesion we'd need to be able to understand whether the long term trajectory is:

F# already has a very good LSP working via FSAutoComplete (which we helped fund, actually).

DocumentDiagnosticAnalyzer was always a weird abstraction since we're using it as the basis for all diagnostics when it wasn't really intended to be used that way. So I can understand moving to a more "proper" thing.

But there's a huge chasm of difference between just swapping diagnostics over (somehow?) and going all-in on yet another LSP implementation. Which is it that F# must do?

dsyme commented 3 years ago

@cartermp As much as I asked the question, I don't think @tmat can answer that for us, except to repeat things from the VS/Roslyn's perspective

  1. They want us to move diagnostics out of-proc using LSP in the next year or two
  2. The long term trajectory for (most) language services in VS is LSP but there's no specific stick or carrot for that

I think the rest is up to us in the F# community to work out. I'd imagine it will be

"going all-in on yet another LSP implementation"

I think this is not on the cards. We would always choose convergence if this is required.

cartermp commented 3 years ago

I think it would be very helpful if @tmat and team can give an update (here, not internally) about the "shape" of Roslyn LSP so that it's more clear what needs to be done.

As most folks know, the F# tooling is really just a small number of .dlls (FCS, FSharp.Editor and its dependents) loaded in-process in VS. There is no out-of-proc model and various services are not split up into different processes (like Roslyn is). It's more or less an all-or-nothing approach to multiprocessing. This unfortunately complicates the idea of only implementing a tiny subset of LSP just for diagnostics.

Really I only see two reasonable options here for the long term:

  1. The large, challenging work of merging FSharp.Editor with FSAutoComplete and figuring out a good development & distribution plan (I did some of the plan stuff when employed, but this was under the context of the VS Nexus project, and was thus halted when the project failed).
  2. Doing the minimal glue work necessary to just be "in process" with some Roslyn host that itself is an LSP process.

Clearly (1) is a lot more work, but if it's considered more economical or healthy in the long-term then the team should bite the bullet and prepare for the long and somewhat painful process of two-way contributions across this codebase and FSAC and negotiate a development/packaging/distribution that leaves everyone with some degree of dissatisfaction. Adding any ad-hoc implementations of LSP along the way are just going to add more to the pile of regrets.

dsyme commented 3 years ago

@tmat Can you please clarify if you are thinking that F# should prioritise this?

From my perspective I don't see a need to rush - the only benefits to F# customers mentioned above are long term ones, so this can be done long term. Also

@cartermp It may be worth our writing a long running tooling RFC for this (even if most of the issues are coming from the VS side of things) as a way of capturing technical content. An example: Today @vzarytovskii explained to me that the Roslyn workspace implementation in the OmniSharp C# out-of-proc compiler isn't as fully featured as the VisualStudioWorkspace implementation used internally in VS - for example, it can't crack F# project files (if I have the details right). They are somehow looking at making progress on that, yet this is exactly the kind of work where Visual F# Tools (and maybe FsAutoComplete) may well benefit by waiting, rather than doing things early. I know FsAutoCOmplete has its own workspace and project file cracker implementation that cracks F# projects, and doesn't rely on Roslyn - however from the VS perspective we would likely want to use the same cracker as used by C# (whether OmniSharp or Visual Studio).

dsyme commented 3 years ago

@cartermp I have been looking through FsAutoComplete and its LSP implementation and I'm sure we would in some sense want to align with it. However I'd expect some changes for using it in VS, e.g.

  1. replacing the project cracker with some component shared with VS C# per above
  2. using #11976 when complete
  3. likely hosting it differently inside whatever is the standard VS machinery for out-of-proc stuff
  4. end-to-end review, performance, correctness, caching cleanout etc.

2 and 4 is of general use to FsAutoComplete, and possibly 1 as well if it's a public component

cartermp commented 3 years ago

replacing the project cracker with some component shared with VS C# per above

I don't think it's worth speculating on this at this time, since this is the single most important aspect of any shuffling around of IDE infrastructure, and it's going to be highly dependent on whatever gets figured out for C#/Roslyn.

One thing is for sure, the F# team must absolutely not be in the business of owning a project system. It is an enormous amount of work.

baronfel commented 3 years ago

One thing is for sure, the F# team must absolutely not be in the business of owning a project system. It is an enormous amount of work.

As someone who is maintaining the FSAC project system, I could not agree more. I've spent more time in the last 3 months on cracking than I have on FSAC perf/features/stability. It's a black hole of pain and despair.

dsyme commented 3 years ago

I don't think it's worth speculating on this at this time, since this is the single most important aspect of any shuffling around of IDE infrastructure, and it's going to be highly dependent on whatever gets figured out for C#/Roslyn.

Yes, I agree.

psfinaki commented 1 year ago

Okay, so I also stumbled on the poor design of analyzers recently and now I don't mind putting some effort into this task. So what are the current recommendations and guidelines from Roslyn?