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

Switch to lsp4j #31

Closed
wants to merge 3 commits into from
Closed

Switch to lsp4j #31

wants to merge 3 commits into from

Conversation

gabro
Copy link
Member

@gabro gabro commented Nov 11, 2017

This PR replaces dragos-vscode-scala LSP bindings with lsp4j.

The reason for this change is that lsp4j is a complete implementation of the LSP, as per the specification. This saves us adding new bindings on demand.

The downside is some "javosity" creeping into ScalametaLanguageServer code, but nothing too awful.

Currently this compiles but it throws a runtime exception about some JSON-RPC error that I'm trying to debug.

Copy link
Member

@olafurpg olafurpg 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 so far 👍 Thanks a lot for taking on this PR! The java api I can live with. Let me know when this is ready for review and I can test it out locally.

)
}
}

// Unimplemented features
override def codeAction(params: CodeActionParams) = ???
Copy link
Member

Choose a reason for hiding this comment

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

Nice to get these listed out 👍

@@ -6,19 +6,6 @@ inThisBuild(
)
)

lazy val languageserver = project
Copy link
Member

Choose a reason for hiding this comment

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

Also remove the sources?

@laughedelic
Copy link
Member

Does this change mean that you have no plans for building the server for Scala.js?

@olafurpg
Copy link
Member

@laughedelic There are no plans to build the server for Scala.js, even with the language-server we have in current master. We are pretty married to the JVM for the server since we invoke the compiler and classload for example custom scalafix rules.

@laughedelic
Copy link
Member

I see. Thanks for clarifying this 👍

@@ -26,7 +26,11 @@ object Main extends LazyLogging {
System.setErr(err)
logger.info(s"Starting server in $workspace")
logger.info(s"Classpath: ${Properties.javaClassPath}")
server.start()
val server = new ScalametaLanguageServer(cwd, out)
val launcher = LSPLauncher.createServerLauncher(server, System.in, out)
Copy link
Member

Choose a reason for hiding this comment

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

A question not related to lsp4j itself, but more in the context of rewriting this:
Would it be hard to get rid of this mechanism of passing cwd through a var? I think that in LSP supposes that you can launch a server independently and then tell it where your project is through the initialization parameters (rootUri).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think that's the way to do it. I wanted to take on that, as soon as I get it up and running

This is to work around a bug with Scala 2.12 and Java runtime
reflection, that affects lsp4j
@olafurpg
Copy link
Member

I got the server starting with this commit here (after a looong debug session!)

0efcec2

The presentation compiler does not start and it seems file watch events don't trigger, but it's at least a start :)

@olafurpg
Copy link
Member

I got the PC and scalafix starting by making the client lazy (it was null causing crashes). However, textDocument/onOpen and didChange don't trigger when I'm running the client. My WIP branch is here https://github.com/scalameta/language-server/compare/master...olafurpg:lsp4j-get?expand=1 it's based off the first commit in this PR before implementing the server in java.

I suspect the problem may be related to trait encodings in 2.11, even if it compiles it could be that the runtime reflection is not invoking our methods. I'm honestly starting to think we might be better off to continue with dragos/language-server, I believe would have taken me less time to write out the full protocol in our current master that I've spent trying to debug this java reflection madness 😅

@gabro
Copy link
Member Author

gabro commented Nov 14, 2017

9g2ifww 1

(original comic)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants