Skip to content

Conversation

@brettfo
Copy link
Member

@brettfo brettfo commented Jun 5, 2019

Marked [WIP] so I can verify signed builds and VS insertion before merging to master. Verified sign check. N.b., insertion will require adding a sign exclusion for Nerdbank.Streams.dll, but that won't be an issue until this actually ships as part of an official VSIX (e.g., not in release/dev16.2 because these changes are explicitly going to be blocked there.)

This allows VS to consume the out-of-process FSharp.Compiler.LanguageServer and threads through the appropriate options. As it currently stands the default behavior for everything remains the same, but it does enable the following scenario:

Consume the regular QuickInfo:

image

Or check the option Text Editor -> F# -> Advanced -> (Preview) Use out of process language server -> Text hover to instead have FSharp.Compiler.LanguageServer provide the LSP equivalent:

image

image

The process going forward to add new LSP features to VS will be to:

  • Add the appropriate method(s) in src/fsharp/FSharp.Compiler.LanguageServer/Methods.fs
  • Indicate that our LSP supports that feature by updating the ServerCapabilities record in src/fsharp/FSharp.Compiler.LanguageServer/LspTypes.fs
  • Add a new option:
    • Add a flag to the Options record in src/fsharp/FSharp.Compiler.LanguageServer/LspExternalAccess.fs.
    • Add a flag to the AdvancedOptions record in vsintegration/src/FSharp.Editor/Options/EditorOptions.fs
    • Add the check box to vsintegration/src/FSharp.UIResources/AdvancedOptionsControl.xaml
  • Honor those options in:
    • The language server (e.g., the Hover method in src/fsharp/FSharp.Compiler.LanguageServer/TextDocument.fs returns None if the option is not enabled.)
    • The VS feature (e.g., the method GetQuickInfoItemAsync in vsintegration/src/FSharp.Editor/QuickInfo/QuickInfoProvider.fs returns null if the preview option is enabled.)

Next steps:

  • Implement what makes sense in the external LSP, and leave in-process (usually in FSharp.Editor.dll) what makes sense. Not everything belongs out-of-process, and one can argue that QuickInfo/textDocument/hover is one of those things that's better in-proc, but it served as an easy demonstration.
  • Once this is in I plan on reverting in release/dev16.2 just the UI changes in AdvancedOptionsControl.xaml and removing the [<Export>] attribute from FSharpLanguageClient.fs because I think we need much more runway of testing this in nightly builds. I added the variable $(IncludeVsLanguageServer) in eng/targets/Settings.props which will be left as true in master and changed to false in release/dev16.2. This will prevent the extra complexity from shipping out of that branch, but makes for an easy way to re-enable it if/when appropriate.

@brettfo brettfo changed the title [WIP] enable preview LSP support [WIP] enable preview LSP support in VS Jun 5, 2019
@brettfo brettfo requested review from TIHan and cartermp June 6, 2019 01:18
@brettfo brettfo changed the title [WIP] enable preview LSP support in VS enable preview LSP support in VS Jun 10, 2019
@brettfo
Copy link
Member Author

brettfo commented Jun 10, 2019

Tagging @cartermp for his thoughts.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

What's the overall goal of this?

It would also help to understand the overall architectural strategy and why we can't use something like FSAC, which is already fully LSP-compliant.

@cartermp
Copy link
Contributor

Note that we can also use FSharpLu.Json for a smaller DU payload:https://www.nuget.org/packages/Microsoft.FSharpLu.Json/

@brettfo
Copy link
Member Author

brettfo commented Jun 10, 2019

What's the overall goal of this?

Two answers depending on what you're asking:

  • LSP: To have an officially-supported LSP that's tightly tied to a specific version (e.g., from the same sources/build) of the compiler and FSharp.Core, instead of going through FSharp.Compiler.Service. Eventually I'd like for this LSP to consume @TIHan's Compilation work in [WIP] [Prototype] Compilation Model #6947.
  • VS: A convenient way to test this LSP implementation and a potential start to enable out-of-proc work in VS to free up some memory, potentially by adding LSP-like calls for project system work like file ordering.

Ultimately I'm less concerned about the VS work right now, but eventually we'll need a supported method for remote F# editor stuff.

@brettfo
Copy link
Member Author

brettfo commented Jun 10, 2019

FSharpLu.Json

So far we only need two small converters, so I'd rather consume what's in this PR to keep us from requiring a signing and symbol archiving exemption. If we end up duplicating a lot of work then we can re-visit the VS exemption issue.

@cartermp
Copy link
Contributor

Sounds good. I also talked to @TIHan about this and it makes sense. We'll want the testbed for rapid iteration.

@brettfo
Copy link
Member Author

brettfo commented Jun 11, 2019

Will rebase to re-merge and re-test.

brettfo added 2 commits June 11, 2019 10:05
Includes plumbing and a stub for `textDocument/hover` (e.g., QuickInfo).
@brettfo brettfo merged commit a4cbfe1 into master Jun 11, 2019
@brettfo brettfo deleted the hover branch June 11, 2019 20:45
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* enable preview LSP support

Includes plumbing and a stub for `textDocument/hover` (e.g., QuickInfo).

* enable easy exclusion of the language server from VS components

* Update src/fsharp/FSharp.Compiler.LanguageServer/TextDocument.fs

Co-Authored-By: Phillip Carter <[email protected]>
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.

4 participants