Implement JSON-RPC and LSP with Monix #164

olafurpg · 2018-01-03T23:38:49Z

This PR gets rid of the dependency on scala-json-rpc and removes languageserver LanguageServer/Connection/MessageReader/.... Only simple LSP case classes remain from the languageserver project in addition to the MessageWriter.

The motivation for this PR is that I have always found it too difficult to introduce support for new message types. Adding support for a feature like workspace/symbol usually took 20 minutes of adding classes to several places, guessing who to serialize/deserialize them, manually test in vscode at least 5 times until it worked (with typically 1-2 sessions ending in silent failure). Another common problem was figuring out how to send error responses instead of showMessage(); DummyResponse(). My patience for this complexity ran out after I spent 50% of the time for #154 figuring out how to initiate a request to the client from the server.

With this PR, adding support for a new message is now as simple as

case class WorkspaceSymbolParams(query: String)
// ...
val workspaceSymbol =
  request[WorkspaceSymbolParams, List[SymbolInformation]]("workspace/symbol")(params => ???)

Responding with an error works like this

val workspaceSymbol =
  requestAsync[WorkspaceSymbolParams, List[SymbolInformation]](
    "workspace/symbol")(params => Task(Left(Response.invalidParams("can't read params"))))

Sending a request to the client works like this

val client: LanguageClient = ???
client.request[MyRequestType, MyResponseType]("method", request): Task[MyResponseType]

The ease of sending requests to the client and first-class support to respond with standard JSON-RPC error codes (without hacks) should simplify the last remaining issue for the v0.1 milestone #161 .

A nice benefit of this refactoring is that we no longer need dummy wrapper types when the response is List[T]. For example, rename, formatting, document highlight. We can do request[Request, List[Response]] since there is no sealed trait LspMessage.

I tried to build as much as possible with monix, using for example Observable[Array[Byte]] instead of InputStream and Task.create to build responses in the LanguageClient.

It's worth noting that after refactoring ScalametaLanguageServer to this new API, almost everything worked on the first run! Completions, scalafmt, scalafix, definition, references, squigglies, workspace symbols, document highlight. Types give you refactoring superpowers. Once we have more experience with this new API, we can consider moving it into a separate lsp4s project, if there is interest.

I left out some changes that can be done in future PRs

s/play-json/circe/ + @JsonCodec (done in Replace play-json with circe #165, merged into this PR )
Better API to turn LanguageServer into a client, to invoke method programmatically for example in tests or through different protocol than stdout/stding
cleaner header parser with more tests, @ShaneDelmore has some prior work in WIP: Migrate monix usage to higher level constructs #160

- fix $/cancelParams - remove scala-json-rpc - remove dead code

olafurpg · 2018-01-03T23:41:15Z

PS. I apologize for the terrible commit log, the best way to review this PR is most likely to look at the entire final diff. There were many failed experiments and I was not diligent at documenting intermediary steps.

Sciss · 2018-01-04T00:03:20Z

Once we have more experience with this new API, we can consider moving it into a separate lsp4s project, if there is interest.

That would be great, especially if it could also be used to write language clients.

xeno-by · 2018-01-04T04:13:57Z

languageserver/src/main/scala/langserver/core/Notifications.scala

+
+  def showMessage(params: ShowMessageParams): Unit
+  final def showMessage(
+      tpe: MessageType,


Looks like an inconsistency with the parameter name of logMessage.

xeno-by · 2018-01-04T04:15:26Z

languageserver/src/main/scala/langserver/messages/Commands.scala

+  capabilities: ClientCapabilities)
+object InitializeParams {
+  implicit val format: OFormat[InitializeParams] = Json.format[InitializeParams]
+}


Is it possible to auto-derive this boilerplate? The necessity to explicitly spell out the serializers for every request/response was one of the biggest annoyances personally to me when I was writing my own simple language server.

Yep, it's going away soon. The next thing I want to work on (unless someone beats me to it) is to replace play-json with circe and use the @JsonCodec macro annotation to make this boilerplate (and other, see Configuration) go away.

Fixed in #165 🎉

I didn't want to change the json library in this PR as it was doing enough already

xeno-by · 2018-01-04T04:15:41Z

languageserver/src/main/scala/langserver/messages/Commands.scala

-sealed trait Response extends Message
-sealed trait ResultResponse extends Response
-
-sealed trait Notification extends Message


xeno-by · 2018-01-04T04:16:35Z

languageserver/src/main/scala/langserver/messages/Commands.scala

+object DocumentSymbolParams {
+  implicit val format: OFormat[DocumentSymbolParams] = Json.format[DocumentSymbolParams]
+}
+case class TextDocumentRenameRequest(params: RenameParams)


How do you figure out which classes to call XxxRequest and which classes to call XxxParams? This is the problem that I faced myself when writing a language server, and I haven't figured out a consistent naming strategy yet.

My understanding is that each Request has (optionally) Params.

The names of the Params classes are taken as-is from the names of the TypeScript interfaces in https://microsoft.github.io/language-server-protocol/specification.

The names of the Requests are essentially transformed from <scope>/<requestType> to <Scope><RequestType>Request, e.g. textDocument/rename -> TextDocumentRenameRequest

If something doesn't follow this rules, I think we should change it.

Ditto, the names should match the interfaces in the lsp spec.

xeno-by · 2018-01-04T04:27:35Z

metaserver/src/main/scala/scala/meta/languageserver/LSPLogger.scala

@@ -21,5 +21,5 @@ class LSPLogger(@BeanProperty var encoder: PatternLayoutEncoder)
 }

 object LSPLogger {
-  var connection: Option[Connection] = None
+  var connection: Option[Notifications] = None


This probably shouldn't be called connection anymore? Same question for other things in the code that are also called connection.

xeno-by · 2018-01-04T04:40:43Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+      shutdown()
+      JsNull
+    }
+    .notification[JsValue]("exit") { _ =>


How hard would it be to implement the spec that says "The server should exit with success code 0 if the shutdown request has been received before; otherwise with error code 1" (https://microsoft.github.io/language-server-protocol/specification#exit).

Easy, we can use an Atomic boolean to determine if shutdown has been received. Done.

xeno-by · 2018-01-04T04:42:04Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+      logger.info("exit(0)")
+      sys.exit(0)
+    }
+    .requestAsync[TextDocumentPositionParams, CompletionList](


What's the difference between request and requestAsync? Looks like the former method simply delegates to the latter?

Yes, the former delegates, I made the distinction to reduce ceremony in adding new handlers. All handlers are currently synchronous except the codeActions handler that is truly asynchronous since it does a workspaceEdit roundtrip.

xeno-by · 2018-01-04T04:55:17Z

metaserver/src/main/scala/scala/meta/languageserver/protocol/BaseProtocolMessage.scala

+    val sb = new java.lang.StringBuilder()
+    header.foreach {
+      case (key, value) =>
+        sb.append(key).append(": ").append(value).append("\n")


xeno-by · 2018-01-04T05:17:58Z

metaserver/src/main/scala/scala/meta/languageserver/providers/DefinitionProvider.scala

@@ -15,13 +15,13 @@ object DefinitionProvider extends LazyLogging {
      uri: Uri,
      position: l.Position,
      tempSourcesDir: AbsolutePath
-  ): DefinitionResult = {
+  ): List[Location] = {


How do you determine what data structures to return from the language server - raw payload, XxxResult or XxxResponse?

Those as specified in https://microsoft.github.io/language-server-protocol/specification

xeno-by · 2018-01-04T05:20:10Z

metaserver/src/main/scala/scala/meta/languageserver/refactoring/OrganizeImports.scala

-        s"Unable to parse TextDocumentIdentifier from $arguments"
-      )
-    )
+    Task {


Why does this have to be a task if the result is returned immediately? Also, if this has to be a task, then why not wrap the computation of result?

I agree, I would either wrap the whole thing in Task or use Task.now if this is synchronous.

gabro · 2018-01-04T09:20:31Z

That would be great, especially if it could also be used to write language clients.

@Sciss lsp4s would be for language servers, but I suppose models could be shared and we could join efforts with @vovapolu, who's already worked on scala-based language clients in https://github.com/ensime/ensime-lsp-clients

Sciss · 2018-01-04T09:33:40Z

@gabro Yes, I think a lot of the types and API, you know from error codes to traits and protocol classes, the rpc implementation etc. could be shared between client and server.

gabro

Impressive work 😮

I've left a few non-blocking comments

gabro · 2018-01-04T10:01:48Z

build.sbt

    libraryDependencies ++= Seq(
-      "com.dhpcs" %% "scala-json-rpc" % "2.0.1",


gabro · 2018-01-04T11:42:38Z

metaserver/src/main/scala/scala/meta/languageserver/providers/DocumentFormattingProvider.scala

@@ -79,14 +78,11 @@ class DocumentFormattingProvider(
      }
    }
    formatResult match {


😇

import cats.syntax.bifunctor._ import cats.instances.either._ formatResult.bimap( Response.invalidParams, formatted => List(TextEdit(fullDocumentRange, formatted)) )

It's a shame the stdlib Either does not have a signature like that, it's so close to fold but different enough

gabro · 2018-01-04T11:50:37Z

metaserver/src/main/scala/scala/meta/languageserver/refactoring/OrganizeImports.scala

-      )
-    )
+    Task {
+      result match {


😇

import cats.syntax.either._ Either.fromOption( result, Left( Response.invalidParams( s"Unable to parse TextDocumentIdentifier from $arguments" ) )

Nice! Only gripe with these APIs is finding the right imports, I just want to type Either.fromOpt<TAB> and have the editor insert the import. Only if we had a language server to do this for us 🤔

We've started using cats, but so far it was a sub-dependency of circe and monix.

- Report message in document formatting provider since invalid params are not reported in the vscode ui. - Use cats syntax for either - Use move executeCommand to separate method

olafurpg

Thanks everyone for the swift review on this huge PR. Very nice to get this in asap to avoid conflicts.

I took the liberty to merge #165 into this PR before addressing comments to prevent conflicts. Looks like this PR is ready 🚢

olafurpg · 2018-01-04T20:23:18Z

languageserver/src/main/scala/langserver/core/Notifications.scala

+
+  def showMessage(params: ShowMessageParams): Unit
+  final def showMessage(
+      tpe: MessageType,


olafurpg · 2018-01-04T20:23:37Z

languageserver/src/main/scala/langserver/messages/Commands.scala

+  capabilities: ClientCapabilities)
+object InitializeParams {
+  implicit val format: OFormat[InitializeParams] = Json.format[InitializeParams]
+}


Fixed in #165 🎉

olafurpg · 2018-01-04T20:24:01Z

languageserver/src/main/scala/langserver/messages/Commands.scala

+  capabilities: ClientCapabilities)
+object InitializeParams {
+  implicit val format: OFormat[InitializeParams] = Json.format[InitializeParams]
+}


I didn't want to change the json library in this PR as it was doing enough already

olafurpg · 2018-01-04T20:24:25Z

languageserver/src/main/scala/langserver/messages/Commands.scala

+object DocumentSymbolParams {
+  implicit val format: OFormat[DocumentSymbolParams] = Json.format[DocumentSymbolParams]
+}
+case class TextDocumentRenameRequest(params: RenameParams)


Ditto, the names should match the interfaces in the lsp spec.

olafurpg · 2018-01-04T20:27:06Z

metaserver/src/main/scala/scala/meta/languageserver/Main.scala

@@ -29,11 +32,17 @@ object Main extends LazyLogging {
      // messes up with the client, since stdout is used for the language server protocol
      System.setOut(out)
      System.setErr(err)
+      val client = new LanguageClient(stdout)


I kept it identical to the lsp4j name, which is LanguageClient.

It's a true "client" in the sense that you can send any generic json rpc requests to a server and get responses back. I don't think a proxy or clientConnection improve readability.

olafurpg · 2018-01-04T21:07:52Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+      logger.info("exit(0)")
+      sys.exit(0)
+    }
+    .requestAsync[TextDocumentPositionParams, CompletionList](


Yes, the former delegates, I made the distinction to reduce ceremony in adding new handlers. All handlers are currently synchronous except the codeActions handler that is truly asynchronous since it does a workspaceEdit roundtrip.

olafurpg · 2018-01-04T21:08:34Z

metaserver/src/main/scala/scala/meta/languageserver/protocol/BaseProtocolMessage.scala

+    val sb = new java.lang.StringBuilder()
+    header.foreach {
+      case (key, value) =>
+        sb.append(key).append(": ").append(value).append("\n")


olafurpg · 2018-01-04T21:11:17Z

metaserver/src/main/scala/scala/meta/languageserver/providers/DocumentFormattingProvider.scala

@@ -79,14 +78,11 @@ class DocumentFormattingProvider(
      }
    }
    formatResult match {


olafurpg · 2018-01-04T21:12:22Z

metaserver/src/main/scala/scala/meta/languageserver/providers/DocumentFormattingProvider.scala

@@ -79,14 +78,11 @@ class DocumentFormattingProvider(
      }
    }
    formatResult match {


It's a shame the stdlib Either does not have a signature like that, it's so close to fold but different enough

olafurpg · 2018-01-04T21:18:50Z

metaserver/src/main/scala/scala/meta/languageserver/refactoring/OrganizeImports.scala

-      )
-    )
+    Task {
+      result match {


Nice! Only gripe with these APIs is finding the right imports, I just want to type Either.fromOpt<TAB> and have the editor insert the import. Only if we had a language server to do this for us 🤔

olafurpg · 2018-01-04T21:46:50Z

CI is green, let's do this

fommil · 2018-01-04T22:21:08Z

FYI ensime's protocol (including LSP) is drop-in ADT and it shows up automatically.

olafurpg added 12 commits January 3, 2018 05:29

Experiment with different service

27848d3

Reimplement json rpc data structures

804b6e8

Implement Observable[LSPMessage]

96667fe

Implement loop

bf51578

Refactor code for cleaner organization

9296fc0

Introduce scala.meta.languageserver.protocol

1469104

Implement LSP with new scheme

44c0acb

Start implementing lsp messages, looking good

ac32a84

Before the great refactoring

101f302

Tests pass!

0e259ec

Clean up languageserver project

0085b08

Clean before PR

78ac116

- fix $/cancelParams - remove scala-json-rpc - remove dead code

olafurpg mentioned this pull request Jan 3, 2018

WIP: Migrate monix usage to higher level constructs #160

Closed

xeno-by reviewed Jan 4, 2018

View reviewed changes

gabro approved these changes Jan 4, 2018

View reviewed changes

Replace play-json with circe

d9b38bc

gabro mentioned this pull request Jan 4, 2018

Replace play-json with circe #165

Closed

gabro and others added 6 commits January 4, 2018 17:19

Re-add commented out code

b80928f

Drop null values in tests

58e78e0

Remove a bit of boilerplate with context bounds and circe.syntax

f7ccaf1

Format

c10c55a

Add explicit cats dependency

9924498

We've started using cats, but so far it was a sub-dependency of circe and monix.

Clean up code a bit

9184870

- Report message in document formatting provider since invalid params are not reported in the vscode ui. - Use cats syntax for either - Use move executeCommand to separate method

olafurpg commented Jan 4, 2018

View reviewed changes

olafurpg merged commit 6999bc0 into scalameta:master Jan 4, 2018

This was referenced Jan 6, 2018

Refactor JSON-RPC read/write loop to use monix #152

Closed

Show progress during startup #92

Closed

olafurpg deleted the json-rpc branch November 30, 2018 10:39

		libraryDependencies ++= Seq(
		"com.dhpcs" %% "scala-json-rpc" % "2.0.1",

Implement JSON-RPC and LSP with Monix #164

Implement JSON-RPC and LSP with Monix #164

Conversation

olafurpg commented Jan 3, 2018 • edited Loading

olafurpg commented Jan 3, 2018

Sciss commented Jan 4, 2018

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

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

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

gabro commented Jan 4, 2018

Sciss commented Jan 4, 2018 • edited Loading

gabro 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

Choose a reason for hiding this comment

olafurpg 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

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

olafurpg commented Jan 4, 2018

fommil commented Jan 4, 2018

olafurpg commented Jan 3, 2018 •

edited

Loading

Sciss commented Jan 4, 2018 •

edited

Loading