Turn HLS-wrapper into an LSP Server #2591

fendor · 2022-01-15T14:33:56Z

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

Sends Request to LSP client on failure
Can be restarted
Documentation for Wrapper LSP

fendor · 2022-01-15T14:34:32Z

I was unable to write tests for this... so only a POC image to show I tested it at least locally.

fendor · 2022-01-15T14:35:18Z

Another noteable improvement: the logs are better structured now:

Consulting the cradle to get project GHC version...
*** Startup ERROR
Failed to find the GHC version of this Cabal project.
Error when calling cabal --builddir=/home/hugin/.cache/hie-bios/dist-hls-036ebb56a4cc135651b7510a90f3136d v2-exec --with-compiler /home/hugin/.cache/hie-bios/wrapper-13a09b18ea883dd377d59db5e821a86b ghc -v0 -- --numeric-version

cabal: Could not resolve dependencies:
[__0] trying: ghcide-1.5.0.1 (user goal)
[__1] rejecting: ghcide:!test (constraint from config file, command line flag,
or user target requires opposite flag selection)
[__1] trying: ghcide:*test
[__2] trying: tasty-rerun-1.1.18 (dependency of ghcide *test)
[__3] trying: tagged-0.8.6.1 (dependency of tasty-rerun)
[__4] trying: template-haskell-2.18.0.0/installed-2.18.0.0 (dependency of
tagged)
[__5] trying: ghc-typelits-knownnat-0.7.6 (dependency of ghcide *test)
[__6] trying: ghc-typelits-natnormalise-0.7.6 (dependency of
ghc-typelits-knownnat)
[__7] next goal: base (dependency of ghcide)
[__7] rejecting: base-4.16.0.0/installed-4.16.0.0 (conflict:
ghc-typelits-natnormalise => ghc-bignum>=1.0 && <1.1, base =>
ghc-bignum==1.2/installed-1.2)
[__7] rejecting: base-4.16.0.0, base-4.15.1.0, base-4.15.0.0, base-4.14.3.0,
base-4.14.2.0, base-4.14.1.0, base-4.14.0.0, base-4.13.0.0, base-4.12.0.0,
base-4.11.1.0, base-4.11.0.0, base-4.10.1.0, base-4.10.0.0, base-4.9.1.0,
base-4.9.0.0, base-4.8.2.0, base-4.8.1.0, base-4.8.0.0, base-4.7.0.2,
base-4.7.0.1, base-4.7.0.0, base-4.6.0.1, base-4.6.0.0, base-4.5.1.0,
base-4.5.0.0, base-4.4.1.0, base-4.4.0.0, base-4.3.1.0, base-4.3.0.0,
base-4.2.0.2, base-4.2.0.1, base-4.2.0.0, base-4.1.0.0, base-4.0.0.0,
base-3.0.3.2, base-3.0.3.1 (constraint from non-upgradeable package requires
installed instance)
[__7] fail (backjumping, conflict set: base, ghc-typelits-natnormalise,
ghcide)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: base, ghc-typelits-natnormalise,
ghcide, ghc-typelits-knownnat, template-haskell, integer-gmp, tagged,
ghcide:test, tasty-rerun
Try running with --minimize-conflict-set to improve the error message.

exe/Wrapper.hs

jneira · 2022-01-17T07:42:02Z

Converted to draft until the lsp change

Turn HLS-wrapper into a full-blown LSP server, capable of sending requests and handling them appropriately. 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.

fendor

Most stuff is basically copy-pasted from ghcide's runLangaugeServer with all shake stuff stripped out.

fendor · 2022-01-31T20:24:20Z

exe/Wrapper.hs

+
+-- | Version of 'getRuntimeGhcVersion' that throws a 'WrapperSetupError' if we
+-- can't get it, and also checks if run-time tool dependencies are missing.
+getRuntimeGhcVersion' :: Cradle Void -> ExceptT WrapperSetupError IO String


Suggested change

getRuntimeGhcVersion' :: Cradle Void -> ExceptT WrapperSetupError IO String

getRuntimeGhcVersion' :: MonadIO m => Cradle Void -> ExceptT WrapperSetupError m String

Maybe? Probably not worth it, though

fendor · 2022-01-31T20:25:13Z

exe/Wrapper.hs

+prettyError (ToolRequirementMissing toolExe name) _ =
+  "This is a " <> T.pack (show name) <> " Project, but we failed to find \"" <>
+  T.pack toolExe <> "\" on the $PATH"


