From 7d2fdacbb06e4dd569952918e39eec470f29d8ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Wro=C5=84ski?= Date: Sat, 15 Oct 2022 12:05:59 +0200 Subject: [PATCH] Add support for actionable diagnotics from scala-cli --- .../meta/internal/metals/Diagnostics.scala | 4 +- .../internal/metals/MetalsEnrichments.scala | 10 ++- .../internal/metals/ScalacDiagnostic.scala | 6 ++ .../codeactions/ActionableDiagnostic.scala | 61 +++++++++++++++++++ .../codeactions/CodeActionProvider.scala | 1 + .../main/scala/tests/BaseScalaCliSuite.scala | 55 +++++++++-------- .../codeactions/BaseCodeActionLspSuite.scala | 49 +++++++++++---- .../ActionableDiagnosticsSuite.scala | 42 +++++++++++++ 8 files changed, 188 insertions(+), 40 deletions(-) create mode 100644 metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala create mode 100644 tests/unit/src/test/scala/tests/codeactions/ActionableDiagnosticsSuite.scala diff --git a/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala b/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala index 037bbc18443..fa1368ee6c8 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala @@ -261,12 +261,14 @@ final class Diagnostics( snapshot: Input, ): Option[Diagnostic] = { val result = edit.toRevised(d.getRange).map { range => - new l.Diagnostic( + val ld = new l.Diagnostic( range, d.getMessage, d.getSeverity, d.getSource, ) + ld.setData(d.getData) + ld } if (result.isEmpty) { d.getRange.toMeta(snapshot).foreach { pos => diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala index 6f8b22a12d8..63874c75efd 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala @@ -677,6 +677,9 @@ object MetalsEnrichments val severity = d.getSeverity.toString.toLowerCase() s"$severity:$hint $uri:${d.getRange.getStart.getLine} ${d.getMessage}" } + def asTextEdit: Option[l.TextEdit] = { + decodeJson(d.getData, classOf[l.TextEdit]) + } } implicit class XtensionSeverityBsp(sev: b.DiagnosticSeverity) { @@ -752,8 +755,8 @@ object MetalsEnrichments } implicit class XtensionDiagnosticBsp(diag: b.Diagnostic) { - def toLsp: l.Diagnostic = - new l.Diagnostic( + def toLsp: l.Diagnostic = { + val ld = new l.Diagnostic( diag.getRange.toLsp, fansi.Str(diag.getMessage, ErrorMode.Strip).plainText, diag.getSeverity.toLsp, @@ -761,6 +764,9 @@ object MetalsEnrichments // We omit diag.getCode since Bloop's BSP implementation uses 'code' with different semantics // than LSP. See https://github.com/scalacenter/bloop/issues/1134 for details ) + ld.setData(diag.getData) + ld + } } implicit class XtensionHttpExchange(exchange: HttpServerExchange) { diff --git a/metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala b/metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala index c446fe9501d..6de84e2b3f4 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala @@ -1,9 +1,15 @@ package scala.meta.internal.metals +import scala.meta.internal.metals.MetalsEnrichments._ + import org.eclipse.{lsp4j => l} object ScalacDiagnostic { + object ScalaAction { + def unapply(d: l.Diagnostic): Option[l.TextEdit] = d.asTextEdit + } + object NotAMember { private val regex = """(?s)value (.+) is not a member of.*""".r def unapply(d: l.Diagnostic): Option[String] = diff --git a/metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala b/metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala new file mode 100644 index 00000000000..92cde048e6e --- /dev/null +++ b/metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala @@ -0,0 +1,61 @@ +package scala.meta.internal.metals.codeactions + +import scala.concurrent.ExecutionContext +import scala.concurrent.Future + +import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals._ +import scala.meta.pc.CancelToken + +import org.eclipse.{lsp4j => l} + +class ActionableDiagnostic() extends CodeAction { + override def kind: String = l.CodeActionKind.QuickFix + + override def contribute( + params: l.CodeActionParams, + token: CancelToken, + )(implicit ec: ExecutionContext): Future[Seq[l.CodeAction]] = { + + def path = + params.getTextDocument.getUri.toAbsolutePath.toURI.toString + + def createDiagnosticAction( + diagnostic: l.Diagnostic, + textEdit: l.TextEdit, + ): l.CodeAction = { + val codeAction = new l.CodeAction() + val diagMessage = diagnostic.getMessage + codeAction.setKind(l.CodeActionKind.QuickFix) + codeAction.setTitle( + s"Apply suggestion: ${diagMessage.split(System.lineSeparator()).headOption.getOrElse(diagMessage)}" + ) + codeAction.setDiagnostics(List(diagnostic).asJava) + codeAction.setEdit( + new l.WorkspaceEdit( + Map(path -> List(textEdit).asJava).asJava + ) + ) + codeAction + } + + val codeActions = params + .getContext() + .getDiagnostics() + .asScala + .groupBy { + case ScalacDiagnostic.ScalaAction(textEdit) => + Some(textEdit) + case _ => None + } + .collect { + case (Some(textEdit), diags) + if params.getRange().overlapsWith(diags.head.getRange()) => + createDiagnosticAction(diags.head, textEdit) + } + .toList + .sorted + + Future.successful(codeActions) + } +} diff --git a/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala index 583af07da15..d82ee53222c 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala @@ -29,6 +29,7 @@ final class CodeActionProvider( new ImplementAbstractMembers(compilers), new ImportMissingSymbol(compilers, buildTargets), new CreateNewSymbol(), + new ActionableDiagnostic(), new StringActions(buffers), extractMemberAction, new SourceOrganizeImports( diff --git a/tests/unit/src/main/scala/tests/BaseScalaCliSuite.scala b/tests/unit/src/main/scala/tests/BaseScalaCliSuite.scala index 136d9e64043..6cf97f2ec1a 100644 --- a/tests/unit/src/main/scala/tests/BaseScalaCliSuite.scala +++ b/tests/unit/src/main/scala/tests/BaseScalaCliSuite.scala @@ -101,35 +101,12 @@ abstract class BaseScalaCliSuite(scalaVersion: String) |} | |""".stripMargin - - private def escape(s: String): String = - s.replace("\\", "\\\\") private def bspLayout = s"""/.bsp/scala-cli.json - |{ - | "name": "scala-cli", - | "argv": [ - | "${escape(ScalaCli.javaCommand)}", - | "-cp", - | "${escape(ScalaCli.scalaCliClassPath().mkString(File.pathSeparator))}", - | "${ScalaCli.scalaCliMainClass}", - | "bsp", - | "." - | ], - | "version": "${BuildInfo.scalaCliVersion}", - | "bspVersion": "2.0.0", - | "languages": [ - | "scala", - | "java" - | ] - |} + |${BaseScalaCliSuite.scalaCliBspJsonContent()} | |/.scala-build/ide-inputs.json - |{ - | "args": [ - | "." - | ] - |} + |${BaseScalaCliSuite.scalaCliIdeInputJson(".")} | |""".stripMargin @@ -354,3 +331,31 @@ abstract class BaseScalaCliSuite(scalaVersion: String) } yield () } } + +object BaseScalaCliSuite { + def scalaCliBspJsonContent(args: List[String] = Nil): String = { + val argv = List( + ScalaCli.javaCommand, + "-cp", + ScalaCli.scalaCliClassPath().mkString(File.pathSeparator), + ScalaCli.scalaCliMainClass, + "bsp", + ".", + ) ++ args + val bsjJson = ujson.Obj( + "name" -> "scala-cli", + "argv" -> argv, + "version" -> BuildInfo.scalaCliVersion, + "bspVersion" -> "2.0.0", + "languages" -> List("scala", "java"), + ) + ujson.write(bsjJson) + } + + def scalaCliIdeInputJson(args: String*): String = { + val ideInputJson = ujson.Obj( + "args" -> args + ) + ujson.write(ideInputJson) + } +} diff --git a/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala b/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala index b61cddb7d92..a339293aaa3 100644 --- a/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala +++ b/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala @@ -9,6 +9,7 @@ import munit.Location import munit.TestOptions import org.eclipse.lsp4j.CodeAction import tests.BaseLspSuite +import tests.BaseScalaCliSuite import tests.FileLayout abstract class BaseCodeActionLspSuite( @@ -48,6 +49,7 @@ abstract class BaseCodeActionLspSuite( kind: List[String] = Nil, scalafixConf: String = "", scalacOptions: List[String] = Nil, + scalaCliOptions: List[String] = Nil, configuration: => Option[String] = None, scalaVersion: String = scalaVersion, renamePath: Option[String] = None, @@ -56,6 +58,9 @@ abstract class BaseCodeActionLspSuite( changeFile: String => String = identity, expectError: Boolean = false, filterAction: CodeAction => Boolean = _ => true, + scalaCliLayout: Boolean = false, + assertion: Option[Unit => Unit] = None, + saveResult: Boolean = true, )(implicit loc: Location): Unit = { val scalacOptionsJson = if (scalacOptions.nonEmpty) @@ -63,12 +68,21 @@ abstract class BaseCodeActionLspSuite( else "" val path = s"a/src/main/scala/a/$fileName" - val layout = - s"""/metals.json - |{"a":{$scalacOptionsJson "scalaVersion" : "$scalaVersion"}} - |$scalafixConf - |/$path - |$input""".stripMargin + val layout = { + if (scalaCliLayout) + s"""/.bsp/scala-cli.json + |${BaseScalaCliSuite.scalaCliBspJsonContent(scalaCliOptions)} + |/.scala-build/ide-inputs.json + |${BaseScalaCliSuite.scalaCliIdeInputJson(".")} + |/$path + |$input""".stripMargin + else + s"""/metals.json + |{"a":{$scalacOptionsJson "scalaVersion" : "$scalaVersion"}} + |$scalafixConf + |/$path + |$input""".stripMargin + } checkEdit( name, @@ -84,6 +98,8 @@ abstract class BaseCodeActionLspSuite( changeFile, expectError, filterAction, + assertion, + saveResult, ) } @@ -101,6 +117,8 @@ abstract class BaseCodeActionLspSuite( changeFile: String => String = identity, expectError: Boolean = false, filterAction: CodeAction => Boolean = _ => true, + assertion: Option[Unit => Unit] = None, + saveResult: Boolean = true, )(implicit loc: Location): Unit = { val files = FileLayout.mapFromString(layout) val (path, input) = files @@ -141,13 +159,20 @@ abstract class BaseCodeActionLspSuite( case _: Throwable if expectError => Nil } _ <- client.applyCodeAction(selectedActionIndex, codeActions, server) - _ <- server.didSave(newPath) { _ => - if (newPath != path) - server.toPath(newPath).readText - else - server.bufferContents(newPath) + _ <- + if (saveResult) { + server.didSave(newPath) { _ => + if (newPath != path) + server.toPath(newPath).readText + else + server.bufferContents(newPath) + } + } else Future {} + _ = assertion match { + case Some(assert) => assert() + case None => + assertNoDiff(server.bufferContents(newPath), actualExpectedCode) } - _ = assertNoDiff(server.bufferContents(newPath), actualExpectedCode) _ = if (expectNoDiagnostics) assertNoDiagnostics() else () _ = extraOperations } yield () diff --git a/tests/unit/src/test/scala/tests/codeactions/ActionableDiagnosticsSuite.scala b/tests/unit/src/test/scala/tests/codeactions/ActionableDiagnosticsSuite.scala new file mode 100644 index 00000000000..b61e914980f --- /dev/null +++ b/tests/unit/src/test/scala/tests/codeactions/ActionableDiagnosticsSuite.scala @@ -0,0 +1,42 @@ +package tests.codeactions + +import scala.meta.internal.metals.codeactions.ActionableDiagnostic + +import coursier.version.Version + +class ActionableDiagnosticsSuite + extends BaseCodeActionLspSuite("actionableDiagnostic") { + + val oldOsLibVersion: Version = Version("0.7.8") + + check( + "actionable-diagnostic-update", + s"""|//> <<>>using lib "com.lihaoyi::os-lib:${oldOsLibVersion.repr}" + | + |object Hello extends App { + | println("Hello") + |} + |""".stripMargin, + "Apply suggestion: com.lihaoyi::os-lib:0.7.8 is outdated", + expectedCode = "", + scalaCliOptions = List("--actions", "-S", scalaVersion), + expectNoDiagnostics = false, + scalaCliLayout = true, + saveResult = false, + assertion = Some { _ => + val sourceContent = server.bufferContents("a/src/main/scala/a/A.scala") + val newOsLibVersion = raw"\d+\.\d+\.\d+".r + .findFirstIn(sourceContent) + .map(Version(_)) + assert( + newOsLibVersion.nonEmpty, + "os-lib dependency should have been updated", + ) + assert( + oldOsLibVersion < newOsLibVersion.get, + s"os-lib dependency should have been updated to newer version than ${oldOsLibVersion.repr}", + ) + }, + ) + +}