Exclude anon classes from dependency registration #1507

retronym · 2024-12-03T00:14:51Z

These can show in edge cases like:

package demo

class SRC[_]
class Filter
class Refined {
  def select(cols: SRC[_]*) = new {
    def where(filter: Filter => Filter)(implicit d: DummyImplicit) = {
      new {
        def using(opt: Box[SRC[_]] => Some[SRC[_]])(implicit d: DummyImplicit) = opt
      }
    }
  }
}

package demo

class Client {
   def temp = new Refined().select(null).where(null).using(null)
}

This can leak an existential type owned by an anon-class into Client, at least prior to this compiler fix:

scala/scala#10940

These can show in edge cases like: ``` package demo class SRC[_] class Filter class Refined { def select(cols: SRC[_]*) = new { def where(filter: Filter => Filter)(implicit d: DummyImplicit) = { new { def using(opt: Box[SRC[_]] => Some[SRC[_]])(implicit d: DummyImplicit) = opt } } } } ``` ``` package demo class Client { def temp = new Refined().select(null).where(null).using(null) } ``` This can leak an existential type owned by an anon-class into `Client`, at least prior to this compiler fix: scala/scala#10940

retronym · 2024-12-03T00:51:11Z

I'm not 100% sure if this leads to an observable under- or over-compilation bug.

One thing I've noticed is that it does create a spurious entry in the external libraries table in the analysis. That should only be used for dependencies on classes in libraryDependencies, not intra-dependencies with this SBT project.

import sbt.internal.inc.{Analysis, Stamps}

scalaVersion := "2.13.15"

val p1 = project

val p2 = project.dependsOn(p1)

TaskKey[Unit]("diagnostic") := {
  val stamps = (p2 / Compile / compile).value.asInstanceOf[Analysis].stamps.asInstanceOf[Stamps]

  println("libraries = " + stamps.libraries)
}

sbt:zinc-refinement> diagnostic
[debug] Other repositories:
[debug] Default repositories:
[debug] Using inline dependencies specified in Scala.
[debug] Copy resource mappings:
[debug]
[debug] [zinc] IncrementalCompile -----------
[debug] IncrementalCompile.incrementalCompile
[debug] previous = Stamps for: 7 products, 2 sources, 1 libraries
[debug] current source = Set(${BASE}/p1/src/main/scala/Refined.scala, ${BASE}/p1/src/main/scala/Box.scala)
[debug] > initialChanges = InitialChanges(Changes(added = Set(), removed = Set(), changed = Set(), unmodified = ...),Set(),Set(),API Changes: Set())
[debug] No changes
[debug] Created transactional ClassFileManager with tempDir = /Users/jz/code/zinc-refinement/p1/target/scala-2.12/classes.bak
[debug] Removing the temporary directory used for backing up class files: /Users/jz/code/zinc-refinement/p1/target/scala-2.12/classes.bak
[debug] [zinc] IncrementalCompile -----------
[debug] IncrementalCompile.incrementalCompile
[debug] previous = Stamps for: 1 products, 1 sources, 2 libraries
[debug] current source = Set(${BASE}/p2/src/main/scala/Client.scala)
[debug] > initialChanges = InitialChanges(Changes(added = Set(), removed = Set(), changed = Set(), unmodified = ...),Set(),Set(),API Changes: Set())
[debug] No changes
[debug] Created transactional ClassFileManager with tempDir = /Users/jz/code/zinc-refinement/p2/target/scala-2.12/classes.bak
[debug] Removing the temporary directory used for backing up class files: /Users/jz/code/zinc-refinement/p2/target/scala-2.12/classes.bak
libraries = Map(${BASE}/p1/target/scala-2.12/classes/demo/Refined.class -> farm(1651ebda9ac28257), ${SBT_BOOT}/scala-2.12.20/lib/scala-library.jar -> farm(d37410c41eebd71a))
[success] Total time: 0 s, completed 3 Dec 2024, 10:43:03 am