Maybe this can be a shortened version, and we can dump the Search Path to stderr, too?

Dumping a search path would be hugely beneficial. I know I've been stuck trying to find out what Path is visible to HLS before.

fendor · 2022-01-31T20:25:27Z

exe/Wrapper.hs

+  " project, since we have a none cradle"
+prettyError (NoLanguageServer ghcVersion candidates) _ =
+  "Failed to find a HLS version for GHC " <> T.pack ghcVersion <>
+  "\nExecutable names we failed to find: " <> T.pack (intercalate "," candidates)


This wording feels bad

FailedToObtainGhcVersion and NoneCradleGhcVersion look they could be identical? i.e. something like Failed to find GHC version for project: {name} (unless name implies something else?) and then for NoneCradle keep the same but with (NoneCradle) or some other marker attached?

They are slightly different though, we should improve the error message!

In the case of a none cradle, we can not figure out the GHC version to use, thus can't decide which HLS version we want to launch. The outside world specifically instructed us not to load this project, probably via a hie.yaml

FailedToObtainGhcVersion entails that some command has failed invocation, e.g. cabal v2-exec -- ghc -v0 --numeric-version, e.g. we attempted to load the project, but we couldn't.

Ah I was paying close enough attention I didn't see the extra details added to FailedToObtainGHCVersion...

Either way, the message could be identical and generic. And then the extra information could be appended. The wording just seems clunky is all.

fendor · 2022-01-31T20:25:54Z

exe/WrapperLspMain.hs

+    outH <- Main.argsHandleOut realArguments
+
+    runLanguageServer (Main.argsLspOptions realArguments) inH outH (Main.argsDefaultHlsConfig realArguments) getConfigFromNotification $ \env exitFun -> do
+        -- Send a message to the client to tell about our problems


WIP comment, will be changed

drsooch · 2022-02-02T00:56:07Z

exe/WrapperLspMain.hs

+    whenJust argsCwd IO.setCurrentDirectory
+    dir <- IO.getCurrentDirectory
+    LSP.setupLogger argsLogFile ["hls"]


There were some earlier inconsistencies with pathing when --cwd was supplied.

haskell-language-server/ghcide/exe/Main.hs

Lines 53 to 56 in 47cb213

-- if user uses --cwd option we need to make this path absolute (and set the current directory to it)

argsCwd <- case argsCwd of

Nothing -> IO.getCurrentDirectory

Just root -> IO.setCurrentDirectory root >> IO.getCurrentDirectory

pepeiborra · 2022-02-02T06:44:37Z

exe/WrapperLspMain.hs

+    inH <- Main.argsHandleIn realArguments
+    outH <- Main.argsHandleOut realArguments
+
+    runLanguageServer (Main.argsLspOptions realArguments) inH outH (Main.argsDefaultHlsConfig realArguments) getConfigFromNotification $ \env exitFun -> do


Duplicating runLanguageServer here is not great. This code should either:

call the real runLanguageServer (you can always parameterise it even more)

fork it (as done here) and clean up.

In other words, I'd prefer if you could clean this up or just call the original

