From 0739b373c28188938a1d1ceba8ca0ec41fbaff53 Mon Sep 17 00:00:00 2001 From: Katarzyna Marek Date: Wed, 18 Oct 2023 16:20:46 +0200 Subject: [PATCH] bugfix: choose correct workspace folder for jars and build target files --- .../internal/metals/FileDecoderProvider.scala | 19 +- .../internal/metals/WorkspaceLspService.scala | 14 +- .../feature/FileDecoderProviderLspSuite.scala | 167 ++++++++++++------ .../scala/tests/BuildServerInitializer.scala | 25 ++- .../src/main/scala/tests/TestingClient.scala | 6 + .../scala/tests/JavaDefinitionSuite.scala | 37 +++- 6 files changed, 191 insertions(+), 77 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scala index 80a69235da0..6e9b6d7ad8c 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scala @@ -4,7 +4,6 @@ import java.io.ByteArrayOutputStream import java.io.File import java.io.PrintStream import java.net.URI -import java.net.URLEncoder import javax.annotation.Nullable import scala.annotation.tailrec @@ -296,12 +295,14 @@ final class FileDecoderProvider( } private def decodeBuildTarget(uri: URI): DecoderResponse = { - val targetName = - URIEncoderDecoder.decode( - uri.toString.stripSuffix(".metals-buildtarget").split('/').last - ) - val text = - new BuildTargetInfo(buildTargets).buildTargetDetails(targetName) + val text = uri + .toString() + .toAbsolutePathSafe + .map { path => + val targetName = path.filename.stripSuffix(".metals-buildtarget") + new BuildTargetInfo(buildTargets).buildTargetDetails(targetName) + } + .getOrElse(s"Error transforming $uri to path") DecoderResponse.success(uri, text) } @@ -771,10 +772,10 @@ final class FileDecoderProvider( object FileDecoderProvider { def createBuildTargetURI( - workspace: AbsolutePath, + workspaceFolder: AbsolutePath, buildTargetName: String, ): URI = URI.create( - s"metalsDecode:${URLEncoder.encode(s"file:///${workspace.filename}/${buildTargetName}.metals-buildtarget")}" + s"metalsDecode:${workspaceFolder.resolve(s"${buildTargetName}.metals-buildtarget").toURI}" ) } diff --git a/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala index 849674a6a39..07e1bb93b8f 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala @@ -242,11 +242,17 @@ class WorkspaceLspService( def getServiceFor(path: AbsolutePath): MetalsLspService = getServiceForOpt(path).getOrElse(fallbackService) - def getServiceFor(uri: String): MetalsLspService = + def getServiceFor(uri: String): MetalsLspService = { + // "metalsDecode" prefix is used for showing special files and is not an actual file system + val strippedUri = uri.stripPrefix("metalsDecode:") (for { - // "metalsDecode" prefix is used for showing special files and is not an actual file system - path <- uri.stripPrefix("metalsDecode:").toAbsolutePathSafe() - } yield getServiceFor(path)).getOrElse(fallbackService) + path <- strippedUri.toAbsolutePathSafe() + service <- + if (strippedUri.startsWith("jar:")) + folderServices.find(_.buildTargets.inverseSources(path).nonEmpty) + else Some(getServiceFor(path)) + } yield service).getOrElse(fallbackService) + } def currentFolder: Option[MetalsLspService] = focusedDocument.flatMap(getServiceForOpt) diff --git a/tests/slow/src/test/scala/tests/feature/FileDecoderProviderLspSuite.scala b/tests/slow/src/test/scala/tests/feature/FileDecoderProviderLspSuite.scala index 3664a44a01f..7cc8f9a010d 100644 --- a/tests/slow/src/test/scala/tests/feature/FileDecoderProviderLspSuite.scala +++ b/tests/slow/src/test/scala/tests/feature/FileDecoderProviderLspSuite.scala @@ -323,32 +323,46 @@ class FileDecoderProviderLspSuite check( "semanticdb-jar-compact", - s""" - |/metals.json - |{ - | "a": { - | "scalaVersion": "${scala.meta.internal.metals.BuildInfo.scala213}", - | "libraryDependencies": ["org.scalameta::munit:0.7.29"] - | } - |} - |/a/src/main/scala/a/Main.scala - |package a - |import munit.Printable - | - |object Main { - | val a : Printable = null - | println(a) - |} - |""".stripMargin, - "a/src/main/scala/a/Main.scala", + Map( + "otherfolder" -> + s""" + |/metals.json + |{ + | "b": { + | "scalaVersion": "${scala.meta.internal.metals.BuildInfo.scala3}" + | } + |} + |/b/src/main/scala/b/Main.scala + |package b + | + |object Main { } + |""".stripMargin, + "somefolder" -> + s""" + |/metals.json + |{ + | "a": { + | "scalaVersion": "${scala.meta.internal.metals.BuildInfo.scala213}", + | "libraryDependencies": ["org.scalameta::munit:0.7.29"] + | } + |} + |/a/src/main/scala/a/Main.scala + |package a + |import munit.Printable + | + |object Main { + | val a : Printable = null + | println(a) + |} + |""".stripMargin, + ), + "somefolder/a/src/main/scala/a/Main.scala", None, - "decode", Right( FileDecoderProviderLspSuite.PrintableSemanticDBFile(coursierCacheDir) ), - customUri = Some( - s"metalsDecode:jar:${coursierCacheDir.toUri}v1/https/repo1.maven.org/maven2/org/scalameta/munit_2.13/0.7.29/munit_2.13-0.7.29-sources.jar!/munit/Printable.scala.semanticdb-compact" - ), + identity(_), + s"metalsDecode:jar:${coursierCacheDir.toUri}v1/https/repo1.maven.org/maven2/org/scalameta/munit_2.13/0.7.29/munit_2.13-0.7.29-sources.jar!/munit/Printable.scala.semanticdb-compact", ) check( @@ -433,19 +447,37 @@ class FileDecoderProviderSbtLspSuite checkBuildTarget( "sbt-buildtarget", - SbtBuildLayout( - s"""|/a/src/main/scala/Main.scala - |package a - |class A { - | def foo(): Unit = () - |} - |/b/src/main/scala/Main.scala - |package b - |class B { - | def foo(): Unit = () - |} - |""".stripMargin, - V.scala3, + Map( + "otherfolder" -> + SbtBuildLayout( + s"""/a/src/main/scala/OtherMain.scala + |package a + |class A { + | def foo(): Unit = () + |} + |/b/src/main/scala/OtherMain.scala + |package b + |class B { + | def foo(): Unit = () + |} + |""".stripMargin, + V.scala3, + ), + "somefolder" -> + SbtBuildLayout( + s"""|/a/src/main/scala/Main.scala + |package a + |class A { + | def foo(): Unit = () + |} + |/b/src/main/scala/Main.scala + |package b + |class B { + | def foo(): Unit = () + |} + |""".stripMargin, + V.scala3, + ), ), "a", // buildTarget, see: SbtBuildLayout Right(FileDecoderProviderLspSuite.sbtBuildTargetResponse), @@ -454,6 +486,7 @@ class FileDecoderProviderSbtLspSuite result, Set("Target", "Scala Version", "Base Directory", "Source Directories"), ), + "somefolder", ) check( @@ -497,23 +530,39 @@ class FileDecoderProviderSbtLspSuite trait FileDecoderProviderLspSpec { self: BaseLspSuite => + def checkBuildTarget( + testName: TestOptions, + input: String, + buildTarget: String, + expected: Either[String, String], + transformResult: String => String, + ): Unit = checkBuildTarget( + testName, + Map("somefolder" -> input), + buildTarget, + expected, + transformResult, + "somefolder", + ) + /** * @param expected - we can use "@workspace" to represent the workspace directory * (can't receive it via params because it will be set at "initialize" request) */ def checkBuildTarget( testName: TestOptions, - input: String, + input: Map[String, String], buildTarget: String, expected: Either[String, String], - transformResult: String => String = identity, + transformResult: String => String, + folderName: String, ): Unit = { test(testName) { for { - _ <- initialize(input) + _ <- initialize(input, expectError = false) result <- server.executeDecodeFileCommand( FileDecoderProvider - .createBuildTargetURI(workspace, buildTarget) + .createBuildTargetURI(workspace.resolve(folderName), buildTarget) .toString ) } yield { @@ -538,6 +587,26 @@ trait FileDecoderProviderLspSpec { self: BaseLspSuite => expected: Either[String, String], transformResult: String => String = identity, customUri: Option[String] = None, + ): Unit = check( + testName, + Map("somefolder" -> input), + s"somefolder/$filePath", + picked, + expected, + transformResult, + customUri.getOrElse( + s"metalsDecode:file://$workspace/somefolder/$filePath.$extension" + ), + ) + + def check( + testName: TestOptions, + input: Map[String, String], + filePath: String, + picked: Option[String], + expected: Either[String, String], + transformResult: String => String, + uri: => String, ): Unit = { test(testName) { cleanWorkspace() @@ -547,13 +616,9 @@ trait FileDecoderProviderLspSpec { self: BaseLspSuite => } } for { - _ <- initialize(input) + _ <- initialize(input, expectError = false) _ <- server.didOpen(filePath) - result <- server.executeDecodeFileCommand( - customUri.getOrElse( - s"metalsDecode:file://$workspace/$filePath.$extension" - ) - ) + result <- server.executeDecodeFileCommand(uri) } yield { assertEquals( if (result.value != null) Right(transformResult(result.value)) @@ -1195,7 +1260,7 @@ object FileDecoderProviderLspSuite { | ${V.scala3} | |Base Directory - | file://@workspace/a[2.13.8]/""".stripMargin + | file://@workspace/somefolder/a[2.13.8]/""".stripMargin def sbtBuildTargetResponse: String = s"""|Target @@ -1205,11 +1270,11 @@ object FileDecoderProviderLspSuite { | ${V.scala3} | |Base Directory - | file:@workspace/a/ + | file:@workspace/somefolder/a/ | |Source Directories - | @workspace/a/src/main/java - | @workspace/a/src/main/scala - | @workspace/a/src/main/scala-3 - | @workspace/a/target/scala-${V.scala3}/src_managed/main (generated)""".stripMargin + | @workspace/somefolder/a/src/main/java + | @workspace/somefolder/a/src/main/scala + | @workspace/somefolder/a/src/main/scala-3 + | @workspace/somefolder/a/target/scala-${V.scala3}/src_managed/main (generated)""".stripMargin } diff --git a/tests/unit/src/main/scala/tests/BuildServerInitializer.scala b/tests/unit/src/main/scala/tests/BuildServerInitializer.scala index 0427f1db0ab..651768c4f9a 100644 --- a/tests/unit/src/main/scala/tests/BuildServerInitializer.scala +++ b/tests/unit/src/main/scala/tests/BuildServerInitializer.scala @@ -97,13 +97,18 @@ object SbtServerInitializer extends BuildServerInitializer { expectError: Boolean, workspaceFolders: List[String] = Nil, )(implicit ec: ExecutionContext): Future[InitializeResult] = { - val sbtVersion = - SbtBuildTool - .loadVersion(workspace) - .getOrElse(V.sbtVersion) - generateBspConfig(workspace, sbtVersion) + val paths = + if (workspaceFolders.isEmpty) List(workspace) + else workspaceFolders.map(workspace.resolve) + paths.foreach { path => + val sbtVersion = + SbtBuildTool + .loadVersion(path) + .getOrElse(V.sbtVersion) + generateBspConfig(path, sbtVersion) + } for { - initializeResult <- server.initialize() + initializeResult <- server.initialize(workspaceFolders) _ <- server.initialized() // choose sbt as the Bsp Server _ = client.selectBspServer = { items => @@ -113,7 +118,13 @@ object SbtServerInitializer extends BuildServerInitializer { ) } } - _ <- server.executeCommand(ServerCommands.BspSwitch) + _ <- paths.zipWithIndex.foldLeft(Future.successful(())) { + case (future, (_, i)) => + future.flatMap { _ => + client.chooseWorkspaceFolder = { _(i) } + server.executeCommand(ServerCommands.BspSwitch).ignoreValue + } + } } yield { if (!expectError) { server.assertBuildServerConnection() diff --git a/tests/unit/src/main/scala/tests/TestingClient.scala b/tests/unit/src/main/scala/tests/TestingClient.scala index eaa0f623542..16d3da53cd0 100644 --- a/tests/unit/src/main/scala/tests/TestingClient.scala +++ b/tests/unit/src/main/scala/tests/TestingClient.scala @@ -75,6 +75,8 @@ class TestingClient(workspace: AbsolutePath, val buffers: Buffers) var selectBspServer: Seq[MessageActionItem] => MessageActionItem = { _ => null } + var chooseWorkspaceFolder: Seq[MessageActionItem] => MessageActionItem = + _.head var chooseBuildTool: Seq[MessageActionItem] => MessageActionItem = { actions => actions @@ -378,6 +380,10 @@ class TestingClient(workspace: AbsolutePath, val buffers: Buffers) .logMessage(Icons.default) ) { new MessageActionItem("ok") + } else if ( + params.getMessage().startsWith("For which folder would you like to") + ) { + chooseWorkspaceFolder(params.getActions().asScala.toSeq) } else { throw new IllegalArgumentException(params.toString) } diff --git a/tests/unit/src/test/scala/tests/JavaDefinitionSuite.scala b/tests/unit/src/test/scala/tests/JavaDefinitionSuite.scala index 3078498e2af..03b8463f8d1 100644 --- a/tests/unit/src/test/scala/tests/JavaDefinitionSuite.scala +++ b/tests/unit/src/test/scala/tests/JavaDefinitionSuite.scala @@ -74,6 +74,8 @@ class JavaDefinitionSuite extends BaseLspSuite("java-definition") { dependencies = List( "org.jboss.xnio:xnio-nio:3.8.11.Final" ), + withoutVirtualDocs = true, + useWorkspaceFolders = true, ) check( @@ -98,6 +100,7 @@ class JavaDefinitionSuite extends BaseLspSuite("java-definition") { expected: String, dependencies: List[String] = Nil, withoutVirtualDocs: Boolean = false, + useWorkspaceFolders: Boolean = false, )(implicit loc: Location): Unit = { test(name, withoutVirtualDocs) { val parsed = FileLayout.mapFromString(input) @@ -107,7 +110,7 @@ class JavaDefinitionSuite extends BaseLspSuite("java-definition") { val deps = dependencies .map(s => s""""$s"""") .mkString("[", ", ", "]") - val layout = + def simpleLayout = s"""|/metals.json |{ | "a": { @@ -115,10 +118,24 @@ class JavaDefinitionSuite extends BaseLspSuite("java-definition") { | } |} |""".stripMargin + + def workspaceFoldersLayout = + Map( + "otherfolder" -> + s"""|/metals.json + |{ + | "a": { } + |} + |""".stripMargin, + "somefolder" -> simpleLayout, + ) for { - _ <- initialize(layout) + _ <- + if (useWorkspaceFolders) + initialize(workspaceFoldersLayout, expectError = false) + else initialize(simpleLayout) // trigger extraction into readonly - info = server.server.workspaceSymbol(depSymbol) + info = server.fullServer.workspaceSymbol(depSymbol) matchedInfo = info.find(_.getLocation().getUri().contains(path)) uri = matchedInfo match { case None => @@ -138,7 +155,12 @@ class JavaDefinitionSuite extends BaseLspSuite("java-definition") { ) ) .asScala - rendered = locations.asScala.map(renderLocation).mkString("\n") + folderRoot = + if (useWorkspaceFolders) workspace.resolve("somefolder") + else workspace + rendered = locations.asScala + .map(renderLocation(_, folderRoot)) + .mkString("\n") _ = assertNoDiff(rendered, expected) } yield () } @@ -171,14 +193,17 @@ class JavaDefinitionSuite extends BaseLspSuite("java-definition") { } } - private def renderLocation(loc: l.Location): String = { + private def renderLocation( + loc: l.Location, + folderRoot: AbsolutePath = workspace, + ): String = { val path = AbsolutePath.fromAbsoluteUri(URI.create(loc.getUri())) val relativePath = path.jarPath .map(jarPath => s"${jarPath.filename}${path}") .getOrElse( path - .toRelative(workspace.resolve(Directories.dependencies)) + .toRelative(folderRoot.resolve(Directories.dependencies)) .toString() ) val input = path.toInput.copy(path = relativePath.replace("\\", "/"))