support language-server over stdio

[mtoohey31]

This pull request adds support for a hidden --lsstdio flag which, when set, causes the language server to communicate using stdin/stdout (logging to stderr), instead of over the network. This is useful since some editors prefer this method of communication, such as helix. Behaviour should be unchanged when using it over the network, but I don't have a VSCode setup, so I'd appreciate if someone could confirm that I haven't accidentally broken anything on that end.

[TimWhiting]

Originally I had it this way, but found that it kept crashing due to trace print statements from the compiler or language server modules getting interpreted as part of the protocol. Maybe the way you set up the loggers fixed that though? Or do you see crashes with Helix sometimes?

[mtoohey31]

Oh good point, I hadn't considered that. I haven't used this extensively yet, so it's possible those kinds of issues are still present and I just haven't hit them yet. It looks like the trace messages at least are going to stderr because of the logging changes, which prevents them from being interpreted as messages to the client. In my helix log I see:

2024-01-01T11:07:38.829 helix_lsp::transport [ERROR] koka err <- "WithSeverity {getMsg = Starting, getSeverity = Info}\n"
2024-01-01T11:07:38.849 helix_lsp::transport [ERROR] koka err <- "WithSeverity {getMsg = LspProcessingLog (VfsLog (Opening (NormalizedUri 8445809718492171282 \"file:///home/mtoohey/repos/koka-base64/base64.kk\"))), getSeverity = Debug}\n"
2024-01-01T11:07:38.849 helix_lsp::transport [ERROR] koka err <- "WithSeverity {getMsg = LspProcessingLog (LspCore (NewConfig Null)), getSeverity = Debug}\n"
2024-01-01T11:07:38.850 helix_lsp::transport [ERROR] koka err <- "Progress: compile: base64.kk\n"
2024-01-01T11:07:38.860 helix_lsp::transport [ERROR] koka err <- "Progress: compile: /home/mtoohey/repos/koka/lib/std/core.kk\n"
2024-01-01T11:07:38.916 helix_lsp::transport [ERROR] koka err <- "Progress: compile: /home/mtoohey/repos/koka/lib/std/core/types.kk\n"
2024-01-01T11:07:38.921 helix_lsp::transport [ERROR] koka err <- "Progress: check  : std/core/types\n"
2024-01-01T11:07:38.965 helix_lsp::transport [ERROR] koka err <- "Progress: compile: /home/mtoohey/repos/koka/lib/std/core/hnd.kk\n"
2024-01-01T11:07:38.983 helix_lsp::transport [ERROR] koka err <- "Progress: check  : std/core/hnd\n"
2024-01-01T11:07:39.550 helix_lsp::transport [ERROR] koka err <- "Progress: check  : std/core\n"
2024-01-01T11:07:41.323 helix_lsp::transport [ERROR] koka err <- "Progress: check  : base64\n"

Do you know if the compiler is consistent about using trace instead of just printing to stdout?

[TimWhiting]

The src/Lib/Printer.hs module uses stdout directly, I would just search the codebase to see if there are any other references.

[mtoohey31]

Ok, I've addressed a few spots that printed to stdout. There is still some code remaining that prints to stdout, but that's things like Platform.Readline or printers (aside from HtmlTextPrinter) which aren't used in the language server.

It's hard to be 100% confident that I've caught every potential stdout print, but I was able to run the language server with helix using these changes on the entire standard library without it crashing.

[TimWhiting]

This looks pretty good. Let me talk with Daan, and make sure he likes the changes. Maybe we will switch to using this for VSCode as well. It would definitely simplify some things.

I will also need to test this on VSCode for awhile to make sure it doesn't cause a regression there.

[daanx]

Very nice @mtoohey31 -- It looks like I could just merge this and the vscode plugin would keep working as is ? Then people using Helix for example can just pass the -flsstdio if needed. Or did I miss something?

[TimWhiting]

@daanx Yes, I think that is correct. I think we could eventually just move the vscode over to stdio as well, which would simplify both the extension and the server, but that probably requires more testing. Merging this as is (after resolving merge conflicts) should not affect the current vscode solution though.

[mtoohey31]

Very nice @mtoohey31 -- It looks like I could just merge this and the vscode plugin would keep working as is ? Then people using Helix for example can just pass the -flsstdio if needed. Or did I miss something?

Thanks! Yes, like Tim said, this should leave the vscode behaviour unchanged, stdio is only used when the flag is passed. I'll try to get the conflicts resolved soon.

@TimWhiting have you had a chance to test these changes with vscode? If not no worries, I should probably install it and do some testing after I resolve the conflicts either way.

