From 96ae4489a7e5e4d86b1f81bcedf85c08835f6693 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Thu, 5 Dec 2024 14:31:10 +0100 Subject: [PATCH 1/7] improvement: ignore compile request for file if it did not change --- .../meta/internal/metals/Compilations.scala | 29 ++++++++------- .../internal/metals/SavedFileSignatures.scala | 36 +++++++++++++++++++ .../scala/tests/sbt/CancelCompileSuite.scala | 2 +- .../src/test/scala/tests/BillLspSuite.scala | 4 +-- .../scala/tests/CancelCompileLspSuite.scala | 2 +- 5 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index 609fbd46c72..c0f2af0b825 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -34,6 +34,7 @@ final class Compilations( downstreamTargets: PreviouslyCompiledDownsteamTargets, bestEffortEnabled: Boolean, )(implicit ec: ExecutionContext) { + private val fileSignatures = new SavedFileSignatures private val compileTimeout: Timeout = Timeout("compile", Duration(10, TimeUnit.MINUTES)) private val cascadeTimeout: Timeout = @@ -106,18 +107,20 @@ final class Compilations( compileBatch(targets).ignoreValue } - def compileFile(path: AbsolutePath): Future[b.CompileResult] = { - def empty = new b.CompileResult(b.StatusCode.CANCELLED) - for { - targetOpt <- expand(path) - result <- targetOpt match { - case None => Future.successful(empty) - case Some(target) => - compileBatch(target) - .map(res => res.getOrElse(target, empty)) - } - _ <- compileWorksheets(Seq(path)) - } yield result + def compileFile(path: AbsolutePath): Future[Option[b.CompileResult]] = { + if(fileSignatures.didSavedContentChanged(path)) { + def empty = new b.CompileResult(b.StatusCode.CANCELLED) + for { + targetOpt <- expand(path) + result <- targetOpt match { + case None => Future.successful(empty) + case Some(target) => + compileBatch(target) + .map(res => res.getOrElse(target, empty)) + } + _ <- compileWorksheets(Seq(path)) + } yield Some(result) + } else Future.successful(None) } def compileFiles( @@ -125,7 +128,7 @@ final class Compilations( focusedDocumentBuildTarget: Option[BuildTargetIdentifier], ): Future[Unit] = { for { - targets <- expand(paths) + targets <- expand(paths.filter(fileSignatures.didSavedContentChanged)) _ <- compileBatch(targets) _ <- focusedDocumentBuildTarget match { case Some(bt) diff --git a/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala b/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala new file mode 100644 index 00000000000..556f6f6f4b2 --- /dev/null +++ b/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala @@ -0,0 +1,36 @@ +package scala.meta.internal.metals + +import java.io.IOException + +import scala.collection.concurrent.TrieMap + +import scala.meta.internal.io.FileIO +import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.mtags.MD5 +import scala.meta.io.AbsolutePath + +class SavedFileSignatures { + private val previousCreateOrModify = TrieMap[AbsolutePath, String]() + + def didSavedContentChanged(path: AbsolutePath): Boolean = { + try { + val md5 = + if (path.exists) Some(MD5.bytesToHex(FileIO.readAllBytes(path))) + else None + synchronized { + if (previousCreateOrModify.get(path) == md5) false + else { + md5 match { + case None => previousCreateOrModify.remove(path) + case Some(md5) => previousCreateOrModify.put(path, md5) + } + true + } + } + } catch { + case e: IOException => + scribe.warn(s"Failed to read contents of $path", e) + true + } + } +} diff --git a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala index 932994d0d21..f7370bb2cea 100644 --- a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala +++ b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala @@ -66,7 +66,7 @@ class CancelCompileSuite server.executeCommand(ServerCommands.CancelCompile) } _ = assertNoDiff(client.workspaceDiagnostics, "") - _ = assertEquals(compileReport.getStatusCode(), StatusCode.CANCELLED) + _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( workspace.resolve("c/src/main/scala/c/C.scala") ) diff --git a/tests/unit/src/test/scala/tests/BillLspSuite.scala b/tests/unit/src/test/scala/tests/BillLspSuite.scala index 7e0dd17f828..335c0a62336 100644 --- a/tests/unit/src/test/scala/tests/BillLspSuite.scala +++ b/tests/unit/src/test/scala/tests/BillLspSuite.scala @@ -252,7 +252,7 @@ class BillLspSuite extends BaseLspSuite("bill") { } server.executeCommand(ServerCommands.CancelCompile) } - _ = assertEquals(compileReport.getStatusCode(), StatusCode.CANCELLED) + _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) // wait for all the side effect (`onComplete`) actions of cancellation to happen _ = Thread.sleep(1000) currentTrace = trace @@ -264,7 +264,7 @@ class BillLspSuite extends BaseLspSuite("bill") { .compileFile( workspace.resolve("src/com/App.scala") ) - _ = assertEquals(compileReport.getStatusCode(), StatusCode.OK) + _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.OK) } yield () } } diff --git a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala index f44dec2380c..52992251c3b 100644 --- a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala +++ b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala @@ -56,7 +56,7 @@ class CancelCompileLspSuite extends BaseLspSuite("compile-cancel") { server.executeCommand(ServerCommands.CancelCompile) } _ = assertNoDiff(client.workspaceDiagnostics, "") - _ = assertEquals(compileReport.getStatusCode(), StatusCode.CANCELLED) + _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( workspace.resolve("c/src/main/scala/c/C.scala") ) From 00662d1db1eef9a087e1dbb78e88508ceba6e826 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Thu, 5 Dec 2024 15:08:26 +0100 Subject: [PATCH 2/7] deduplicate reading file content --- .../meta/internal/metals/Compilations.scala | 15 +++-- .../internal/metals/MetalsLspService.scala | 36 +++++++---- .../metals/ProjectMetalsLspService.scala | 4 +- .../internal/metals/SavedFileSignatures.scala | 64 ++++++++++++++----- .../scala/tests/sbt/CancelCompileSuite.scala | 5 +- .../test/scala/tests/sbt/SbtServerSuite.scala | 3 +- .../src/main/scala/tests/TestingServer.scala | 9 ++- .../src/test/scala/tests/BillLspSuite.scala | 5 +- .../scala/tests/CancelCompileLspSuite.scala | 5 +- .../tests/UnsupportedDebuggingLspSuite.scala | 5 +- 10 files changed, 106 insertions(+), 45 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index c0f2af0b825..c91f8c5de7e 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -107,28 +107,30 @@ final class Compilations( compileBatch(targets).ignoreValue } - def compileFile(path: AbsolutePath): Future[Option[b.CompileResult]] = { - if(fileSignatures.didSavedContentChanged(path)) { + def compileFile(path: PathWithContent): Future[Option[b.CompileResult]] = { + if (fileSignatures.didSavedContentChanged(path)) { def empty = new b.CompileResult(b.StatusCode.CANCELLED) for { - targetOpt <- expand(path) + targetOpt <- expand(path.path) result <- targetOpt match { case None => Future.successful(empty) case Some(target) => compileBatch(target) .map(res => res.getOrElse(target, empty)) } - _ <- compileWorksheets(Seq(path)) + _ <- compileWorksheets(Seq(path.path)) } yield Some(result) } else Future.successful(None) } def compileFiles( - paths: Seq[AbsolutePath], + pathsWithContent: Seq[PathWithContent], focusedDocumentBuildTarget: Option[BuildTargetIdentifier], ): Future[Unit] = { + val paths = + pathsWithContent.filter(fileSignatures.didSavedContentChanged).map(_.path) for { - targets <- expand(paths.filter(fileSignatures.didSavedContentChanged)) + targets <- expand(paths) _ <- compileBatch(targets) _ <- focusedDocumentBuildTarget match { case Some(bt) @@ -159,6 +161,7 @@ final class Compilations( lastCompile = Set.empty cascadeBatch.cancelAll() compileBatch.cancelAll() + fileSignatures.cancel() } def recompileAll(): Future[Unit] = { diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index bf467ec561d..7d4e7d2c51b 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -738,8 +738,9 @@ abstract class MetalsLspService( else current } + val content = FileIO.readAllBytes(path) // Update md5 fingerprint from file contents on disk - fingerprints.add(path, FileIO.slurp(path, charset)) + fingerprints.add(path, new String(content, charset)) // Update in-memory buffer contents from LSP client buffers.put(path, params.getTextDocument.getText) @@ -772,7 +773,10 @@ abstract class MetalsLspService( Future .sequence( List( - maybeCompileOnDidFocus(path, prevBuildTarget), + maybeCompileOnDidFocus( + PathWithContent(path, content), + prevBuildTarget, + ), compilers.load(List(path)), parser, interactive, @@ -808,20 +812,21 @@ abstract class MetalsLspService( } else if (recentlyOpenedFiles.isRecentlyActive(path)) { CompletableFuture.completedFuture(DidFocusResult.RecentlyActive) } else { - maybeCompileOnDidFocus(path, prevBuildTarget).asJava + worksheetProvider.onDidFocus(path) + maybeCompileOnDidFocus(PathWithContent(path), prevBuildTarget).asJava } } protected def maybeCompileOnDidFocus( - path: AbsolutePath, + path: PathWithContent, prevBuildTarget: b.BuildTargetIdentifier, ): Future[DidFocusResult.Value] = - buildTargets.inverseSources(path) match { + buildTargets.inverseSources(path.path) match { case Some(target) if prevBuildTarget != target => compilations .compileFile(path) .map(_ => DidFocusResult.Compiled) - case _ if path.isWorksheet => + case _ if path.path.isWorksheet => compilations .compileFile(path) .map(_ => DidFocusResult.Compiled) @@ -918,16 +923,22 @@ abstract class MetalsLspService( } protected def onChange(paths: Seq[AbsolutePath]): Future[Unit] = { - paths.foreach { path => - fingerprints.add(path, FileIO.slurp(path, charset)) - } + val pathsWithContent = + paths.map { path => + val content = FileIO.readAllBytes(path) + fingerprints.add(path, new String(content, charset)) + PathWithContent(path, content) + } Future .sequence( List( Future(indexer.reindexWorkspaceSources(paths)), compilations - .compileFiles(paths, Option(focusedDocumentBuildTarget.get())), + .compileFiles( + pathsWithContent, + Option(focusedDocumentBuildTarget.get()), + ), ) ++ paths.map(f => Future(interactiveSemanticdbs.textDocument(f))) ) .ignoreValue @@ -938,7 +949,10 @@ abstract class MetalsLspService( .sequence( List( compilations - .compileFiles(List(path), Option(focusedDocumentBuildTarget.get())), + .compileFiles( + List(PathWithContent.deleted(path)), + Option(focusedDocumentBuildTarget.get()), + ), Future { diagnostics.didDelete(path) testProvider.onFileDelete(path) diff --git a/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala index 8629f7579d4..3dfe0f2a105 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala @@ -607,7 +607,9 @@ class ProjectMetalsLspService( for { _ <- connect(ImportBuildAndIndex(session)) } { - focusedDocument().foreach(path => compilations.compileFile(path)) + focusedDocument().foreach(path => + compilations.compileFile(PathWithContent(path)) + ) } } } diff --git a/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala b/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala index 556f6f6f4b2..453915362bc 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala @@ -12,25 +12,55 @@ import scala.meta.io.AbsolutePath class SavedFileSignatures { private val previousCreateOrModify = TrieMap[AbsolutePath, String]() - def didSavedContentChanged(path: AbsolutePath): Boolean = { - try { - val md5 = - if (path.exists) Some(MD5.bytesToHex(FileIO.readAllBytes(path))) - else None - synchronized { - if (previousCreateOrModify.get(path) == md5) false - else { - md5 match { - case None => previousCreateOrModify.remove(path) - case Some(md5) => previousCreateOrModify.put(path, md5) + def didSavedContentChanged(pathWithContent: PathWithContent): Boolean = { + val path = pathWithContent.path + pathWithContent + .getSignature() + .map { md5 => + synchronized { + if (previousCreateOrModify.get(path) == md5) false + else { + md5 match { + case None => previousCreateOrModify.remove(path) + case Some(md5) => previousCreateOrModify.put(path, md5) + } + true } - true } } - } catch { - case e: IOException => - scribe.warn(s"Failed to read contents of $path", e) - true - } + .getOrElse(true) } + + def cancel(): Unit = previousCreateOrModify.clear() +} + +class PathWithContent( + val path: AbsolutePath, + optContent: Option[PathWithContent.Content], +) { + def getSignature(): Either[IOException, Option[String]] = { + optContent + .map(_.map(content => MD5.bytesToHex(content))) + .map(Right[IOException, Option[String]](_)) + .getOrElse { + try { + if (path.exists) + Right(Some(MD5.bytesToHex(FileIO.readAllBytes(path)))) + else Right(None) + } catch { + case e: IOException => + scribe.warn(s"Failed to read contents of $path", e) + Left(e) + } + } + } +} + +object PathWithContent { + // None if the file doesn't exist + type Content = Option[Array[Byte]] + def apply(path: AbsolutePath) = new PathWithContent(path, None) + def apply(path: AbsolutePath, content: Array[Byte]) = + new PathWithContent(path, Some(Some(content))) + def deleted(path: AbsolutePath) = new PathWithContent(path, Some(None)) } diff --git a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala index f7370bb2cea..cbd08ea62b5 100644 --- a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala +++ b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala @@ -1,6 +1,7 @@ package tests import scala.meta.internal.metals.MetalsServerConfig +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import scala.meta.internal.metals.{BuildInfo => V} @@ -58,7 +59,7 @@ class CancelCompileSuite _ <- server.server.buildServerPromise.future (compileReport, _) <- server.server.compilations .compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) .zip { // wait until the compilation start @@ -68,7 +69,7 @@ class CancelCompileSuite _ = assertNoDiff(client.workspaceDiagnostics, "") _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) _ = assertNoDiff( client.workspaceDiagnostics, diff --git a/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala b/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala index fc456dd1c67..c1b1a53e5d2 100644 --- a/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala +++ b/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala @@ -9,6 +9,7 @@ import scala.meta.internal.metals.CreateSession import scala.meta.internal.metals.Messages import scala.meta.internal.metals.Messages.ImportBuildChanges import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import scala.meta.internal.metals.{BuildInfo => V} import scala.meta.internal.process.SystemProcess @@ -454,7 +455,7 @@ class SbtServerSuite _ <- initialize(layout) // make sure to compile once _ <- server.server.compilations.compileFile( - workspace.resolve("target/Foo.scala") + PathWithContent(workspace.resolve("target/Foo.scala")) ) } yield { // Sleep 100 ms: that should be enough to see the compilation looping diff --git a/tests/unit/src/main/scala/tests/TestingServer.scala b/tests/unit/src/main/scala/tests/TestingServer.scala index 544cb8f2a5d..5c1c24ada9c 100644 --- a/tests/unit/src/main/scala/tests/TestingServer.scala +++ b/tests/unit/src/main/scala/tests/TestingServer.scala @@ -1055,7 +1055,10 @@ final case class TestingServer( val compilations = paths.map(path => - fullServer.getServiceFor(path).compilations.compileFile(path) + fullServer + .getServiceFor(path) + .compilations + .compileFile(m.internal.metals.PathWithContent(path)) ) for { @@ -1117,7 +1120,9 @@ final case class TestingServer( codeLenses.trySuccess(lenses.toList) else if (retries > 0) { retries -= 1 - server.compilations.compileFile(path) + server.compilations.compileFile( + m.internal.metals.PathWithContent(path) + ) } else { val error = s"Could not fetch any code lenses in $maxRetries tries" codeLenses.tryFailure(new NoSuchElementException(error)) diff --git a/tests/unit/src/test/scala/tests/BillLspSuite.scala b/tests/unit/src/test/scala/tests/BillLspSuite.scala index 335c0a62336..ba3283df438 100644 --- a/tests/unit/src/test/scala/tests/BillLspSuite.scala +++ b/tests/unit/src/test/scala/tests/BillLspSuite.scala @@ -5,6 +5,7 @@ import scala.concurrent.Future import scala.meta.internal.metals.Directories import scala.meta.internal.metals.Messages import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.RecursivelyDelete import scala.meta.internal.metals.ServerCommands import scala.meta.io.AbsolutePath @@ -243,7 +244,7 @@ class BillLspSuite extends BaseLspSuite("bill") { ) (compileReport, _) <- server.server.compilations .compileFile( - workspace.resolve("src/com/App.scala") + PathWithContent(workspace.resolve("src/com/App.scala")) ) .zip { // wait until the compilation start @@ -262,7 +263,7 @@ class BillLspSuite extends BaseLspSuite("bill") { _ = assert(currentTrace.contains(s"buildTarget/compile - ($cancelId)")) compileReport <- server.server.compilations .compileFile( - workspace.resolve("src/com/App.scala") + PathWithContent(workspace.resolve("src/com/App.scala")) ) _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.OK) } yield () diff --git a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala index 52992251c3b..4f9e0e54d3f 100644 --- a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala +++ b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala @@ -1,5 +1,6 @@ package tests +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import ch.epfl.scala.bsp4j.StatusCode @@ -48,7 +49,7 @@ class CancelCompileLspSuite extends BaseLspSuite("compile-cancel") { _ <- server.server.buildServerPromise.future (compileReport, _) <- server.server.compilations .compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) .zip { // wait until the compilation start @@ -58,7 +59,7 @@ class CancelCompileLspSuite extends BaseLspSuite("compile-cancel") { _ = assertNoDiff(client.workspaceDiagnostics, "") _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) _ = assertNoDiff( client.workspaceDiagnostics, diff --git a/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala b/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala index 144255114fe..c58e7a65a1c 100644 --- a/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala +++ b/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala @@ -9,6 +9,7 @@ import scala.util.Success import scala.meta.internal.metals.ClientCommands import scala.meta.internal.metals.InitializationOptions import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals.PathWithContent class UnsupportedDebuggingLspSuite extends BaseLspSuite("unsupported-debugging") { @@ -62,7 +63,9 @@ class UnsupportedDebuggingLspSuite ) _ <- server.server.compilations - .compileFile(server.toPath("a/src/main/scala/Main.scala")) + .compileFile( + PathWithContent(server.toPath("a/src/main/scala/Main.scala")) + ) } yield { val clientCommands = client.clientCommands.asScala.map(_.getCommand).toSet assert(!clientCommands.contains(ClientCommands.RefreshModel.id)) From eb07833cf14c0b1b33fd249f2b1ecbfbd5888af5 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Mon, 9 Dec 2024 17:48:38 +0100 Subject: [PATCH 3/7] don't call no op compilations --- .../meta/internal/metals/Compilations.scala | 60 ++++---- .../meta/internal/metals/FileChanges.scala | 131 ++++++++++++++++++ .../internal/metals/MetalsLspService.scala | 58 ++------ .../metals/MutableMd5Fingerprints.scala | 6 +- .../metals/ProjectMetalsLspService.scala | 4 +- .../internal/metals/SavedFileSignatures.scala | 66 --------- .../scala/tests/sbt/CancelCompileSuite.scala | 7 +- .../test/scala/tests/sbt/SbtServerSuite.scala | 3 +- .../src/main/scala/tests/TestingServer.scala | 6 +- .../src/test/scala/tests/BillLspSuite.scala | 9 +- .../scala/tests/CancelCompileLspSuite.scala | 7 +- .../tests/UnsupportedDebuggingLspSuite.scala | 5 +- 12 files changed, 192 insertions(+), 170 deletions(-) create mode 100644 metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala delete mode 100644 metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index c91f8c5de7e..34a00164cdd 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -34,7 +34,7 @@ final class Compilations( downstreamTargets: PreviouslyCompiledDownsteamTargets, bestEffortEnabled: Boolean, )(implicit ec: ExecutionContext) { - private val fileSignatures = new SavedFileSignatures + private val fileChanges = new FileChanges(buildTargets, workspace) private val compileTimeout: Timeout = Timeout("compile", Duration(10, TimeUnit.MINUTES)) private val cascadeTimeout: Timeout = @@ -96,6 +96,7 @@ final class Compilations( def compileTarget( target: b.BuildTargetIdentifier ): Future[b.CompileResult] = { + fileChanges.willCompile(List(target)) compileBatch(target).map { results => results.getOrElse(target, new b.CompileResult(b.StatusCode.CANCELLED)) } @@ -104,48 +105,52 @@ final class Compilations( def compileTargets( targets: Seq[b.BuildTargetIdentifier] ): Future[Unit] = { + fileChanges.willCompile(targets.toList) compileBatch(targets).ignoreValue } - def compileFile(path: PathWithContent): Future[Option[b.CompileResult]] = { - if (fileSignatures.didSavedContentChanged(path)) { - def empty = new b.CompileResult(b.StatusCode.CANCELLED) - for { - targetOpt <- expand(path.path) - result <- targetOpt match { - case None => Future.successful(empty) - case Some(target) => - compileBatch(target) - .map(res => res.getOrElse(target, empty)) - } - _ <- compileWorksheets(Seq(path.path)) - } yield Some(result) - } else Future.successful(None) + def compileFile( + path: AbsolutePath, + fingerprint: Option[Fingerprint] = None, + assumeDidNotChange: Boolean = false, + ): Future[Option[b.CompileResult]] = { + def empty = new b.CompileResult(b.StatusCode.CANCELLED) + for { + targetOpt <- fileChanges.buildTargetToCompile( + path, + fingerprint, + assumeDidNotChange, + ) + result <- targetOpt match { + case None => Future.successful(empty) + case Some(target) => + compileBatch(target) + .map(res => res.getOrElse(target, empty)) + } + _ <- + if (assumeDidNotChange && targetOpt.isEmpty) Future.successful(()) + else compileWorksheets(Seq(path)) + } yield Some(result) } def compileFiles( - pathsWithContent: Seq[PathWithContent], + paths: Seq[(AbsolutePath, Fingerprint)], focusedDocumentBuildTarget: Option[BuildTargetIdentifier], ): Future[Unit] = { - val paths = - pathsWithContent.filter(fileSignatures.didSavedContentChanged).map(_.path) for { - targets <- expand(paths) + targets <- fileChanges.buildTargetsToCompile( + paths, + focusedDocumentBuildTarget, + ) _ <- compileBatch(targets) - _ <- focusedDocumentBuildTarget match { - case Some(bt) - if !targets.contains(bt) && - buildTargets.isInverseDependency(bt, targets.toList) => - compileBatch(bt) - case _ => Future.successful(()) - } - _ <- compileWorksheets(paths) + _ <- compileWorksheets(paths.map(_._1)) } yield () } def cascadeCompile(targets: Seq[BuildTargetIdentifier]): Future[Unit] = { val inverseDependencyLeaves = targets.flatMap(buildTargets.inverseDependencyLeaves).distinct + fileChanges.willCompile(inverseDependencyLeaves.toList) cascadeBatch(inverseDependencyLeaves).map(_ => ()) } @@ -161,7 +166,6 @@ final class Compilations( lastCompile = Set.empty cascadeBatch.cancelAll() compileBatch.cancelAll() - fileSignatures.cancel() } def recompileAll(): Future[Unit] = { diff --git a/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala new file mode 100644 index 00000000000..5c6b17d272f --- /dev/null +++ b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala @@ -0,0 +1,131 @@ +package scala.meta.internal.metals + +import scala.collection.concurrent.TrieMap +import scala.collection.mutable +import scala.concurrent.ExecutionContext +import scala.concurrent.Future + +import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.io.AbsolutePath + +import ch.epfl.scala.bsp4j.BuildTargetIdentifier + +class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( + implicit ec: ExecutionContext +) { + private val previousCreateOrModify = TrieMap[AbsolutePath, String]() + private val dirtyBuildTargets = mutable.Set[BuildTargetIdentifier]() + + def buildTargetsToCompile( + paths: Seq[(AbsolutePath, Fingerprint)], + focusedDocumentBuildTarget: Option[BuildTargetIdentifier], + ): Future[Seq[BuildTargetIdentifier]] = + for { + toCompile <- paths.foldLeft( + Future.successful(Seq.empty[BuildTargetIdentifier]) + ) { case (toCompile, (path, fingerprint)) => + toCompile.flatMap { acc => + findBuildTargetIfShouldCompile(path, Some(fingerprint)).map(acc ++ _) + } + } + } yield { + val allToCompile = + if (focusedDocumentBuildTarget.exists(dirtyBuildTargets(_))) + toCompile ++ focusedDocumentBuildTarget + else toCompile + willCompile(allToCompile) + allToCompile + } + + def buildTargetToCompile( + path: AbsolutePath, + fingerprint: Option[Fingerprint], + assumeDidNotChange: Boolean, + ): Future[Option[BuildTargetIdentifier]] = { + for { + toCompile <- findBuildTargetIfShouldCompile( + path, + fingerprint, + assumeDidNotChange, + ) + } yield { + willCompile(toCompile.toSeq) + toCompile + } + } + + def willCompile(ids: Seq[BuildTargetIdentifier]): Unit = + buildTargets + .buildTargetTransitiveDependencies(ids.toList) + .foreach(dirtyBuildTargets.remove) + + private def findBuildTargetIfShouldCompile( + path: AbsolutePath, + fingerprint: Option[Fingerprint], + assumeDidNotChange: Boolean = false, + ): Future[Option[BuildTargetIdentifier]] = { + expand(path).map( + _.filter(bt => + dirtyBuildTargets.contains( + bt + ) || (!assumeDidNotChange && didContentChange(path, fingerprint, bt)) + ) + ) + } + + private def expand( + path: AbsolutePath + ): Future[Option[BuildTargetIdentifier]] = { + val isCompilable = + (path.isScalaOrJava || path.isSbt) && + !path.isDependencySource(workspace()) && + !path.isInTmpDirectory(workspace()) + + if (isCompilable) { + val targetOpt = buildTargets.inverseSourcesBsp(path) + targetOpt.foreach { + case tgts if tgts.isEmpty => scribe.warn(s"no build target for: $path") + case _ => + } + + targetOpt + } else + Future.successful(None) + } + + private def didContentChange( + path: AbsolutePath, + fingerprint: Option[Fingerprint], + buildTarget: BuildTargetIdentifier, + ): Boolean = { + val didChange = didContentChange(path, fingerprint) + if (didChange) { + buildTargets + .allInverseDependencies(buildTarget) + .foreach { bt => + if (bt != buildTarget) dirtyBuildTargets.add(bt) + } + } + didChange + } + + private def didContentChange( + path: AbsolutePath, + fingerprint: Option[Fingerprint], + ): Boolean = { + fingerprint + .map { fingerprint => + synchronized { + if (previousCreateOrModify.getOrElse(path, null) == fingerprint.md5) + false + else { + previousCreateOrModify.put(path, fingerprint.md5) + true + } + } + } + .getOrElse(true) + } + + def cancel(): Unit = previousCreateOrModify.clear() +} diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index 7d4e7d2c51b..e9bd250c98f 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -732,15 +732,8 @@ abstract class MetalsLspService( // and we would lose the notion of the focused document recentlyOpenedFiles.add(path) - val prevBuildTarget = focusedDocumentBuildTarget.getAndUpdate { current => - val shouldUpdate = !clientConfig.isDidFocusProvider() || current == null - if (shouldUpdate) buildTargets.inverseSources(path).getOrElse(current) - else current - } - - val content = FileIO.readAllBytes(path) // Update md5 fingerprint from file contents on disk - fingerprints.add(path, new String(content, charset)) + val fingerprint = fingerprints.add(path, FileIO.slurp(path, charset)) // Update in-memory buffer contents from LSP client buffers.put(path, params.getTextDocument.getText) @@ -773,10 +766,7 @@ abstract class MetalsLspService( Future .sequence( List( - maybeCompileOnDidFocus( - PathWithContent(path, content), - prevBuildTarget, - ), + compilations.compileFile(path, Some(fingerprint)), compilers.load(List(path)), parser, interactive, @@ -799,11 +789,6 @@ abstract class MetalsLspService( uri: String ): CompletableFuture[DidFocusResult.Value] = { val path = uri.toAbsolutePath - val prevBuildTarget = focusedDocumentBuildTarget.getAndUpdate { current => - buildTargets - .inverseSources(path) - .getOrElse(current) - } scalaCli.didFocus(path) // Don't trigger compilation on didFocus events under cascade compilation // because save events already trigger compile in inverse dependencies. @@ -812,30 +797,16 @@ abstract class MetalsLspService( } else if (recentlyOpenedFiles.isRecentlyActive(path)) { CompletableFuture.completedFuture(DidFocusResult.RecentlyActive) } else { - worksheetProvider.onDidFocus(path) - maybeCompileOnDidFocus(PathWithContent(path), prevBuildTarget).asJava + compilations + .compileFile(path, assumeDidNotChange = true) + .map( + _.map(_ => DidFocusResult.Compiled) + .getOrElse(DidFocusResult.AlreadyCompiled) + ) + .asJava } } - protected def maybeCompileOnDidFocus( - path: PathWithContent, - prevBuildTarget: b.BuildTargetIdentifier, - ): Future[DidFocusResult.Value] = - buildTargets.inverseSources(path.path) match { - case Some(target) if prevBuildTarget != target => - compilations - .compileFile(path) - .map(_ => DidFocusResult.Compiled) - case _ if path.path.isWorksheet => - compilations - .compileFile(path) - .map(_ => DidFocusResult.Compiled) - case Some(_) => - Future.successful(DidFocusResult.AlreadyCompiled) - case None => - Future.successful(DidFocusResult.NoBuildTarget) - } - override def didChange( params: DidChangeTextDocumentParams ): CompletableFuture[Unit] = { @@ -923,11 +894,10 @@ abstract class MetalsLspService( } protected def onChange(paths: Seq[AbsolutePath]): Future[Unit] = { - val pathsWithContent = + val pathsWithFingerPrints = paths.map { path => - val content = FileIO.readAllBytes(path) - fingerprints.add(path, new String(content, charset)) - PathWithContent(path, content) + val fingerprint = fingerprints.add(path, FileIO.slurp(path, charset)) + (path, fingerprint) } Future @@ -936,7 +906,7 @@ abstract class MetalsLspService( Future(indexer.reindexWorkspaceSources(paths)), compilations .compileFiles( - pathsWithContent, + pathsWithFingerPrints, Option(focusedDocumentBuildTarget.get()), ), ) ++ paths.map(f => Future(interactiveSemanticdbs.textDocument(f))) @@ -950,7 +920,7 @@ abstract class MetalsLspService( List( compilations .compileFiles( - List(PathWithContent.deleted(path)), + List((path, null)), Option(focusedDocumentBuildTarget.get()), ), Future { diff --git a/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala b/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala index 105687cb8f3..3bc468bcec5 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala @@ -33,8 +33,10 @@ final class MutableMd5Fingerprints extends Md5Fingerprints { path: AbsolutePath, text: String, md5: Option[String] = None, - ): Unit = { - add(path, Fingerprint(text, md5.getOrElse(MD5.compute(text)))) + ): Fingerprint = { + val fingerprint = Fingerprint(text, md5.getOrElse(MD5.compute(text))) + add(path, fingerprint) + fingerprint } private def add( diff --git a/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala index 3dfe0f2a105..8629f7579d4 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala @@ -607,9 +607,7 @@ class ProjectMetalsLspService( for { _ <- connect(ImportBuildAndIndex(session)) } { - focusedDocument().foreach(path => - compilations.compileFile(PathWithContent(path)) - ) + focusedDocument().foreach(path => compilations.compileFile(path)) } } } diff --git a/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala b/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala deleted file mode 100644 index 453915362bc..00000000000 --- a/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala +++ /dev/null @@ -1,66 +0,0 @@ -package scala.meta.internal.metals - -import java.io.IOException - -import scala.collection.concurrent.TrieMap - -import scala.meta.internal.io.FileIO -import scala.meta.internal.metals.MetalsEnrichments._ -import scala.meta.internal.mtags.MD5 -import scala.meta.io.AbsolutePath - -class SavedFileSignatures { - private val previousCreateOrModify = TrieMap[AbsolutePath, String]() - - def didSavedContentChanged(pathWithContent: PathWithContent): Boolean = { - val path = pathWithContent.path - pathWithContent - .getSignature() - .map { md5 => - synchronized { - if (previousCreateOrModify.get(path) == md5) false - else { - md5 match { - case None => previousCreateOrModify.remove(path) - case Some(md5) => previousCreateOrModify.put(path, md5) - } - true - } - } - } - .getOrElse(true) - } - - def cancel(): Unit = previousCreateOrModify.clear() -} - -class PathWithContent( - val path: AbsolutePath, - optContent: Option[PathWithContent.Content], -) { - def getSignature(): Either[IOException, Option[String]] = { - optContent - .map(_.map(content => MD5.bytesToHex(content))) - .map(Right[IOException, Option[String]](_)) - .getOrElse { - try { - if (path.exists) - Right(Some(MD5.bytesToHex(FileIO.readAllBytes(path)))) - else Right(None) - } catch { - case e: IOException => - scribe.warn(s"Failed to read contents of $path", e) - Left(e) - } - } - } -} - -object PathWithContent { - // None if the file doesn't exist - type Content = Option[Array[Byte]] - def apply(path: AbsolutePath) = new PathWithContent(path, None) - def apply(path: AbsolutePath, content: Array[Byte]) = - new PathWithContent(path, Some(Some(content))) - def deleted(path: AbsolutePath) = new PathWithContent(path, Some(None)) -} diff --git a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala index cbd08ea62b5..c5722c80e61 100644 --- a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala +++ b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala @@ -1,7 +1,6 @@ package tests import scala.meta.internal.metals.MetalsServerConfig -import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import scala.meta.internal.metals.{BuildInfo => V} @@ -58,9 +57,7 @@ class CancelCompileSuite ) _ <- server.server.buildServerPromise.future (compileReport, _) <- server.server.compilations - .compileFile( - PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) - ) + .compileFile(workspace.resolve("c/src/main/scala/c/C.scala")) .zip { // wait until the compilation start Thread.sleep(1000) @@ -69,7 +66,7 @@ class CancelCompileSuite _ = assertNoDiff(client.workspaceDiagnostics, "") _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( - PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) + workspace.resolve("c/src/main/scala/c/C.scala") ) _ = assertNoDiff( client.workspaceDiagnostics, diff --git a/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala b/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala index c1b1a53e5d2..fc456dd1c67 100644 --- a/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala +++ b/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala @@ -9,7 +9,6 @@ import scala.meta.internal.metals.CreateSession import scala.meta.internal.metals.Messages import scala.meta.internal.metals.Messages.ImportBuildChanges import scala.meta.internal.metals.MetalsEnrichments._ -import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import scala.meta.internal.metals.{BuildInfo => V} import scala.meta.internal.process.SystemProcess @@ -455,7 +454,7 @@ class SbtServerSuite _ <- initialize(layout) // make sure to compile once _ <- server.server.compilations.compileFile( - PathWithContent(workspace.resolve("target/Foo.scala")) + workspace.resolve("target/Foo.scala") ) } yield { // Sleep 100 ms: that should be enough to see the compilation looping diff --git a/tests/unit/src/main/scala/tests/TestingServer.scala b/tests/unit/src/main/scala/tests/TestingServer.scala index 5c1c24ada9c..d28efafa9bd 100644 --- a/tests/unit/src/main/scala/tests/TestingServer.scala +++ b/tests/unit/src/main/scala/tests/TestingServer.scala @@ -1058,7 +1058,7 @@ final case class TestingServer( fullServer .getServiceFor(path) .compilations - .compileFile(m.internal.metals.PathWithContent(path)) + .compileFile(path) ) for { @@ -1120,9 +1120,7 @@ final case class TestingServer( codeLenses.trySuccess(lenses.toList) else if (retries > 0) { retries -= 1 - server.compilations.compileFile( - m.internal.metals.PathWithContent(path) - ) + server.compilations.compileFile(path) } else { val error = s"Could not fetch any code lenses in $maxRetries tries" codeLenses.tryFailure(new NoSuchElementException(error)) diff --git a/tests/unit/src/test/scala/tests/BillLspSuite.scala b/tests/unit/src/test/scala/tests/BillLspSuite.scala index ba3283df438..acbcd74b1d1 100644 --- a/tests/unit/src/test/scala/tests/BillLspSuite.scala +++ b/tests/unit/src/test/scala/tests/BillLspSuite.scala @@ -5,7 +5,6 @@ import scala.concurrent.Future import scala.meta.internal.metals.Directories import scala.meta.internal.metals.Messages import scala.meta.internal.metals.MetalsEnrichments._ -import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.RecursivelyDelete import scala.meta.internal.metals.ServerCommands import scala.meta.io.AbsolutePath @@ -243,9 +242,7 @@ class BillLspSuite extends BaseLspSuite("bill") { |""".stripMargin ) (compileReport, _) <- server.server.compilations - .compileFile( - PathWithContent(workspace.resolve("src/com/App.scala")) - ) + .compileFile(workspace.resolve("src/com/App.scala")) .zip { // wait until the compilation start while (!trace.contains(s"buildTarget/compile")) { @@ -262,9 +259,7 @@ class BillLspSuite extends BaseLspSuite("bill") { cancelId = cancelMatch.get.group(1) _ = assert(currentTrace.contains(s"buildTarget/compile - ($cancelId)")) compileReport <- server.server.compilations - .compileFile( - PathWithContent(workspace.resolve("src/com/App.scala")) - ) + .compileFile(workspace.resolve("src/com/App.scala")) _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.OK) } yield () } diff --git a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala index 4f9e0e54d3f..42fe4cac273 100644 --- a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala +++ b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala @@ -1,6 +1,5 @@ package tests -import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import ch.epfl.scala.bsp4j.StatusCode @@ -48,9 +47,7 @@ class CancelCompileLspSuite extends BaseLspSuite("compile-cancel") { ) _ <- server.server.buildServerPromise.future (compileReport, _) <- server.server.compilations - .compileFile( - PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) - ) + .compileFile(workspace.resolve("c/src/main/scala/c/C.scala")) .zip { // wait until the compilation start Thread.sleep(1000) @@ -59,7 +56,7 @@ class CancelCompileLspSuite extends BaseLspSuite("compile-cancel") { _ = assertNoDiff(client.workspaceDiagnostics, "") _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( - PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) + workspace.resolve("c/src/main/scala/c/C.scala") ) _ = assertNoDiff( client.workspaceDiagnostics, diff --git a/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala b/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala index c58e7a65a1c..144255114fe 100644 --- a/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala +++ b/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala @@ -9,7 +9,6 @@ import scala.util.Success import scala.meta.internal.metals.ClientCommands import scala.meta.internal.metals.InitializationOptions import scala.meta.internal.metals.MetalsEnrichments._ -import scala.meta.internal.metals.PathWithContent class UnsupportedDebuggingLspSuite extends BaseLspSuite("unsupported-debugging") { @@ -63,9 +62,7 @@ class UnsupportedDebuggingLspSuite ) _ <- server.server.compilations - .compileFile( - PathWithContent(server.toPath("a/src/main/scala/Main.scala")) - ) + .compileFile(server.toPath("a/src/main/scala/Main.scala")) } yield { val clientCommands = client.clientCommands.asScala.map(_.getCommand).toSet assert(!clientCommands.contains(ClientCommands.RefreshModel.id)) From f8de3d555663a0ae157fea0da3b10da402f27e7e Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Tue, 10 Dec 2024 12:32:25 +0100 Subject: [PATCH 4/7] fixes --- .../meta/internal/metals/Compilations.scala | 16 +++------- .../meta/internal/metals/FileChanges.scala | 13 +++++--- .../meta/internal/metals/IndexProviders.scala | 1 + .../scala/meta/internal/metals/Indexer.scala | 6 ++++ .../internal/metals/MetalsLspService.scala | 31 ++++++++++--------- 5 files changed, 38 insertions(+), 29 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index 34a00164cdd..f65c3ba441b 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -27,14 +27,14 @@ final class Compilations( languageClient: MetalsLanguageClient, refreshTestSuites: () => Unit, afterSuccessfulCompilation: () => Unit, - isCurrentlyFocused: b.BuildTargetIdentifier => Boolean, + buildtargetInFocus: () => Option[b.BuildTargetIdentifier], compileWorksheets: Seq[AbsolutePath] => Future[Unit], onStartCompilation: () => Unit, userConfiguration: () => UserConfiguration, downstreamTargets: PreviouslyCompiledDownsteamTargets, + fileChanges: FileChanges, bestEffortEnabled: Boolean, )(implicit ec: ExecutionContext) { - private val fileChanges = new FileChanges(buildTargets, workspace) private val compileTimeout: Timeout = Timeout("compile", Duration(10, TimeUnit.MINUTES)) private val cascadeTimeout: Timeout = @@ -133,15 +133,9 @@ final class Compilations( } yield Some(result) } - def compileFiles( - paths: Seq[(AbsolutePath, Fingerprint)], - focusedDocumentBuildTarget: Option[BuildTargetIdentifier], - ): Future[Unit] = { + def compileFiles(paths: Seq[(AbsolutePath, Fingerprint)]): Future[Unit] = { for { - targets <- fileChanges.buildTargetsToCompile( - paths, - focusedDocumentBuildTarget, - ) + targets <- fileChanges.buildTargetsToCompile(paths, buildtargetInFocus()) _ <- compileBatch(targets) _ <- compileWorksheets(paths.map(_._1)) } yield () @@ -301,7 +295,7 @@ final class Compilations( targets, () => { refreshTestSuites() - if (targets.exists(isCurrentlyFocused)) { + if (targets.exists(buildtargetInFocus().contains)) { languageClient.refreshModel() } }, diff --git a/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala index 5c6b17d272f..b4502de3cb3 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala @@ -13,9 +13,14 @@ import ch.epfl.scala.bsp4j.BuildTargetIdentifier class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( implicit ec: ExecutionContext ) { - private val previousCreateOrModify = TrieMap[AbsolutePath, String]() + private val previousSignature = TrieMap[AbsolutePath, String]() private val dirtyBuildTargets = mutable.Set[BuildTargetIdentifier]() + def addAllDirty( + targets: IterableOnce[BuildTargetIdentifier] + ): mutable.Set[BuildTargetIdentifier] = + dirtyBuildTargets.addAll(targets) + def buildTargetsToCompile( paths: Seq[(AbsolutePath, Fingerprint)], focusedDocumentBuildTarget: Option[BuildTargetIdentifier], @@ -116,10 +121,10 @@ class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( fingerprint .map { fingerprint => synchronized { - if (previousCreateOrModify.getOrElse(path, null) == fingerprint.md5) + if (previousSignature.getOrElse(path, null) == fingerprint.md5) false else { - previousCreateOrModify.put(path, fingerprint.md5) + previousSignature.put(path, fingerprint.md5) true } } @@ -127,5 +132,5 @@ class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( .getOrElse(true) } - def cancel(): Unit = previousCreateOrModify.clear() + def cancel(): Unit = previousSignature.clear() } diff --git a/metals/src/main/scala/scala/meta/internal/metals/IndexProviders.scala b/metals/src/main/scala/scala/meta/internal/metals/IndexProviders.scala index aee813d94cf..22b466b436b 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/IndexProviders.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/IndexProviders.scala @@ -42,4 +42,5 @@ trait IndexProviders { def folder: AbsolutePath def implementationProvider: ImplementationProvider def resetService(): Unit + def fileChanges: FileChanges } diff --git a/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala b/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala index 90922ae0f92..f25a9b88f86 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala @@ -104,6 +104,12 @@ case class Indexer(indexProviders: IndexProviders)(implicit rc: ReportContext) { data.reset() buildTargetClasses.clear() data.addWorkspaceBuildTargets(importedBuild.workspaceBuildTargets) + fileChanges.addAllDirty( + importedBuild.workspaceBuildTargets + .getTargets() + .map(_.getId()) + .asScala + ) data.addScalacOptions( importedBuild.scalacOptions, bspSession.map(_.mainConnection), diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index e9bd250c98f..e03978387bd 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -190,6 +190,8 @@ abstract class MetalsLspService( val buildTargets: BuildTargets = BuildTargets.from(folder, mainBuildTargetsData, tables) + val fileChanges: FileChanges = new FileChanges(buildTargets, () => folder) + val buildTargetClasses = new BuildTargetClasses(buildTargets) @@ -216,11 +218,12 @@ abstract class MetalsLspService( headDoctor.executeRefreshDoctor() else () }, - buildTarget => focusedDocumentBuildTarget.get() == buildTarget, + () => Option(focusedDocumentBuildTarget.get()), worksheets => onWorksheetChanged(worksheets), onStartCompilation, () => userConfig, downstreamTargets, + fileChanges, clientConfig.initialConfig.enableBestEffort, ) var indexingPromise: Promise[Unit] = Promise[Unit]() @@ -731,9 +734,12 @@ abstract class MetalsLspService( // In some cases like peeking definition didOpen might be followed up by close // and we would lose the notion of the focused document recentlyOpenedFiles.add(path) + focusedDocumentBuildTarget.set( + buildTargets.inverseSources(path).getOrElse(null) + ) // Update md5 fingerprint from file contents on disk - val fingerprint = fingerprints.add(path, FileIO.slurp(path, charset)) + fingerprints.add(path, FileIO.slurp(path, charset)) // Update in-memory buffer contents from LSP client buffers.put(path, params.getTextDocument.getText) @@ -766,7 +772,7 @@ abstract class MetalsLspService( Future .sequence( List( - compilations.compileFile(path, Some(fingerprint)), + compilations.compileFile(path, assumeDidNotChange = true), compilers.load(List(path)), parser, interactive, @@ -790,6 +796,9 @@ abstract class MetalsLspService( ): CompletableFuture[DidFocusResult.Value] = { val path = uri.toAbsolutePath scalaCli.didFocus(path) + focusedDocumentBuildTarget.set( + buildTargets.inverseSources(path).getOrElse(null) + ) // Don't trigger compilation on didFocus events under cascade compilation // because save events already trigger compile in inverse dependencies. if (path.isDependencySource(folder)) { @@ -904,11 +913,7 @@ abstract class MetalsLspService( .sequence( List( Future(indexer.reindexWorkspaceSources(paths)), - compilations - .compileFiles( - pathsWithFingerPrints, - Option(focusedDocumentBuildTarget.get()), - ), + compilations.compileFiles(pathsWithFingerPrints), ) ++ paths.map(f => Future(interactiveSemanticdbs.textDocument(f))) ) .ignoreValue @@ -918,11 +923,7 @@ abstract class MetalsLspService( Future .sequence( List( - compilations - .compileFiles( - List((path, null)), - Option(focusedDocumentBuildTarget.get()), - ), + compilations.compileFiles(List((path, null))), Future { diagnostics.didDelete(path) testProvider.onFileDelete(path) @@ -1191,7 +1192,9 @@ abstract class MetalsLspService( thresholdMillis = 1.second.toMillis, ) { val path = params.getTextDocument.getUri.toAbsolutePath - codeLensProvider.findLenses(path).map(_.toList.asJava) + codeLensProvider.findLenses(path).map(_.toList.asJava).map { found => + found + } } } } From efda2b8c8d0c569e133521b72f12c1998ebc247f Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Tue, 10 Dec 2024 16:44:19 +0100 Subject: [PATCH 5/7] always compile worksheets --- .../main/scala/scala/meta/internal/metals/Compilations.scala | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index f65c3ba441b..58e93c9123f 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -127,9 +127,7 @@ final class Compilations( compileBatch(target) .map(res => res.getOrElse(target, empty)) } - _ <- - if (assumeDidNotChange && targetOpt.isEmpty) Future.successful(()) - else compileWorksheets(Seq(path)) + _ <- compileWorksheets(Seq(path)) } yield Some(result) } From 35f4cc3bde92d379fba9746102c652fa8d4d2a21 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Tue, 10 Dec 2024 17:33:43 +0100 Subject: [PATCH 6/7] test: fix reindex scala-cli test --- .../scala/scala/meta/internal/metals/FileChanges.scala | 8 ++++---- .../src/test/scala/tests/scalacli/ScalaCliSuite.scala | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala index b4502de3cb3..e1d1d9f8c01 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala @@ -13,7 +13,7 @@ import ch.epfl.scala.bsp4j.BuildTargetIdentifier class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( implicit ec: ExecutionContext ) { - private val previousSignature = TrieMap[AbsolutePath, String]() + private val previousSignatures = TrieMap[AbsolutePath, String]() private val dirtyBuildTargets = mutable.Set[BuildTargetIdentifier]() def addAllDirty( @@ -121,10 +121,10 @@ class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( fingerprint .map { fingerprint => synchronized { - if (previousSignature.getOrElse(path, null) == fingerprint.md5) + if (previousSignatures.getOrElse(path, null) == fingerprint.md5) false else { - previousSignature.put(path, fingerprint.md5) + previousSignatures.put(path, fingerprint.md5) true } } @@ -132,5 +132,5 @@ class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( .getOrElse(true) } - def cancel(): Unit = previousSignature.clear() + def cancel(): Unit = previousSignatures.clear() } diff --git a/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala b/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala index 2d402f8381e..671bce45ba7 100644 --- a/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala +++ b/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala @@ -1,6 +1,7 @@ package tests.scalacli import scala.concurrent.Future +import scala.concurrent.Promise import scala.meta.internal.metals.DebugUnresolvedMainClassParams import scala.meta.internal.metals.FileOutOfScalaCliBspScope @@ -502,10 +503,11 @@ class ScalaCliSuite extends BaseScalaCliSuite("3.3.3") { | ^^ |""".stripMargin, ) + _ = server.server.indexingPromise = Promise[Unit]() _ <- server.didChange("Main.scala") { text => text.replace("// >", "//>") } - _ <- server.didSave("Main.scala") + _ <- server.server.indexingPromise.future // cause another compilation to wait on workspace reload, the previous gets cancelled _ <- server.didSave("Main.scala") _ = assertEquals( From 69293ee3a8870ee6a097f8b780a68b47fa238ec0 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Wed, 15 Jan 2025 16:38:30 +0100 Subject: [PATCH 7/7] review changes --- .../meta/internal/metals/Compilations.scala | 31 ++++--------------- .../meta/internal/metals/FileChanges.scala | 9 ++++-- .../internal/metals/MetalsLspService.scala | 7 ++--- .../metals/MutableMd5Fingerprints.scala | 7 ++++- 4 files changed, 21 insertions(+), 33 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index 58e93c9123f..3559a1f9d66 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -23,7 +23,6 @@ import ch.epfl.scala.{bsp4j => b} final class Compilations( buildTargets: BuildTargets, classes: BuildTargetClasses, - workspace: () => AbsolutePath, languageClient: MetalsLanguageClient, refreshTestSuites: () => Unit, afterSuccessfulCompilation: () => Unit, @@ -89,9 +88,11 @@ final class Compilations( source: AbsolutePath, compileInverseDependencies: Boolean, ): Future[Unit] = - expand(source).flatMap(targets => - compilationFinished(targets.toSeq, compileInverseDependencies) - ) + fileChanges + .expand(source) + .flatMap(targets => + compilationFinished(targets.toSeq, compileInverseDependencies) + ) def compileTarget( target: b.BuildTargetIdentifier @@ -196,28 +197,8 @@ final class Compilations( .ignoreValue } - private def expand( - path: AbsolutePath - ): Future[Option[b.BuildTargetIdentifier]] = { - val isCompilable = - (path.isScalaOrJava || path.isSbt) && - !path.isDependencySource(workspace()) && - !path.isInTmpDirectory(workspace()) - - if (isCompilable) { - val targetOpt = buildTargets.inverseSourcesBsp(path) - targetOpt.foreach { - case tgts if tgts.isEmpty => scribe.warn(s"no build target for: $path") - case _ => - } - - targetOpt - } else - Future.successful(None) - } - def expand(paths: Seq[AbsolutePath]): Future[Seq[b.BuildTargetIdentifier]] = { - val expansions = paths.map(expand) + val expansions = paths.map(fileChanges.expand) Future.sequence(expansions).map(_.flatten) } diff --git a/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala index e1d1d9f8c01..f6992c7865e 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/FileChanges.scala @@ -42,6 +42,11 @@ class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( allToCompile } + /** + * The `assumeDidNotChange` should be set when we want to assume that the file did not change + * if it is not present in the `previousSignatures` (opened for the first time), + * we set it to `true` for operations such as `didFocus` or `didOpen`. + */ def buildTargetToCompile( path: AbsolutePath, fingerprint: Option[Fingerprint], @@ -78,7 +83,7 @@ class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( ) } - private def expand( + def expand( path: AbsolutePath ): Future[Option[BuildTargetIdentifier]] = { val isCompilable = @@ -121,7 +126,7 @@ class FileChanges(buildTargets: BuildTargets, workspace: () => AbsolutePath)( fingerprint .map { fingerprint => synchronized { - if (previousSignatures.getOrElse(path, null) == fingerprint.md5) + if (previousSignatures.getOrElse(path, "") == fingerprint.md5) false else { previousSignatures.put(path, fingerprint.md5) diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index e03978387bd..64d0601ad95 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -210,7 +210,6 @@ abstract class MetalsLspService( val compilations: Compilations = new Compilations( buildTargets, buildTargetClasses, - () => folder, languageClient, () => testProvider.refreshTestSuites.apply(()), () => { @@ -923,7 +922,7 @@ abstract class MetalsLspService( Future .sequence( List( - compilations.compileFiles(List((path, null))), + compilations.compileFiles(List((path, Fingerprint.empty))), Future { diagnostics.didDelete(path) testProvider.onFileDelete(path) @@ -1192,9 +1191,7 @@ abstract class MetalsLspService( thresholdMillis = 1.second.toMillis, ) { val path = params.getTextDocument.getUri.toAbsolutePath - codeLensProvider.findLenses(path).map(_.toList.asJava).map { found => - found - } + codeLensProvider.findLenses(path).map(_.toList.asJava) } } } diff --git a/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala b/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala index 3bc468bcec5..b8a09b618fe 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MutableMd5Fingerprints.scala @@ -91,4 +91,9 @@ final class MutableMd5Fingerprints extends Md5Fingerprints { override def toString: String = s"Md5FingerprintProvider($fingerprints)" } -case class Fingerprint(text: String, md5: String) +case class Fingerprint(text: String, md5: String) { + def isEmpty: Boolean = md5.isEmpty() +} +object Fingerprint { + def empty: Fingerprint = Fingerprint("", "") +}