eed3si9n · 2024-12-03T02:33:27Z

internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala

+                 * }
+                 *
+                 * Or the status quo might just be perfectly fine -- if compilation doesn't need to force `Foo`,
+                 * then there isn't a real dependency.
                 */
                val fqn = fullName(targetSymbol, '.', targetSymbol.moduleSuffix, false)
                global.findAssociatedFile(fqn) match {


This is the last line I can leave comment, but a few lines down is

case Some((at, true)) => processExternalDependency(fqn, at)

which could explain the additional library (external) dependency. I think normally we check local, subproject, then library in order, but confusing the dependency level could potentially lead to a weird invalidation down the line.

confusing the dependency level could potentially lead to a weird invalidation down the line.

The customer report we're investigating was using direct-to-JAR compilation, which I can imagine cold trigger over-compilation if the spurious external is declared on the entire JAR.

Indeed, if I:

set exportJars := true

add p1/src/main/demo/Other.scala

p2/Compile/compile

Then, Zinc overcompiles p2/src/main/demo/Client.scala:

[debug] IncrementalCompile.incrementalCompile [debug] previous = Stamps for: 1 products, 1 sources, 2 libraries [debug] current source = Set(${BASE}/p2/src/main/scala/Client.scala) [debug] Invalidating '${BASE}/p1/target/scala-2.12/p1_2.12-0.1.0-SNAPSHOT.jar' because could not find class demo.Refined.select$$anon.where$$anon on the classpath. [debug] > initialChanges = InitialChanges(Changes(added = Set(), removed = Set(), changed = Set(), unmodified = ...),Set(),Set(${BASE}/p1/target/scala-2.12/p1_2.12-0.1.0-SNAPSHOT.jar),API Changes: Set()) [debug] [debug] Initial source changes: [debug] removed: Set() [debug] added: Set() [debug] modified: Set() [debug] Invalidated products: Set() [debug] External API changes: API Changes: Set() [debug] Modified binary dependencies: Set(${BASE}/p1/target/scala-2.12/p1_2.12-0.1.0-SNAPSHOT.jar) [debug] Initial directly invalidated classes: Set() [debug] Sources indirectly invalidated by: [debug] product: Set() [debug] binary dep: Set(${BASE}/p2/src/main/scala/Client.scala) [debug] external source: Set() [debug] all 1 sources are invalidated [debug] Created transactional ClassFileManager with tempDir = /Users/jz/code/zinc-refinement/p2/target/scala-2.12/classes.bak [debug] Initial set of included nodes: [debug] Recompiling all sources: number of invalidated sources > 50.0 percent of all sources [debug] About to delete class files: [debug] Client.class [debug] We backup class files: [debug] Client.class [debug] compilation cycle 1 [info] compiling 1 Scala source to /Users/jz/code/zinc-refinement/p2/target/scala-2.12/classes ...

internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala

Co-authored-by: Jiahui (Jerry) Tan <66892505+Friendseeker@users.noreply.github.com>

Friendseeker · 2024-12-04T03:48:03Z

I made a GitHub repo containing retronym's reproduction for people to play around with: https://github.com/Friendseeker/zinc-1507

Also made some minor simplification to the reproduction.

Friendseeker · 2024-12-04T04:36:59Z

Just want to double check if my understanding of the issue is accurate here, as I am not familiar with compiler internal.

So an ExistentialType (in this case SRC[_]) has a quantified type (call it tpe), for which its enclosing class (tpe.typeSymbol.enclClass) is an anon class. Zinc then tries to register the class dependency between Client and that anon class. As Zinc does not recognize the anon class as declared by any source files, Zinc decides that the anon class must be from a library and registers an external library dependency.

The 3 compiler gurus (Eugene, Dale, Lukas) likely grasp the issue much better than me, but I guess there's probably no edge case that can emerge from not tracking class level dependency with anon class? Say an anon class is changed, then some name hash of the enclosing top-level named class will change, so tracking class level dep with anon class is completely unnecessary.

retronym · 2024-12-04T11:56:34Z

So an ExistentialType (in this case SRC[_]) has a quantified type (call it tpe), for which its enclosing class (tpe.typeSymbol.enclClass) is an anon class. Zinc then tries to register the class dependency between Client and that anon class. As Zinc does not recognize the anon class as declared by any source files, Zinc decides that the anon class must be from a library and registers an external library dependency.

This is basically correct. SRC[_] is a shorthand for SRC[$1] forSome { type $1 }. The compiler create a Symbol represent $1, as it does to represent anything named entity in the program (ie classes, params, type params, methods). All symbols have an owner which is typically the symbol of the closest, suitable enclosing Symbol.

The compiler really shouldn't let the symbol for this anonymous class within the method "leak" out into method return type, even in a seemingly innocuous way as above where it just the owner of a symbol. We're fixing that in the compiler.

But Zinc also can stand to be more discriminating in its interpretation of the Symbol's it encounters. Calling flatnameon an anoymous class symbol at the point in compilation whereExtractAPIruns gives nonsensical results (the actual class name will beanon$N` once a later compiler phase lifts classes out of methods into the enclosing class.

Run this to see what I mean scala --scala-version 2.13.15 -Vprint:all -e 'class C { def f = new { "fanon"} }'

The 3 compiler gurus (Eugene, Dale, Lukas) likely grasp the issue much better than me, but I guess there's probably no edge case that can emerge from not tracking class level dependency with anon class? Say an anon class is changed, then some name hash of the enclosing top-level named class will change, so tracking class level dep with anon class is completely unnecessary.

Yep, any changes to the anonymous class that are relevant to incremental compilation are reflected in the structural type of the method, which is determines its API hash.

eed3si9n

LGTM. Thanks!

retronym added 2 commits December 3, 2024 10:13

Add some notes to the saga of absent associatedFile

f1547d0

retronym requested review from lrytz and dwijnand December 3, 2024 02:29

eed3si9n reviewed Dec 3, 2024

View reviewed changes

Friendseeker reviewed Dec 3, 2024

View reviewed changes

internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala Outdated Show resolved Hide resolved

Update internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala

c752dae

Co-authored-by: Jiahui (Jerry) Tan <66892505+Friendseeker@users.noreply.github.com>

Friendseeker approved these changes Dec 4, 2024

View reviewed changes

eed3si9n approved these changes Dec 6, 2024

View reviewed changes

eed3si9n merged commit e2f5ea1 into sbt:1.10.x Dec 6, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude anon classes from dependency registration #1507

Exclude anon classes from dependency registration #1507

retronym commented Dec 3, 2024 •

edited

Loading

retronym commented Dec 3, 2024 •

edited

Loading

eed3si9n Dec 3, 2024

retronym Dec 3, 2024

retronym Dec 3, 2024 •

edited

Loading

Friendseeker commented Dec 4, 2024 •

edited

Loading

Friendseeker commented Dec 4, 2024 •

edited

Loading

retronym commented Dec 4, 2024

eed3si9n left a comment

Exclude anon classes from dependency registration #1507

Exclude anon classes from dependency registration #1507

Conversation

retronym commented Dec 3, 2024 • edited Loading

retronym commented Dec 3, 2024 • edited Loading

eed3si9n Dec 3, 2024

Choose a reason for hiding this comment

retronym Dec 3, 2024

Choose a reason for hiding this comment

retronym Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Friendseeker commented Dec 4, 2024 • edited Loading

Friendseeker commented Dec 4, 2024 • edited Loading

retronym commented Dec 4, 2024

eed3si9n left a comment

Choose a reason for hiding this comment

retronym commented Dec 3, 2024 •

edited

Loading

retronym commented Dec 3, 2024 •

edited

Loading

retronym Dec 3, 2024 •

edited

Loading

Friendseeker commented Dec 4, 2024 •

edited

Loading

Friendseeker commented Dec 4, 2024 •

edited

Loading