Implement textEdit/codeAction #154

olafurpg · 2017-12-22T22:30:22Z

This PR implements a codeAction to remove unused imports with scalafix. The patch produced by scalafix is converted to precise TextEdit per changed token, instead of sending a single TextEdit to replace the whole file as we do with scalafmt.

codeActions appear as lightbulbs above the cursor and when the lightbulb is clicked a dialogue opens with a list of commands to run (not shown in gif).

All of ScalafixPatchEnrichments is copy pasted from the scalafix sources, with the next release of scalafix this file will no longer be necessary since in master we have an API to get List[TokenPatch] from a Patch

olafurpg · 2017-12-23T04:08:06Z

First time scalafix fails the build 😄

--- /home/travis/build/scalameta/language-server/languageserver/src/main/scala/langserver/messages/Commands.scala
+++ <expected fix from ExplicitResultTypes+RemoveUnusedImports+Disable>
@@ -5,6 +5,7 @@
 import langserver.types._
 import langserver.utils.JsonRpcUtils
 import play.api.libs.json.OFormat
+import langserver.messages.ApplyWorkspaceEditParams
 
 sealed trait Message
 sealed trait ServerCommand extends Message
@@ -229,7 +230,7 @@
 case class WorkspaceSymbolRequest(params: WorkspaceSymbolParams) extends ServerCommand
 case class ApplyWorkspaceEditParams(label: Option[String], edit: WorkspaceEdit)
 object ApplyWorkspaceEditParams {
-  implicit val format = Json.format[ApplyWorkspaceEditParams]
+  implicit val format: OFormat[ApplyWorkspaceEditParams] = Json.format[ApplyWorkspaceEditParams]
 }

ShaneDelmore · 2017-12-23T15:16:50Z

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

