Skip to content

Commit

Permalink
bugfix: Compile only existing classes
Browse files Browse the repository at this point in the history
Previously, we would ask zinc to compile even a file that was removed and I think this was causing scalameta/metals#6478

Now, we invalidated all changed sources, but only compile new ones.
  • Loading branch information
tgodzik committed Nov 1, 2024
1 parent ede1b04 commit 054f826
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
}
}
}
}
Expand Down Expand Up @@ -231,21 +238,22 @@ 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],
classfileManager: ClassFileManager
): 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
Expand Down
71 changes: 71 additions & 0 deletions frontend/src/test/scala/bloop/bsp/BspCompileSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 054f826

Please sign in to comment.