Overcompilation on 1.10.x due to potentially incorrect initial invalidation

[OndrejSpanel]

Follow-up to #1396

steps

• checkout https://github.com/OndrejSpanel/ZincApiHash
• if using IntelliJ IDE, make sure you have highlighting set to Built-in, not Compiler
• from sbt use clean;compile
• change the local variable name in Entities.f or change the comment in the same file
• use compile

problem

There are two compilation cycles:

[info] compiling 2 Scala sources to C:\Dev\ZincOvercompile\target\scala-3.5.1\classes ...
[info] done compiling
[info] compiling 1 Scala source to C:\Dev\ZincOvercompile\target\scala-3.5.1\classes ...
[info] done compiling

expectation

No more compilation cycles should be needed on a change which does not change the API.

notes

• I have developed a SBT plugin to help me isolating problems like this. It checks sbt.internal.inc.Analysis and prints which files were compiled and what classed have changed their API as a result of compilation. When used on this repro, it shows apiHash change on EntityState class
• when you repeat modify / compile again, there is only one compilation cycle. The additional cycle happens only on the first compile after clean;compile.

[eed3si9n]

Thanks for the reproduction.

I haven't tested it but I wonder if this is something specific to the way Scala 3 calculates the API hash or if it occurs on Scala 2.13 as well.

[eed3si9n]

Using sbt 1.10.2

sbt:ZincApiHash> compile
[debug] not up to date. inChanged = true, force = false
[debug] Updating ...
[debug] Done updating
[debug] [zinc] IncrementalCompile -----------
[debug] IncrementalCompile.incrementalCompile
[debug] previous = Stamps for: 9 products, 4 sources, 1 libraries
[debug] current source = Set(${BASE}/src/main/scala/Entities.scala, ${BASE}/src/main/scala/definitions.scala, ${BASE}/src/main/scala/State.scala, ${BASE}/src/main/scala/AnimatedState.scala)
[debug] > initialChanges = InitialChanges(Changes(added = Set(), removed = Set(), changed = Set(${BASE}/src/main/scala/Entities.scala), unmodified = ...),Set(),Set(),API Changes: Set())
[debug]
[debug] Initial source changes:
[debug]   removed: Set()
[debug]   added: Set()
[debug]   modified: Set(${BASE}/src/main/scala/Entities.scala)
[debug] Invalidated products: Set()
[debug] External API changes: API Changes: Set()
[debug] Modified binary dependencies: Set()
[debug] Initial directly invalidated classes: Set(Entities, EntityState)
[debug] Sources indirectly invalidated by:
[debug]   product: Set()
[debug]   binary dep: Set()
[debug]   external source: Set()
[debug] All initially invalidated classes: Set(Entities, EntityState)
[debug] All initially invalidated sources:Set(${BASE}/src/main/scala/Entities.scala)
[debug] Created transactional ClassFileManager with tempDir = /private/tmp/ZincApiHash/target/scala-3.5.1/classes.bak
[debug] Initial set of included nodes: Entities, definitions$package, EntityState
[debug] Including AnimatedState by EntityState
[debug] About to delete class files:
[debug]   definitions$package.class
[debug]   Entities.class
[debug]   Entities$.class
[debug]   EntityState.class
[debug]   definitions$package$.class
[debug]   definitions$package.tasty
[debug]   Entities.tasty
[debug]   Entities$.tasty
[debug]   EntityState.tasty
[debug]   definitions$package$.tasty
[debug] We backup class files:
[debug]   definitions$package.class
[debug]   Entities.class
[debug]   Entities$.class
[debug]   EntityState.class
[debug]   definitions$package$.class
[debug]   definitions$package.tasty
[debug]   Entities.tasty
[debug]   Entities$.tasty
[debug]   EntityState.tasty
[debug]   definitions$package$.tasty
[debug] compilation cycle 1
[info] compiling 2 Scala sources to /private/tmp/ZincApiHash/target/scala-3.5.1/classes ...

sbt 1.9.9

sbt:ZincApiHash> compile
[debug] not up to date. inChanged = true, force = false
[debug] Updating ...
[debug] Done updating
[debug] [zinc] IncrementalCompile -----------
[debug] IncrementalCompile.incrementalCompile
[debug] previous = Stamps for: 9 products, 4 sources, 1 libraries
[debug] current source = Set(${BASE}/src/main/scala/Entities.scala, ${BASE}/src/main/scala/definitions.scala, ${BASE}/src/main/scala/State.scala, ${BASE}/src/main/scala/AnimatedState.scala)
[debug] > initialChanges = InitialChanges(Changes(added = Set(), removed = Set(), changed = Set(${BASE}/src/main/scala/Entities.scala), unmodified = ...),Set(),Set(),API Changes: Set())
[debug]
[debug] Initial source changes:
[debug] 	removed: Set()
[debug] 	added: Set()
[debug] 	modified: Set(${BASE}/src/main/scala/Entities.scala)
[debug] Invalidated products: Set()
[debug] External API changes: API Changes: Set()
[debug] Modified binary dependencies: Set()
[debug] Initial directly invalidated classes: Set(Entities)
[debug] Sources indirectly invalidated by:
[debug] 	product: Set()
[debug] 	binary dep: Set()
[debug] 	external source: Set()
[debug] All initially invalidated classes: Set(Entities)
[debug] All initially invalidated sources:Set(${BASE}/src/main/scala/Entities.scala)
[debug] Created transactional ClassFileManager with tempDir = /private/tmp/ZincApiHash/target/scala-3.5.1/classes.bak
[debug] Initial set of included nodes: Entities
[debug] About to delete class files:
[debug] 	Entities$.class
[debug] 	Entities.class
[debug] 	Entities$.tasty
[debug] 	Entities.tasty
[debug] We backup class files:
[debug] 	Entities$.class
[debug] 	Entities.class
[debug] 	Entities$.tasty
[debug] 	Entities.tasty

So to me the problem is not union type, but actually the initial invalidation source has regressed in 1.10.x?

[OndrejSpanel]

I think the different set of initial invalidation is caused by #1284. I am not sure why does apiHash change, though, which is what I observe here for EntityState class.

[eed3si9n]

Yea. Given the change is around the initial invalidation, that could be right:

-    val invalidatedClasses = removedClasses ++ dependentOnRemovedClasses ++ modifiedClasses
+    val mutualDependentOnModifiedClasses = {
+      val dependentOnModifiedClasses = modifiedClasses.flatMap(previous.memberRef.internal.reverse)
+      dependentOnModifiedClasses.filter(dependent =>
+        previous.memberRef.internal.reverse(dependent).exists(modifiedClasses)
+      )
+    }
+    val invalidatedClasses =
+      removedClasses ++ dependentOnRemovedClasses ++ modifiedClasses ++ mutualDependentOnModifiedClasses

@Friendseeker Do you remember why we need to invalidate based on memberRef here?

[eed3si9n]

I guess #598 (comment)

Zinc is assuming that compiling the modified sources by themselves should always work, but it looks like this assumption is broken when files refer to each other, I've created a repo to illustrate this and reproduce the bug: https://github.com/smarter/trait-class-bug/commits/master

Because Zinc knows that A.scala refers to symbols in B.scala, and B.scala refers to symbols in A.scala, it should never attempt to compile one of those without the other, even for the first compilation of modified sources.

by @smarter. So we detect circular dependency at the class level, and invalidate them together?