calling the original is a bit tricky, as the original runLanguageServer handles the initialisation request and sends a response which we probably don't want to... If we don't care about that (when I think about it, we probably don't have to), we can re-use runLanguageServer with basically a startup continuation.

I think we can re-use the code, we would open a shake database, etc... but that doesn't matter too much, right?

The shake database thing is just a legacy concept - we are not using Shake anymore so all it is now is a bunch of concurrent mutable hash maps.

pepeiborra · 2022-02-02T06:45:36Z

exe/WrapperLspMain.hs

+    -> (Config -> Value -> Either T.Text Config)
+    -> (LSP.LanguageContextEnv Config -> IO () -> IO ())
+    -> IO ()
+runLanguageServer options inH outH defaultConfig onConfigurationChange onRun = do


Why all these arguments?

Suggested change

runLanguageServer options inH outH defaultConfig onConfigurationChange onRun = do

runLanguageServer onRun = do

pepeiborra · 2022-02-02T06:46:03Z

exe/WrapperLspMain.hs

+    clientMsgChan :: Chan ReactorMessage <- newChan
+
+    let asyncHandlers = mconcat
+          [ cancelHandler cancelRequest


Why handle cancellation?

michaelpj · 2022-02-02T16:01:42Z

I think we have to think up some way to do this without the massive duplication. I don't think we can justify all that just for some better reporting at startup :/

Another thing: how will running a whole LSP server here look to the client? Will we e.g. respond to their initialize request? But then what happens when the real HLS starts up, it will expect to get sent initialize also.

fendor · 2022-02-02T16:27:24Z

I think we have to think up some way to do this without the massive duplication.

True, but I think we can reduce it.

Will we e.g. respond to their initialize request?

No we do not.

But then what happens when the real HLS starts up, it will expect to get sent initialize also

Once the HLS-wrapper LSP server starts, the user has only one possible interaction: press restart.
Restarting exits the LSP server, and then VSCode automatically tries to launch the wrapper again, which performs the checks again, and if it succeeds, launches HLS again.

I just realise, we can't rely on that every lsp-client has the same behaviour as VSCode (e.g. try to restart the language server if it exits), which makes it a bit trickier... Afaict, the LSP server always parses the initialisation request (unless we can introduce some way to avoid that).

pepeiborra · 2022-02-02T19:23:56Z

I just realise, we can't rely on that every lsp-client has the same behaviour as VSCode (e.g. try to restart the language server if it exits), which makes it a bit trickier... Afaict, the LSP server always parses the initialisation request (unless we can introduce some way to avoid that).

I wouldn't worry too much about this. If the LSP client doesn't try to restart, we have already notified the user that something was wrong and given them a strong hint about what to do

michaelpj · 2022-02-14T17:03:36Z

I had a possibly insane idea. What if:

We taught all the haskell-language-server binaries a wrapper subcommand that causes them to act as a wrapper.
We build the "wrapper" by just building the normal executable, but statically and with all the plugin flags turned off to keep its footprint small.

Then we can hopefully avoid the duplication: because we'll reuse the LSP setup that we normally do. And we can probably inherit e.g. arguments for log files etc.

hasufell · 2022-02-14T17:14:03Z

I had a possibly insane idea. What if:

We taught all the haskell-language-server binaries a wrapper subcommand that causes them to act as a wrapper.

We build the "wrapper" by just building the normal executable, but statically and with all the plugin flags turned off to keep its footprint small.

Then we can hopefully avoid the duplication: because we'll reuse the LSP setup that we normally do. And we can probably inherit e.g. arguments for log files etc.

That requires either:

users to change haskell-language-server-wrapper to haskell-language-server-wrapper --wrapper in their LSP client config, or
a wrapper shell script that invokes it with --wrapper

michaelpj · 2022-02-14T17:24:10Z

That requires either...

True. But having a wrapper script that just calls minimal-haskell-language-server --wrapper doesn't seem so bad.

pepeiborra · 2022-02-14T22:50:43Z

I had a possibly insane idea. What if:

We taught all the haskell-language-server binaries a wrapper subcommand that causes them to act as a wrapper.

We build the "wrapper" by just building the normal executable, but statically and with all the plugin flags turned off to keep its footprint small.

Then we can hopefully avoid the duplication:

Initially I really liked this idea, but then I realised that the main HLS binary is just calling Development.Ide.Main.defaultMain with a list of plugins and a list of custom commands.

So most of the duplication is already abstracted, and what's left is due to the need to inline runLanguageServer in order to prevent the wrapper from handling the Initialize request. This problem doesn't go away, the duplication would just move to the custom wrapper subcommand right?

@fendor did you consider my suggestion in #2591 (comment) yet?

michaelpj · 2022-02-14T23:27:17Z

So most of the duplication is already abstracted, and what's left is due to the need to inline runLanguageServer in order to prevent the wrapper from handling the Initialize request.

Hmm, good point. So maybe we just need to paramterize that by something to say "don't really start up" 🤔

michaelpj · 2022-02-14T23:31:44Z

... which is what you suggested already.

fendor · 2022-02-15T07:44:41Z

did you consider my suggestion in #2591 (comment) yet?

Yes, I think it is a good idea to parameterise runLanguageServer more, but I didn't have time to implement that change.

hasufell · 2022-02-15T18:02:00Z

I had a possibly insane idea. What if:

We taught all the haskell-language-server binaries a wrapper subcommand that causes them to act as a wrapper.

We build the "wrapper" by just building the normal executable, but statically and with all the plugin flags turned off to keep its footprint small.

Then we can hopefully avoid the duplication: because we'll reuse the LSP setup that we normally do. And we can probably inherit e.g. arguments for log files etc.

Actually, instead of adding a subcommand, we can simply re-use the strategy many unix cli programs use: rely on argv[0] like busybox, git etc. In haskell, that's getProgName (with same caveats on windows).

fendor · 2022-06-16T09:35:34Z

Closed in favour of #2960

fendor commented Jan 15, 2022

View reviewed changes

exe/Wrapper.hs Outdated Show resolved Hide resolved

jneira marked this pull request as draft January 17, 2022 07:41

fendor force-pushed the feature/hls-wrapper-messages branch 2 times, most recently from 758dc37 to 0a5dc78 Compare January 18, 2022 11:49

fendor force-pushed the feature/hls-wrapper-messages branch 2 times, most recently from 2280b6d to ab88d2b Compare January 31, 2022 20:16

fendor changed the title ~~Allow HLS-wrapper to send LSP messages~~ Turn HLS-wrapper into a LSP Server Jan 31, 2022

fendor changed the title ~~Turn HLS-wrapper into a LSP Server~~ Turn HLS-wrapper into an LSP Server Jan 31, 2022

fendor requested review from pepeiborra, jneira and michaelpj January 31, 2022 20:17

fendor force-pushed the feature/hls-wrapper-messages branch from ab88d2b to 9304319 Compare January 31, 2022 20:18

fendor commented Jan 31, 2022

View reviewed changes

fendor self-assigned this Jan 31, 2022

fendor marked this pull request as ready for review January 31, 2022 22:45

drsooch reviewed Feb 2, 2022

View reviewed changes

pepeiborra reviewed Feb 2, 2022

View reviewed changes

fendor mentioned this pull request Mar 14, 2022

Consider user installed HLSes (e.g. via ghcup compile) haskell/vscode-haskell#543

Merged

smatting mentioned this pull request Jun 15, 2022

Turn HLS-wrapper into an LSP Server #2960

Merged

fendor closed this Jun 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn HLS-wrapper into an LSP Server #2591

Turn HLS-wrapper into an LSP Server #2591

fendor commented Jan 15, 2022 •

edited

Loading

fendor commented Jan 15, 2022

fendor commented Jan 15, 2022

jneira commented Jan 17, 2022

fendor left a comment

fendor Jan 31, 2022

fendor Jan 31, 2022

drsooch Feb 2, 2022

fendor Jan 31, 2022

drsooch Feb 2, 2022

fendor Feb 2, 2022

drsooch Feb 2, 2022 •

edited

Loading

fendor Jan 31, 2022

drsooch Feb 2, 2022

pepeiborra Feb 2, 2022

fendor Feb 2, 2022

pepeiborra Feb 2, 2022

pepeiborra Feb 2, 2022

pepeiborra Feb 2, 2022

michaelpj commented Feb 2, 2022

fendor commented Feb 2, 2022

pepeiborra commented Feb 2, 2022

michaelpj commented Feb 14, 2022

hasufell commented Feb 14, 2022

michaelpj commented Feb 14, 2022

pepeiborra commented Feb 14, 2022

michaelpj commented Feb 14, 2022

michaelpj commented Feb 14, 2022

fendor commented Feb 15, 2022

hasufell commented Feb 15, 2022

fendor commented Jun 16, 2022

	getRuntimeGhcVersion' :: Cradle Void -> ExceptT WrapperSetupError IO String
	getRuntimeGhcVersion' :: MonadIO m => Cradle Void -> ExceptT WrapperSetupError m String

	-- if user uses --cwd option we need to make this path absolute (and set the current directory to it)
	argsCwd <- case argsCwd of
	Nothing -> IO.getCurrentDirectory
	Just root -> IO.setCurrentDirectory root >> IO.getCurrentDirectory

	runLanguageServer options inH outH defaultConfig onConfigurationChange onRun = do
	runLanguageServer onRun = do

Turn HLS-wrapper into an LSP Server #2591

Turn HLS-wrapper into an LSP Server #2591

Conversation

fendor commented Jan 15, 2022 • edited Loading

fendor commented Jan 15, 2022

fendor commented Jan 15, 2022

jneira commented Jan 17, 2022

fendor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drsooch Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Feb 2, 2022

fendor commented Feb 2, 2022

pepeiborra commented Feb 2, 2022

michaelpj commented Feb 14, 2022

hasufell commented Feb 14, 2022

michaelpj commented Feb 14, 2022

pepeiborra commented Feb 14, 2022

michaelpj commented Feb 14, 2022

michaelpj commented Feb 14, 2022

fendor commented Feb 15, 2022

hasufell commented Feb 15, 2022

fendor commented Jun 16, 2022

fendor commented Jan 15, 2022 •

edited

Loading

drsooch Feb 2, 2022 •

edited

Loading