Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalidate macro source when its dependency changes #1282

Merged
merged 5 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ private[inc] abstract class IncrementalCommon(
if (isFullCompilation) Set.empty[String]
else
invalidateAfterInternalCompilation(
analysis.relations,
analysis,
newApiChanges,
recompiledClasses,
cycleNum >= options.transitiveStep,
Expand Down Expand Up @@ -451,12 +451,13 @@ private[inc] abstract class IncrementalCommon(
* @return A list of invalidated class names for the next incremental compiler run.
*/
def invalidateAfterInternalCompilation(
relations: Relations,
analysis: Analysis,
changes: APIChanges,
recompiledClasses: Set[String],
invalidateTransitively: Boolean,
isScalaClass: String => Boolean
): Set[String] = {
val relations = analysis.relations
val initial = changes.allModified.toSet
val dependsOnClass = findClassDependencies(_, relations)
val firstClassInvalidation: Set[String] = {
Expand All @@ -477,15 +478,22 @@ private[inc] abstract class IncrementalCommon(
if (secondClassInvalidation.nonEmpty)
log.debug(s"Invalidated due to generated class file collision: ${secondClassInvalidation}")

val newInvalidations = (firstClassInvalidation -- recompiledClasses) ++ secondClassInvalidation
// Invalidate macros that transitively depend on any of the recompiled classes
val thirdClassInvalidation = {
Copy link
Member

@Friendseeker Friendseeker Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in terms of transitive dependency correctness. There's probably still some edge cases left but this did cover the edge cases I can think of.

Preferably someone can do a performance review. If the performance is acceptable then I personally think this is good to go. If not, we might need to think of a way to optimize computation of thirdClassInvalidation (e.g. 2-way BFS, some tweaked version of union find...), or fall back to initial approach of checking at invalidateInitial only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think computing thirdClassInvalidation would impact performance, because invalidateClassesInternally already calls transitiveDeps - although there it operates on the classes in changes, which is a subset of recompiledClasses we're operating on here (because we care about classes that changed in behaviour, not just those that changed in API). Let me know if I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Overall LGTM.

val transitive = IncrementalCommon.transitiveDeps(recompiledClasses, log)(dependsOnClass)
(transitive -- recompiledClasses).filter(analysis.apis.internalAPI(_).hasMacro)
}

val newInvalidations =
(firstClassInvalidation -- recompiledClasses) ++ secondClassInvalidation ++ thirdClassInvalidation
if (newInvalidations.isEmpty) {
log.debug("No classes were invalidated.")
Set.empty
} else {
if (invalidateTransitively) {
newInvalidations ++ recompiledClasses
} else {
firstClassInvalidation ++ secondClassInvalidation
firstClassInvalidation ++ secondClassInvalidation ++ thirdClassInvalidation
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,8 @@ case class ProjectStructure(
compile(i).map { analysis =>
discoverMainClasses(Some(analysis.apis)) match {
case Seq(mainClassName) =>
val cp = ((i.si.allJars.map(_.toPath) :+ classesDir) ++ outputJar).map(_.toAbsolutePath)
val jars = i.si.allJars.map(_.toPath)
val cp = (jars ++ (unmanagedJars :+ output) ++ internalClasspath).map(_.toAbsolutePath)
val loader = ClasspathUtil.makeLoader(cp, i.si, baseDirectory)
val buffer = new ByteArrayOutputStream(8192)
val oldOut = System.out
Expand Down
13 changes: 13 additions & 0 deletions zinc/src/sbt-test/macros/macro-use/app/App.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package app

object App {
def main(args: Array[String]): Unit = {
val res = lib.Macro.append("ABC")
val exp = args(0)
if (res != exp) {
val e = new Exception(s"assertion failed: expected $exp, obtained $res")
e.setStackTrace(Array())
throw e
}
}
}
6 changes: 6 additions & 0 deletions zinc/src/sbt-test/macros/macro-use/build.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"projects": [
{ "name": "lib", "scalaVersion": "2.13.12" },
{ "name": "app", "scalaVersion": "2.13.12", "dependsOn": [ "lib" ] }
]
}
5 changes: 5 additions & 0 deletions zinc/src/sbt-test/macros/macro-use/lib/Access.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package lib

class Access {
def give = new Data().suffix
}
5 changes: 5 additions & 0 deletions zinc/src/sbt-test/macros/macro-use/lib/Data.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package lib

class Data {
def suffix = "_" + new InternalApi().value
}
5 changes: 5 additions & 0 deletions zinc/src/sbt-test/macros/macro-use/lib/InternalApi.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package lib

class InternalApi {
def value: Int = 1
}
15 changes: 15 additions & 0 deletions zinc/src/sbt-test/macros/macro-use/lib/Macro.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package lib

import scala.language.experimental.macros

import scala.reflect.macros.blackbox.Context

object Macro {
def append(s: String): String = macro impl

def impl(c: Context)(s: c.Tree): c.Tree = {
import c.universe._
val suffix = new Access().give
q"""$s + $suffix"""
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package lib

class InternalApi {
def value: Int = 2
}
3 changes: 3 additions & 0 deletions zinc/src/sbt-test/macros/macro-use/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
> app/run ABC_1
$ copy-file lib/changes/InternalApi.scala lib/InternalApi.scala
> app/run ABC_2