[TimWhiting]

No, I have not had a chance to test stdio for vscode yet.

[mtoohey31]

Oh, I meant just these changes with vscode still using the network to confirm this hasn't broken anything there. Vscode with stdio can wait like you suggested. No worries though, I've rebased things, and tested with helix and vscode, and things appear to be working as expected.

[mtoohey31]

Also, inlay hints appear to be working correctly with Helix too!

[TimWhiting]

There is a bunch of changes coming soon in the dev-modules branch enabling parallel compilation and error reporting which would be highly beneficial to the language server. We'll probably merge this at the same time or right after that branch, and it should be in the next release in a week or two.

[oxalica]

a hidden --lsstdio flag

Could we make it (and maybe also --lsport) visible so it's more discoverable? I'm using neovim with coc.nvim, where we just need to specify the command for language server to make everything work. koka --help only mentions --language-server but it prompts a weird error without any explanation.

$ koka --language-server
does not exist

It's not until I looked into the vscode extension code to find the hidden required argument --lsport.

BTW: coc.nvim also expects communication over stdio. I wrote a wrapper script which redirects TCP into stdio here. It may help before the next release.

[mtoohey31]

In terms of documenting the flags, I think that would be a simple change, probably something like:

diff --git a/src/Compiler/Options.hs b/src/Compiler/Options.hs
index 3a2c1892..cb3f6696 100644
--- a/src/Compiler/Options.hs
+++ b/src/Compiler/Options.hs
@@ -385,6 +385,10 @@ options = (\(xss,yss) -> (concat xss, concat yss)) $ unzip
  , flag   []    ["checkcore"]      (\b f -> f{coreCheck=b})         "check generated core"
  , emptyline

+ , numOption (-1) "port" [] ["lsport"]    (\i f -> f{languageServerPort=i}) "specify language server port"
+ , flag []     ["lsstdio"]     (\b f -> f{languageServerStdio=b}) "use language server over stdio"
+ , emptyline
+
  -- hidden
  , hide $ fflag       ["asan"]      (\b f -> f{asan=b})             "compile with address, undefined, and leak sanitizer"
  , hide $ fflag       ["stdalloc"]  (\b f -> f{useStdAlloc=b})      "use the standard libc allocator"
@@ -403,8 +407,6 @@ options = (\(xss,yss) -> (concat xss, concat yss)) $ unzip
  , hide $ fflag       ["specialize"]  (\b f -> f{optSpecialize=b})    "enable inline specialization"
  , hide $ fflag       ["unroll"]      (\b f -> f{optUnroll=(if b then 1 else 0)}) "enable recursive definition unrolling"
  , hide $ fflag       ["eagerpatbind"] (\b f -> f{optEagerPatBind=b}) "load pattern fields as early as possible"
- , hide $ numOption (-1) "port" [] ["lsport"]    (\i f -> f{languageServerPort=i}) "language Server port"
- , hide $ flag []     ["lsstdio"]     (\b f -> f{languageServerStdio=b}) "language Server stdio"

  -- deprecated
  , hide $ option []    ["cmake"]           (ReqArg cmakeFlag "cmd")        "use <cmd> to invoke cmake"

What are your thoughts on that @TimWhiting? I assume --lsport was hidden originally cause the VSCode plugin was the only thing using it, but if other editors are going to be using the language server, would it be OK to make them visible?

Also, here's my attempt at fixing the error message.

