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 F# to LSP pull diagnostics #11969

Open
tmat opened this issue Aug 13, 2021 · 25 comments
Open

Migrate F# to LSP pull diagnostics #11969

tmat opened this issue Aug 13, 2021 · 25 comments
Labels
Area-VS VS support for F# not covered elsewhere Feature Request
Milestone

Comments

@tmat
Copy link
Member

tmat commented Aug 13, 2021

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

The goal is to replace this with LSP pull diagnostics.

@dsyme
Copy link
Contributor

dsyme commented Aug 13, 2021

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

@tmat
Copy link
Member Author

tmat commented Aug 13, 2021

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

@dsyme
Copy link
Contributor

dsyme commented Aug 13, 2021

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
Copy link
Member Author

tmat commented Aug 13, 2021

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
Copy link
Contributor

dsyme commented Aug 13, 2021

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
Copy link
Member Author

tmat commented Aug 13, 2021

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
Copy link
Contributor

dsyme commented Aug 13, 2021

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
Copy link
Contributor

dsyme commented Aug 13, 2021

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
Copy link
Member Author

tmat commented Aug 13, 2021

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
Copy link
Contributor

dsyme commented Aug 14, 2021

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
Copy link
Member Author

tmat commented Aug 14, 2021

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
Copy link
Contributor

dsyme commented Aug 14, 2021

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

@tmat
Copy link
Member Author

tmat commented Aug 14, 2021

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
Copy link
Contributor

dsyme commented Aug 14, 2021

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
Copy link
Member Author

tmat commented Aug 14, 2021

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
Copy link
Contributor

dsyme commented Aug 14, 2021

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

@cartermp
Copy link
Contributor

cartermp commented Aug 16, 2021

@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
Copy link
Contributor

dsyme commented Aug 16, 2021

@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

  • in the next 1-2 years take diagnostics out of proc in VS via an extremely minimal LSP implementation private to VS

  • in 3-5 years time converge with one LSP implementation for F# (though some services may stay in-proc in VS for perf)

"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
Copy link
Contributor

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
Copy link
Contributor

dsyme commented Aug 17, 2021

@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

  • This is a large cost for a small team, especially if you are proposing we should be the first language among those listed to address this

  • Finishing Plan: Modernizing F# Analysis #11976 is an absolute pre-requisite for this. Otherwise, the out-of-proc service will have memory leaks (or it will be hard to reason about its memory usage) and correctness problems.

  • F# has alignment issues to consider with the existing F# LSP implementation in FsAutoComplete (per @cartermp above)

@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
Copy link
Contributor

dsyme commented Aug 17, 2021

@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 Plan: Modernizing F# Analysis #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
Copy link
Contributor

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
Copy link
Member

baronfel commented Aug 17, 2021

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
Copy link
Contributor

dsyme commented Aug 17, 2021

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.

@dsyme dsyme added Area-VS VS support for F# not covered elsewhere and removed Area-LangService-API labels Apr 5, 2022
@psfinaki
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VS VS support for F# not covered elsewhere Feature Request
Projects
Status: New
Development

No branches or pull requests

8 participants