From 054f826c2e22bf1f17d5043fb00ca1c072e8169c Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Thu, 31 Oct 2024 19:16:54 +0100 Subject: [PATCH] bugfix: Compile only existing classes Previously, we would ask zinc to compile even a file that was removed and I think this was causing https://github.com/scalameta/metals/issues/6478 Now, we invalidated all changed sources, but only compile new ones. --- .../inc/bloop/internal/BloopIncremental.scala | 1 + .../inc/bloop/internal/BloopNameHashing.scala | 108 ++++++++++-------- .../test/scala/bloop/bsp/BspCompileSpec.scala | 71 ++++++++++++ 3 files changed, 130 insertions(+), 50 deletions(-) diff --git a/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopIncremental.scala b/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopIncremental.scala index 4fc08d9c9d..2353be3bb1 100644 --- a/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopIncremental.scala +++ b/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopIncremental.scala @@ -115,6 +115,7 @@ object BloopIncremental { val doCompile = (srcs: Set[VirtualFile], changes: DependencyChanges) => { for { callback <- Task.now(callbackBuilder()) + _ = incremental.log.debug("Compiling " + srcs + " with " + changes) _ <- compile(srcs, changes, callback, manager) } yield callback.get } diff --git a/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopNameHashing.scala b/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopNameHashing.scala index d5e4406521..35c4448be4 100644 --- a/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopNameHashing.scala +++ b/backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopNameHashing.scala @@ -90,59 +90,66 @@ private final class BloopNameHashing( case ref: VirtualFileRef => PlainVirtualFileConverter.converter.toVirtualFile(ref) } - recompileClasses(invalidatedSources, binaryChanges, previous, compileTask, manager).flatMap { - current => - // Return immediate analysis as all sources have been recompiled - if (invalidatedSources == allSources) Task.now(current) - else { - val recompiledClasses: Set[String] = { - // Represents classes detected as changed externally and internally (by a previous cycle) - classesToRecompile ++ - // Maps the changed sources by the user to class names we can count as invalidated - initialChangedSources.flatMap(previous.relations.classNames) ++ - initialChangedSources.flatMap(current.relations.classNames) - } + recompileClasses( + invalidatedSources.filter(allSources), + invalidatedSources, + binaryChanges, + previous, + compileTask, + manager + ).flatMap { current => + // Return immediate analysis as all sources have been recompiled + if (invalidatedSources == allSources) Task.now(current) + else { + val recompiledClasses: Set[String] = { + // Represents classes detected as changed externally and internally (by a previous cycle) + classesToRecompile ++ + // Maps the changed sources by the user to class names we can count as invalidated + initialChangedSources.flatMap(previous.relations.classNames) ++ + initialChangedSources.flatMap(current.relations.classNames) + } - val newApiChanges = - detectAPIChanges( - recompiledClasses, - previous.apis.internalAPI, - current.apis.internalAPI - ) - debug("\nChanges:\n" + newApiChanges) - val nextInvalidations = invalidateAfterInternalCompilation( - current, - newApiChanges, + val newApiChanges = + detectAPIChanges( recompiledClasses, - cycleNum >= options.transitiveStep, - IncrementalCommon.comesFromScalaSource(previous.relations, Some(current.relations)) + previous.apis.internalAPI, + current.apis.internalAPI ) + debug("\nChanges:\n" + newApiChanges) + val nextInvalidations = invalidateAfterInternalCompilation( + current, + newApiChanges, + recompiledClasses, + cycleNum >= options.transitiveStep, + IncrementalCommon.comesFromScalaSource(previous.relations, Some(current.relations)) + ) + debug(s"Next invalidations: $nextInvalidations") - val continue = lookup.shouldDoIncrementalCompilation(nextInvalidations, current) + val continue = lookup.shouldDoIncrementalCompilation(nextInvalidations, current) - profiler.registerCycle( - invalidatedClasses, - invalidatedByPackageObjects, - initialChangedSources, - invalidatedSources, - recompiledClasses, - newApiChanges, - nextInvalidations, - continue - ) + profiler.registerCycle( + invalidatedClasses, + invalidatedByPackageObjects, + initialChangedSources, + invalidatedSources, + recompiledClasses, + newApiChanges, + nextInvalidations, + continue + ) - entrypoint( - if (continue) nextInvalidations else Set.empty, - Set.empty, - allSources, - IncrementalCommon.emptyChanges, - lookup, - current, - compileTask, - manager, - cycleNum + 1 - ) - } + entrypoint( + if (continue) nextInvalidations else Set.empty, + Set.empty, + allSources, + IncrementalCommon.emptyChanges, + lookup, + current, + compileTask, + manager, + cycleNum + 1 + ) + } } } } @@ -231,7 +238,8 @@ private final class BloopNameHashing( } def recompileClasses( - sources: Set[VirtualFile], + currentInvalidatedSources: Set[VirtualFile], + invalidatedSources: Set[VirtualFile], binaryChanges: DependencyChanges, previous: Analysis, compileTask: (Set[VirtualFile], DependencyChanges) => Task[Analysis], @@ -239,13 +247,13 @@ private final class BloopNameHashing( ): Task[Analysis] = { val pruned = IncrementalCommon.pruneClassFilesOfInvalidations( - sources, + invalidatedSources, previous, classfileManager, PlainVirtualFileConverter.converter ) debug("********* Pruned: \n" + pruned.relations + "\n*********") - compileTask(sources, binaryChanges).map { fresh => + compileTask(currentInvalidatedSources, binaryChanges).map { fresh => debug("********* Fresh: \n" + fresh.relations + "\n*********") /* This is required for both scala compilation and forked java compilation, despite diff --git a/frontend/src/test/scala/bloop/bsp/BspCompileSpec.scala b/frontend/src/test/scala/bloop/bsp/BspCompileSpec.scala index e2f36d7603..9a7e18177c 100644 --- a/frontend/src/test/scala/bloop/bsp/BspCompileSpec.scala +++ b/frontend/src/test/scala/bloop/bsp/BspCompileSpec.scala @@ -349,6 +349,77 @@ class BspCompileSpec( } } + test("compile incrementally renamed file") { + TestUtil.withinWorkspace { workspace => + def path(suffix: String) = s"/main/scala/scala/meta/internal/metals/Foo$suffix.scala" + object Sources { + val `Foo0.scala` = + s"""${path("0")} + |package scala.meta.internal.metals + | + |final class Foo0(conn: () => String) { + | def foo(s: String): String = s + |} + """.stripMargin + } + + val logger = new RecordingLogger(ansiCodesSupported = false) + val `A` = TestProject(workspace, "a", List(Sources.`Foo0.scala`)) + val projects = List(`A`) + val fooFilePath = `A`.srcFor(path("0")) + writeFile(fooFilePath, Sources.`Foo0.scala`) + + def assertCorrectArtifacts(compiledState: ManagedBspTestState, suffix: String): Unit = { + + val buildProject = compiledState.toTestState.getProjectFor(`A`) + val externalClassesDirA = + compiledState.client.getUniqueClassesDirFor(buildProject, forceGeneration = true) + + val classFilesAfterSuccess = takeDirectorySnapshot(externalClassesDirA) + assertEquals( + classFilesAfterSuccess.map(_.path.toString()), + List(s"/scala/meta/internal/metals/Foo$suffix.class") + ) + } + + loadBspState(workspace, projects, logger) { state => + val firstCompiledState = state.compile(`A`) + assertExitStatus(firstCompiledState, ExitStatus.Ok) + assertValidCompilationState(firstCompiledState, projects) + assertCorrectArtifacts(firstCompiledState, "0") + + // try to rename 10 times the same file + // VS Code will first change the file, then rename it + val last = (1 to 10).foldLeft(firstCompiledState) { (state, num) => + val previousFile = `A`.srcFor(path(s"${num - 1}")) + val newContents = Sources.`Foo0.scala`.replace(s"Foo0", s"Foo$num") + // rename class + writeFile( + previousFile, + newContents + ) + val compiledStateChangedFile = state.compile(`A`) + assertExitStatus(compiledStateChangedFile, ExitStatus.Ok) + assertValidCompilationState(compiledStateChangedFile, projects) + assertCorrectArtifacts(compiledStateChangedFile, s"$num") + + // rename file + val newFile = fooFilePath.getParent.resolve(s"Foo$num.scala") + Files.move(previousFile.underlying, newFile.underlying) + + val compiledStateRenameFile = compiledStateChangedFile.compile(`A`) + assertExitStatus(compiledStateRenameFile, ExitStatus.Ok) + assertValidCompilationState(compiledStateRenameFile, projects) + + assertCorrectArtifacts(compiledStateRenameFile, s"$num") + compiledStateRenameFile + } + + assertValidCompilationState(last, projects) + } + } + } + /** * Checks several variants regarding the previous execution of post * compilation tasks when the compile result is a success and when it is a