There might be a cleaner way to do this, and I wasn't sure exactly what the message should be. Should we suggest checking the `--lsport` and `--lsstdio` flags? ```diff diff --git a/src/Main/langserver/LanguageServer/Run.hs b/src/Main/langserver/LanguageServer/Run.hs index ca87099e..779e2764 100644 --- a/src/Main/langserver/LanguageServer/Run.hs +++ b/src/Main/langserver/LanguageServer/Run.hs @@ -14,7 +14,7 @@ {-# LANGUAGE DataKinds #-} module LanguageServer.Run (runLanguageServer) where -import System.Exit ( exitFailure ) +import System.Exit ( exitFailure, die ) import GHC.IO.IOMode (IOMode(ReadWriteMode)) import GHC.Conc (atomically) import GHC.IO.Handle (BufferMode(NoBuffering), hSetBuffering) @@ -39,6 +39,9 @@ import LanguageServer.Handlers ( lspHandlers, ReactorInput(..) ) import LanguageServer.Monad (newLSStateVar, runLSM, LSM, getLSState, LSState (messages, progress), getProgress, updateSignatureContext, SignatureContext(..)) import Compiler.Options (Flags (languageServerPort, languageServerStdio)) import Debug.Trace (trace) +import Control.Exception (try) +import Control.Exception.Base (throw) +import System.IO.Error (isDoesNotExistError) runLanguageServer :: Flags -> [FilePath] -> IO () runLanguageServer flags files = do @@ -52,10 +55,16 @@ runLanguageServer flags files = do hSetBuffering stdin NoBuffering runLanguageServerWithHandles stdin stdout -- Connect to localhost on the port given by the client - else connect "127.0.0.1" (show $ languageServerPort flags) (\(socket, _) -> do + else do + res <- try (connect "127.0.0.1" (show $ languageServerPort flags) (\(socket, _) -> do -- Create a handle to the client from the socket handle <- socketToHandle socket ReadWriteMode - runLanguageServerWithHandles handle handle) + runLanguageServerWithHandles handle handle)) :: IO (Either IOError ()) + case res of + Left ex | isDoesNotExistError ex -> die $ "nothing was listening on port " ++ + show (languageServerPort flags) + Left ex -> throw ex + Right _ -> return () where useStdio = languageServerStdio flags runLanguageServerWithHandles inHandle outHandle = do ```

[TimWhiting]

What are your thoughts on that @TimWhiting? I assume --lsport was hidden originally cause the VSCode plugin was the only thing using it, but if other editors are going to be using the language server, would it be OK to make them visible?

I originally had it visible, and Daan made it hidden I think. @daanx did you have a reason? I think we at least want to hide or remove it on the executable that doesn't include language server support, (i.e. src/Main/plain..), but I think it would be fine to show it in the other configuration.

Also, here's my attempt at fixing the error message.

That looks great!

[TimWhiting]

Hey @mtoohey31 can you merge dev into your branch or rebase your branch on dev? Compiler/Options.hs got moved to Compile/Options.hs, and the whole compilation pipeline changed to be more parallel. You should notice big speed improvements on larger projects, and when compiling the std library.

Also will you commit your changes to the error message when the port is not available?

[TimWhiting]

a hidden --lsstdio flag

Could we make it (and maybe also --lsport) visible so it's more discoverable? I'm using neovim with coc.nvim, where we just need to specify the command for language server to make everything work. koka --help only mentions --language-server but it prompts a weird error without any explanation.

$ koka --language-server
does not exist

It's not until I looked into the vscode extension code to find the hidden required argument --lsport.

BTW: coc.nvim also expects communication over stdio. I wrote a wrapper script which redirects TCP into stdio here. It may help before the next release.

What about just adding additional documentation / information about the required flags on the --language-server argument, instead of making the additional flags visible. Thinking about it some more, I think it is preferable to not have the flags cluttering up the help information because no one except editor extension authors should have to deal with these flags. @oxalica @mtoohey31

[daanx]

Hi @mtoohey31 -- awesome PR, I would like to integrate it. I pushed a major update today to dev with a much better build system that will hopefully also benefit the language server (it is much more parallel and efficient). Can you try to rebase of this since some modules moved around. I am hoping to do a fresh release soon and it would be great if this can be part of it so we can see if it starts working with other editors. Thanks again!

[mtoohey31]

Sure, I'll get started on rebasing things now, and I'll include the changes for the error message when the port isn't available.

Have we reached a conclusion on what we'd like to do about documenting --lsport and --lsstdio? It's fine with me if we don't do it in this PR. For now I won't include any changes related to that.

[mtoohey31]

Ok, should be good to go now. The language server definitely feels much faster after integrating those changes!

[TimWhiting]

Thanks! I was able to test and get it to work in vscode as well. Trace statements still were not going to stderr, due to the internal trace library that formatted everything as a Doc and used a printer that printed to stdout. I committed some changes to transition the vscode extension to this protocol (since it has a simpler setup), while still handling older versions of the compiler that work with the socket protocol. Once we can remove this from the vscode extension it will make it a lot more clean!

[daanx]

Wow you guys are fast :-) awesome. I will check it out tomorrow.

• I'll document the new flags -- the idea is really to only hide flags that are needed for developers but otherwise it is better to expose this
• Let's try to use stdio always
• Glad to hear the new build feels faster :-) It is often 5 to 10x faster on larger builds and for the language server it only runs the typechecker further speeding up things. (there is still a bit more we can do later on)
• Let me know if you find bugs -- I had an error diagnostic to stick yesterday and I couldn't get rid of it unless I restarted vscode. I haven't been able to repro though...

Thanks again!