-    CodeActionResult(Nil)
+    val removeUnusedImports = request.params.context.diagnostics.collectFirst {
+      case Diagnostic(_, _, _, Some("scalac"), "Unused import") =>
+        // This command doesn't seem to do much


This comment confuses me.

It's a leftover comment before I made it remove unused imports, comment removed

ShaneDelmore · 2017-12-23T15:21:58Z

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

+      case Some(document) =>
+        removeUnused(
+          uri,
+          schema.Database(document :: Nil).toDb(None).documents.head


This reads to me like it is creating a single document database just to convert it to a different database format to then extract the single document provided initially? Is this correct? If so a method to name the purpose would be helpful, or just an extension method, toOtherDocumentType.

Also, while this is not new code introduced here schema.Database(...).toDb... is also confusing. It reads as “Create a db then convert it to a db”.

I've moved this to an extension method.

The API around scala.meta.Database / schema.Database conversions is quite awkward, we used to have fine grained conversions per type Document/Message/Synthetic but now it's all in a big fat method only to convert scala.meta.Database. language-server has been a great test on a lot of the scalameta APIs, I would love to incorporate some of our workarounds here into the core API

ShaneDelmore · 2017-12-23T15:26:19Z

metaserver/src/main/scala/scalafix/languageserver/ScalafixPatchEnrichments.scala

+// Copy-pasta from scalafix because all of these methods are private.
+// We should expose a package private API to get a list of token patches from
+// a Patch.
+// TODO(olafur): Figure out how to expose a minimal public API in scalafix.Patch


I think we can actually remove this with the next scalafix release, Guillaume already refactored this API to support --diff and // scalafix:off for rewrites

ShaneDelmore

I had a couple of questions but overall it looks really good.

In case an exception happens on server startup then the server exited silently.

Users can put the cursor or hover over an unused import to see a lightbulb appear. If the user clicks on the lightbulb, a dialogue appears with options to run commands like "Remove unused import". This commit turned out to be quite tricky since I wasn't sure how to submit a request from the server to the client for workspace/applyEdit. It turned out to be quite dead simple, just send a JsonRpcRequestMessage and discard the response.

olafurpg · 2017-12-23T22:28:34Z

Thank you for the review @ShaneDelmore !

laughedelic

Cool 💯 I tried it out and it works! I left just a couple of questions.

One unexpected thing for me was that it removes all unused imports while I'd expect a code action bound to a diagnostic act only on the underlined region. Is it by design or is it just the way Scalafix does it by default?

laughedelic · 2017-12-26T01:31:17Z

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

@@ -58,6 +59,13 @@ abstract class Connection(inStream: InputStream, outStream: OutputStream)(implic
    }
  }

+  var i = new AtomicInteger()


Should it be private?

Yes! And it can also be a val

laughedelic · 2017-12-26T01:40:16Z

metaserver/src/test/scala/tests/compiler/CompilerSuite.scala

@@ -14,6 +16,10 @@ class CompilerSuite extends MegaSuite {
      Nil
  )

+  def toDocument(name: String, code: String): Document = {
+    InteractiveSemanticdb.toDocument(compiler, code, name, 10000)


What is this number?

/** @param timeout max number of milliseconds to allow the presentation compiler */

from: https://github.com/scalameta/scalameta/blob/00e7bb0683ad646b20c72482976e2b1f651da2b5/scalameta/semanticdb-scalac-core/src/main/scala/scala/meta/interactive/InteractiveSemanticdb.scala#L47

I have some WIP experiments to use monix Task to avoid ad-hoc timeouts and support proper cancellation for presentation compiler requests

laughedelic · 2017-12-26T01:46:26Z

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

@@ -10,6 +10,7 @@ case object WorkspaceCommand extends Enum[WorkspaceCommand] {

  case object ClearIndexCache extends WorkspaceCommand
  case object ResetPresentationCompiler extends WorkspaceCommand
+  case object ScalafixUnusedImports extends WorkspaceCommand


Would it be possible (in the future) to adapt Scalafix rewrites to code actions so that it won't be necessary to define a WorkspaceCommand for each type of rewrite?

Scalafix absolutely, scalafix can run custom rules from configuration in .scalafix.conf, we can probably embed that config as command arguments to get away with only a single Scalafix WorkspaceCommand.

olafurpg

Thanks for the review!

olafurpg · 2017-12-23T22:25:10Z

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

+      case Some(document) =>
+        removeUnused(
+          uri,
+          schema.Database(document :: Nil).toDb(None).documents.head


I've moved this to an extension method.

The API around scala.meta.Database / schema.Database conversions is quite awkward, we used to have fine grained conversions per type Document/Message/Synthetic but now it's all in a big fat method only to convert scala.meta.Database. language-server has been a great test on a lot of the scalameta APIs, I would love to incorporate some of our workarounds here into the core API

olafurpg · 2017-12-23T22:27:08Z

metaserver/src/main/scala/scalafix/languageserver/ScalafixPatchEnrichments.scala

+// Copy-pasta from scalafix because all of these methods are private.
+// We should expose a package private API to get a list of token patches from
+// a Patch.
+// TODO(olafur): Figure out how to expose a minimal public API in scalafix.Patch


I think we can actually remove this with the next scalafix release, Guillaume already refactored this API to support --diff and // scalafix:off for rewrites

olafurpg · 2017-12-29T12:24:31Z

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

@@ -58,6 +59,13 @@ abstract class Connection(inStream: InputStream, outStream: OutputStream)(implic
    }
  }

+  var i = new AtomicInteger()


Yes! And it can also be a val

olafurpg · 2017-12-29T12:26:00Z

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

@@ -10,6 +10,7 @@ case object WorkspaceCommand extends Enum[WorkspaceCommand] {

  case object ClearIndexCache extends WorkspaceCommand
  case object ResetPresentationCompiler extends WorkspaceCommand
+  case object ScalafixUnusedImports extends WorkspaceCommand


Scalafix absolutely, scalafix can run custom rules from configuration in .scalafix.conf, we can probably embed that config as command arguments to get away with only a single Scalafix WorkspaceCommand.

olafurpg · 2017-12-29T12:34:00Z

metaserver/src/test/scala/tests/compiler/CompilerSuite.scala

@@ -14,6 +16,10 @@ class CompilerSuite extends MegaSuite {
      Nil
  )

+  def toDocument(name: String, code: String): Document = {
+    InteractiveSemanticdb.toDocument(compiler, code, name, 10000)


I have some WIP experiments to use monix Task to avoid ad-hoc timeouts and support proper cancellation for presentation compiler requests

olafurpg · 2017-12-29T12:34:56Z

@laughedelic PS, that gif from atom looks amazing 😍

olafurpg · 2017-12-29T12:38:29Z

One unexpected thing for me was that it removes all unused imports while I'd expect a code action bound to a diagnostic act only on the underlined region. Is it by design or is it just the way Scalafix does it by default?

This is intentional, we could make scalafix remove only a single unused import but I'm not sure how useful that would be. In IntelliJ there is only a single "Organize imports" which both removes unused imports and sorts/groups them. To remove a single imports it's easier to just remove it manually, IMO

olafurpg requested review from gabro and ShaneDelmore December 22, 2017 22:30

olafurpg force-pushed the codeAction branch from feb10a1 to bcc8da4 Compare December 23, 2017 04:09

ShaneDelmore reviewed Dec 23, 2017

View reviewed changes

olafurpg force-pushed the codeAction branch 2 times, most recently from f6653a5 to 795e27f Compare December 23, 2017 15:18

ShaneDelmore reviewed Dec 23, 2017

View reviewed changes

olafurpg added 3 commits December 23, 2017 20:10

Report top-level errors in Main

07a04c0

In case an exception happens on server startup then the server exited silently.

Implement boilerplate around textDocument/codeAction

9740964

olafurpg force-pushed the codeAction branch from 795e27f to fcb6207 Compare December 23, 2017 20:15

Address review feedback

bf30533

laughedelic approved these changes Dec 26, 2017

View reviewed changes

olafurpg commented Dec 29, 2017

View reviewed changes

s/var/private val/

fdc9c83

olafurpg merged commit cdbf2f2 into scalameta:master Dec 29, 2017

olafurpg deleted the codeAction branch December 29, 2017 12:38

olafurpg mentioned this pull request Jan 3, 2018

Implement JSON-RPC and LSP with Monix #164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement textEdit/codeAction #154

Implement textEdit/codeAction #154

olafurpg commented Dec 22, 2017

olafurpg commented Dec 23, 2017

ShaneDelmore Dec 23, 2017

olafurpg Dec 23, 2017

ShaneDelmore Dec 23, 2017

ShaneDelmore Dec 23, 2017

olafurpg Dec 23, 2017

ShaneDelmore Dec 23, 2017

olafurpg Dec 23, 2017

ShaneDelmore left a comment

olafurpg commented Dec 23, 2017

laughedelic left a comment •

edited

Loading

laughedelic Dec 26, 2017

olafurpg Dec 29, 2017

laughedelic Dec 26, 2017

gabro Dec 29, 2017

olafurpg Dec 29, 2017

laughedelic Dec 26, 2017

olafurpg Dec 29, 2017

olafurpg left a comment

olafurpg Dec 23, 2017

olafurpg Dec 23, 2017

olafurpg Dec 29, 2017

olafurpg Dec 29, 2017

olafurpg Dec 29, 2017

olafurpg commented Dec 29, 2017

olafurpg commented Dec 29, 2017

Implement textEdit/codeAction #154

Implement textEdit/codeAction #154

Conversation

olafurpg commented Dec 22, 2017

olafurpg commented Dec 23, 2017

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

ShaneDelmore left a comment

Choose a reason for hiding this comment

olafurpg commented Dec 23, 2017

laughedelic left a comment • 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

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

olafurpg commented Dec 29, 2017

olafurpg commented Dec 29, 2017

laughedelic left a comment •

edited

Loading