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

Upgrade to lsp-1.0 #1284

Merged
merged 35 commits into from
Feb 14, 2021
Merged

Upgrade to lsp-1.0 #1284

merged 35 commits into from
Feb 14, 2021

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Feb 1, 2021

This involved a signficant rework of all LSP interaction and hls-plugin-api

TODO

  • Release new versions of lsp,lsp-test
  • Make lsp-test automatically respond to "progress create" and "apply edit" requests.
  • Port GHCIDE tests
  • Port HLS plugins
  • Restore executeCommand
  • Port HLS tests
  • Restore tracing/telemetry
  • Fix warnings/hlints

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 1, 2021

/cc @bubba

@wz1000 wz1000 force-pushed the lsp-1.0 branch 5 times, most recently from ecf402c to 8108dbc Compare February 1, 2021 12:23
@wz1000 wz1000 marked this pull request as draft February 1, 2021 12:29
Copy link
Collaborator

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

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

Just did a quick scan, but so far makes a lot of sense. Did ghcide use a reactor style channel beforehand?

-> WithProgressFunc
-> WithIndefiniteProgressFunc
initialise :: Rules ()
-> Maybe (LSP.LanguageContextEnv Config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hiding the LSP functionality into LanguageContextEnv is exactly what I had in mind 👍

ghcide/src/Development/IDE/Core/Shake.hs Show resolved Hide resolved
@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 6, 2021

The major changes introduced by this PR are now pretty much ready, here is a summary:

lsp-1.0

lsp-1.0 brings many improvments to the LSP library, with a much nicer interface and type-safety as well as lots of bug fixes.

For a summary of the changes, see the release notes on https://github.com/alanz/lsp/releases/tag/1.0.0.0

For an example of the concrete differences introduced due to this API, consider the request function from lsp-test.

In the old version, this is its type:

request :: (ToJSON params, FromJSON a) => ClientMethod -> params -> Session (ResponseMessage a) 

To use this function, the user had to guess the correct type of params and a(the shape of the response) themselves, and ensure that it matched up with the ClientMethod in the first parameter.

Using lsp-1.0, we can give the following type to request:

request :: SClientMethod m -> MessageParams m -> Session (ResponseMessage m)

MessageParams is a type family that correctly resolves to the
type of the parameters that need to be supplied to the request, depending on the
ClientMethod that was specified as the first argument

ResponseMessage is also parameterized over the LSP method, and is guaranteed
contain a response of the correct "shape".

Changes in hls-plugin-api

Overall, the changes serve to unify the handling

The PluginDescriptor record doesn't need to have a seperate field
for each type of request that can be responded to by plugins.
Instead, we use a PluginHandlers type that contains a "dependent
map"
from LSP request
methods to the handlers for those methods.

To add a handler for a message type m into this map, it must implement the PluginMethod typeclass. This typeclass specifies two things:

  1. How do we combine multiple responses from different plugins into a single response?
  2. Given a plugin id, how do we check if this plugin is enabled for this method type in the config

To extend 'hls-plugin-api' to support a new kind of request, all that is needed is to
supply an instance for PluginMethod for the new request method.

For instance, to support "go to definition" plugins, all we need to do is supply

instance PluginMethod TextDocumentDefinition where
  ...

The combineResponses method of this class now implements much of the logic that previously lived in ghcide as part of Development.IDE.Plugin.HLS to combine multiple plugin responses.

To define plugin handlers, plugins can use the monoid instance of PluginHandlers along with mkPluginHandler:

mkPluginHandler
  :: PluginMethod m
  => SClientMethod m
  -> PluginMethodHandler ideState m
  -> PluginHandlers ideState

type PluginMethodHandler a m = a -> PluginId -> MessageParams m -> LspM Config (Either ResponseError (ResponseResult m))

An example:

descriptor :: PluginId -> PluginDescriptor IdeState
descriptor plId = (defaultPluginDescriptor plId)
  { pluginRules = exampleRules
  , pluginCommands = [PluginCommand "codelens.todo" "example adding" addTodoCmd]
  , pluginHandlers = mkPluginHandler STextDocumentCodeAction     codeAction
                  <> mkPluginHandler STextDocumentCodeLens       codeLens
                  <> mkPluginHandler STextDocumentHover          hover
                  <> mkPluginHandler STextDocumentDocumentSymbol symbols
                  <> mkPluginHandler STextDocumentCompletion     completion
  }

Changes in ghcide

Most of the code that lived in ghcide for interacting with the client had to be re-written, along with changes to the ghcide plugin API.

In particular, the WithMessage datatype was removed, and we can add handlers for different LSP methods using the Handler type and its
Monoid instance: https://hackage.haskell.org/package/lsp-1.0.0.1/docs/Language-LSP-Server.html#t:Handlers

We alias the LSP requestHandler and notificationHandler functions to provide
new versions that don't block the lsp message loop, and simply forward the
computation to a channel, where it will be picked up by a ghcide worker thread,
which can decide to execute the computation synchronously (for notifications),
asynchronously (for requests) or not at all (for cancelled requests).

requestHandler
  :: forall (m :: Method FromClient Request) c.
     SMethod m
  -> (IdeState -> MessageParams m -> LspM c (Either ResponseError (ResponseResult m)))
  -> Handlers (ServerM c)

notificationHandler
  :: forall (m :: Method FromClient Notification) c.
     SMethod m
  -> (IdeState -> MessageParams m -> LspM c ())
  -> Handlers (ServerM c)

Again, these handlers can be composed using (<>) or mconcat

Code Actions

Code actions used to return a Maybe Value response, as well as a
WorkspaceApplyEdit that would be send to the client as a new request. However,
now since all the handlers live inside the LspM monad, they can simply use
sendRequest to send the apply edit request themselves.

This also allows them to wait for the response to this request, so that the result
of the ExecuteCommandRequest can depend on whether the client successfully applied the edit.
However, right now we just ignore the response to preserve the original behaviour.

@wz1000 wz1000 marked this pull request as ready for review February 6, 2021 13:10
@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 6, 2021

While there is still some work to be done on this, all the major changes are in, so I would appreciate reviews on the general API and direction of the PR

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 6, 2021

Also, if anyone wants to help with porting the HLS testsuite, please feel free to reach out. I believe it would be a good exercise to familiarize yourself with the new lsp API 😛

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 6, 2021

Another (maybe helpful) tip for people reviewing the changes, especially to the notifications: ignore white-space changes, that makes a diff a lot less scary

ghcide/exe/Main.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/Compile.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/FileStore.hs Outdated Show resolved Hide resolved
@jneira jneira mentioned this pull request Feb 10, 2021
35 tasks
@wz1000 wz1000 force-pushed the lsp-1.0 branch 7 times, most recently from da71cd7 to 1adb8cf Compare February 12, 2021 11:12
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

As discussed, let's land this ASAP and deal with bugs/issues later. Dependencies need to be all in Hackage for that though

I plan to build all my future work on top of this branch so please avoid rewriting the history (just squash merge at the end)

@alanz
Copy link
Collaborator

alanz commented Feb 12, 2021

FWIW, I have been running with it for the last few days, and seems solid. Plus my tree-sitter version is based on it, and I have not seen anything dire while investigating behaviour for that.

@wz1000 wz1000 added the merge me Label to trigger pull request merge label Feb 14, 2021
@wz1000 wz1000 mentioned this pull request Feb 14, 2021
7 tasks
@mergify mergify bot merged commit ad0a154 into haskell:master Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants