WIP: Replace lsp implementation with lsp4s

[zainab-ali]

This is a WIP and shouldn't be reviewed yet, but I wanted to give it some visibility so others didn't repeat the work.

Since last month's tooling hackday I, with the help of @peterneyens, have been working on integrating lsp4s. It's a much simpler task than I anticipated, partly due to lsp4s's clean structure (logging seems to be the most complicated aspect). I expect to have a working implementation over the next few weeks.

There are some changes that may be worth discussing, even in these early stages, notably the dependency on monix. According to some discussion in #1935, this might be problematic.

[olafurpg]

Cool to see lsp4s getting more use! Please note that documentation is slim and our testing infrastructure is not as good as I'd like it to be. The module is however used in metals, bloop, upcoming IntelliJ-scala and also at Twitter so it seems to be working OK 😅 We recently extracted the module to a separate repo https://github.com/scalameta/lsp4s, happy to answer questions or respond to issues.

[olafurpg]

There is a v0.1.0 release on Maven http://search.maven.org/#artifactdetails%7Corg.scalameta%7Clsp4s_2.12%7C0.1.0%7Cjar

[fommil]

Cool. Just a heads up that this breaks from the convention of minimising the dependency list, which is typically the biggest pain when supporting new versions of scala... but that's not a hill I care to die on.

[olafurpg]

For the record, lsp4s brings in a lot of dependencies

• circe
• monix
• scribe
• shapeless (via circe-generic)
• jawn (via circe-parser)
• cats (via circe-core, and soon monix as well)

There's a discussion in https://github.com/scalameta/lsp4s/issues/8 to slim it down (and I gave it an honest try in scalameta/lsp4s#4) but it's not something I expect to prioritize in the coming months.

[zainab-ali]

We were aware of its dependencies, but didn't think that any of them were a sticking point. Even if they are, I think the plan would be to minimise them.

@fommil would you be happy with this PR depending on the current form of lsp4s, or do you think there are some dependencies we should try to rip out of there first? I'm not too familiar with the pains of scala version support.

[olafurpg]

@andyscott was kind enough to contribute a PR scalameta/lsp4s#9 that removes the shapeless dependency. I also opened scalameta/lsp4s#10 that removes the pprint dependency that was included by accident and was not used.

Replacing circe with another JSON library should not be too hard, you can use https://github.com/scalameta/lsp4s/pulls/4 as a starting point. I think you'll have a harder time replacing Monix, however.

[fommil]

@zainab-ali yeah it's great! 😄

We might want to just use circe everywhere. @Tomahna was suggesting similar recently. Our internal json lib will probably just morph into a case study in my book.

You don't need to check anything with me 😁 ... I'm not really using ensime anymore and I'm not watching the repo (except in a few work projects that benefit from autocomplete) so please merge / release stuff when you think it's ready. I can share sonatype creds for the ensime user with any of you, just send me your public gpg key. Of course, I'm happy to help out if you have questions.

[olafurpg]

FYI https://github.com/scalameta/lsp4s/releases/tag/v0.2.0

[fommil]

@zainab-ali btw I have been writing a jsonformat encoder/decoder typeclass for my book with examples using magnolia and shapeless. It is able to handle default arguments and user config for null. That means users can define this kind of thing

sealed abstract class Possibly[A]
case class Missing[A]() extends Possibly[A]
case class Found[A](a: A) extends Possibly[A]
object Possibly {
  implicit def encoder[A: JsEncoder]: JsEncoder[Possibly] = JsEncoder[A].contramap(_.a)
  implicit def decoder[A: JsDecoder]: JsDecoder[Possibly] = JsDecoder[A].map(Found(_))
}

and have

final case class Wibble(@json.nulls() foo: Possibly[Option[String]] = Missing())

which will distinguish between getting a null, not getting anything, and getting a valid value.

Anyways, I don't have plans to bring it into ensime but just food for thought.

[zainab-ali]

This seems to have turned into a fairly large (if not entire) rewrite of the lsp implementation and it would be worth discussing some of the decisions here before moving too much further.

Main concerns

IO

The code is written in a functional style, so it needs something to handle IO. I've depended on Monix's task for this, since it was there, but would be happy to swap this out for something else. I don't think there's a discussion on the choice of IO for Ensime yet.

Use of a functional library

After using functional IO and error handling, I found that I needed an EitherT so hacked together an EitherTask to do the job. Introducing a functional library is a much larger discussion, and should probably be deferred to a different PR, but it would be worth discussing what we'd want to use. It's worth noting that circe seems to be pulling in cats.

Dependencies

The notable dependencies pulled in by lsp4s seem to be:

• monix
• cats (pulled in by monix)
• circe
• scribe

Remaining work

There isn't too much left, but given that I thought this would take me a few weeks, I probably can't be relied on for a decent time estimate.

1. More tests. Both unit tests mocking up the ensime actor system and integration tests sending and receiving json lsp messages. I need to dive into the integration test system and figure out what to do here.
2. Clean up the logging. The scribe / slf4j boundary needs to be sorted out
3. Add publishDiagnostics. This is the only message in the previous implementation that is missing in the current one.

Even once these are tackled, there will be plenty of messages missing. These can be added in later PRs. Making a GitHub issue with a checklist of messages that are / aren't implemented is probably the way to go.