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

Turn HLS-wrapper into an LSP Server #2960

Merged
merged 10 commits into from Jun 26, 2022

Conversation

smatting
Copy link
Contributor

@smatting smatting commented Jun 15, 2022

Introduces better error handling to HLS-wrapper to show LSP clients
dedicated error messages.
This should help users understand why their Language Server isn't
starting.

Closes #2589

Note: This is continuation of @fendor 's PR #2591 developed with his help at Zurihac with notable additions:

  • runLanuageServer has been refactored to act as an entrypoint for both HLS LSP and the HLS-wrapper LSP. This shoud be true refactoring for the HLS LSP call site
  • The HLS-wrapper LSP contains only the bare minimim setup and handlers
  • some changes to the wording of error messages

I've manually tested the happy path (wrapper calls hls) and a error case in emacs, where restarting worked.

@@ -30,7 +31,8 @@ data ReactorMessage
| ReactorRequest SomeLspId (IO ()) (ResponseError -> IO ())

type ReactorChan = Chan ReactorMessage
type ServerM c = ReaderT (ReactorChan, IdeState) (LspM c)
newtype ServerM c a = ServerM { unServerM :: ReaderT (ReactorChan, IdeState) (LspM c) a }
Copy link
Contributor Author

@smatting smatting Jun 15, 2022

Choose a reason for hiding this comment

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

I couldn't get the call sites of runLanguageServer to compile without this refactoring: the compiler seemed to infer the wrong kind for implicit type variable a when using a type alias for ServerM.

ghcide/src/Development/IDE/Main.hs Outdated Show resolved Hide resolved
@fendor fendor self-requested a review June 16, 2022 09:35
@fendor fendor mentioned this pull request Jun 16, 2022
3 tasks
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.

Looks good to me!

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for pushing this through!

@wz1000
Copy link
Collaborator

wz1000 commented Jun 20, 2022

Is it still possible to use ghcide/hls without the wrapper?

@fendor
Copy link
Collaborator

fendor commented Jun 20, 2022

@wz1000 of course, this is just in the case that the wrapper encounters an error and wants to display it to the user.

@wz1000
Copy link
Collaborator

wz1000 commented Jun 20, 2022

I don't quite understand how LSP state (eg config, VFS state) updates which could potentially be intercepted by the wrapper are communicated to the main HLS process.

@fendor
Copy link
Collaborator

fendor commented Jun 20, 2022

HLS-wrapper only activates the LSP mode if there was a breaking start-up issue. For example, when we can't figure out the GHC version, consequentially can't start the main HLS process. The only way forward is still to restart HLS and the wrapper, but there is a UI pop-up that tells you this instead of silently not starting, in combination with a "Restart" button that may be displayed to the user.

@smatting
Copy link
Contributor Author

I don't understand what test is failing and why. There were some merge conflicts in e7dee82 but it was only code blocks that have been moved by this PR.

@fendor fendor added the merge me Label to trigger pull request merge label Jun 26, 2022
@mergify mergify bot merged commit cdc8f78 into haskell:master Jun 26, 2022
hololeap pushed a commit to hololeap/haskell-language-server that referenced this pull request Aug 26, 2022
* Make wrapper a LSP on failure

* Fix incorrect imports

* revert import block for smaller diff

* add missing imports

* Fix: callProcess on win32 machines not called

* import container only on win32

* add missing liftIO

Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
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.

Turn HLS-Wrapper into an LSP Server
4 participants