From 85e0e2c1d0449a28b1892617f0a0c1b8add75345 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Mon, 18 Sep 2023 21:41:55 +0200 Subject: [PATCH] Refix compact names w/o breaking local names The previous fix phase-traveled to after flatten, which is the threshold for Symbol#name to start returning flattened names. Unfortunately there is a bug (one of a few in nsc) in that the owner of the local class symbols isn't changed as a part of a lambda lift info transform, it's changed as a side-effect of doing lambda lift tree transform. Additionally the flattened name is cached on first use, which in this case means using the wrong owner - that's why the compiler failed in pekko, because there's a check in the backend that the same classfile (by name) isn't emitted twice. Turns out, we don't need the full names of method local classes, so we can push that condition a bit higher in registerGeneratedClasses, avoiding useless work, useless phase travel, and erroneous caching. We also just straight up avoid phase traveling from xsbt-api (which runs after typer) all the way to flatten (which is almost at the end) for the flatten, compactified name, by reimplementing `flattenedName`, with the correct, recursive, flattened owner and name. By the by, the test case is pretty convoluted, despite trying to make it simpler - how many times have you defined and instantiated a method-local anonymous class, defining one of its methods using a case object? --- .../scala/scala/reflect/ReflectAccess.scala | 29 +++++++++++ .../src/main/scala/xsbt/API.scala | 44 ++++++++-------- .../src/main/scala/xsbt/CallbackGlobal.scala | 11 ++-- .../src/main/scala/xsbt/trace.scala | 26 ++++++++++ .../scala/sbt/internal/inc/IncHandler.scala | 51 ++++++++++++------- .../compactify-nested-class/main.scala | 11 ++++ .../compactify-nested-class/test | 2 + .../compactify-nested/main.scala | 15 ++++++ .../compactify-nested/test | 2 + 9 files changed, 146 insertions(+), 45 deletions(-) create mode 100644 internal/compiler-bridge/src/main/scala/scala/reflect/ReflectAccess.scala create mode 100644 internal/compiler-bridge/src/main/scala/xsbt/trace.scala create mode 100644 zinc/src/sbt-test/source-dependencies/compactify-nested-class/main.scala create mode 100644 zinc/src/sbt-test/source-dependencies/compactify-nested-class/test create mode 100644 zinc/src/sbt-test/source-dependencies/compactify-nested/main.scala create mode 100644 zinc/src/sbt-test/source-dependencies/compactify-nested/test diff --git a/internal/compiler-bridge/src/main/scala/scala/reflect/ReflectAccess.scala b/internal/compiler-bridge/src/main/scala/scala/reflect/ReflectAccess.scala new file mode 100644 index 0000000000..677b1cc6f8 --- /dev/null +++ b/internal/compiler-bridge/src/main/scala/scala/reflect/ReflectAccess.scala @@ -0,0 +1,29 @@ +package scala.reflect + +import scala.tools.nsc.Global + +// Gives access to `private[reflect] def compactifyName` +object ReflectAccess { + def flattenedOwner(g: Global)(sym: g.Symbol): g.Symbol = { + val chain = sym.owner.ownerChain.dropWhile(o => o.isClass && !o.hasPackageFlag) + if (chain.isEmpty) g.NoSymbol else chain.head + } + + def flattenedName(g: Global)(sym: g.Symbol): g.Name = { + val owner = sym.owner + + val flat = + if ( + owner.hasPackageFlag || + owner.isRoot || + owner == g.NoSymbol || + owner.hasPackageFlag + ) + "" + sym.rawname + else { + "" + flattenedName(g)(owner) + NameTransformer.NAME_JOIN_STRING + sym.rawname + } + val nameString = if (owner.isJava) flat else g.compactifyName(flat) + if (sym.isType) g.newTypeNameCached(nameString) else g.newTermNameCached(nameString) + } +} diff --git a/internal/compiler-bridge/src/main/scala/xsbt/API.scala b/internal/compiler-bridge/src/main/scala/xsbt/API.scala index 8fcfae4c96..5274914ab2 100644 --- a/internal/compiler-bridge/src/main/scala/xsbt/API.scala +++ b/internal/compiler-bridge/src/main/scala/xsbt/API.scala @@ -126,7 +126,8 @@ final class API(val global: CallbackGlobal) extends Compat with GlobalHelpers wi * @param allClassSymbols The class symbols found in all the compilation units. */ def registerGeneratedClasses(classSymbols: Iterator[Symbol]): Unit = { - classSymbols.foreach { symbol => + // Guard against a local class in case it surreptitiously leaks here + classSymbols.filter(!_.isLocalClass).foreach { symbol => val sourceFile = symbol.sourceFile val sourceVF0 = if (sourceFile == null) symbol.enclosingTopLevelClass.sourceFile @@ -138,29 +139,26 @@ final class API(val global: CallbackGlobal) extends Compat with GlobalHelpers wi } def registerProductNames(names: FlattenedNames): Unit = { - // Guard against a local class in case it surreptitiously leaks here - if (!symbol.isLocalClass) { - val pathToClassFile = s"${names.binaryName}.class" - val classFile = { - JarUtils.outputJar match { - case Some(outputJar) => - new java.io.File(JarUtils.classNameInJar(outputJar, pathToClassFile)) - case None => - val outputDir = global.settings.outputDirs.outputDirFor(sourceFile).file - new java.io.File(outputDir, pathToClassFile) - } + val pathToClassFile = s"${names.binaryName}.class" + val classFile = { + JarUtils.outputJar match { + case Some(outputJar) => + new java.io.File(JarUtils.classNameInJar(outputJar, pathToClassFile)) + case None => + val outputDir = global.settings.outputDirs.outputDirFor(sourceFile).file + new java.io.File(outputDir, pathToClassFile) } - val zincClassName = names.className - val srcClassName = classNameAsString(symbol) - sourceVF foreach { source => - callback.generatedNonLocalClass( - source, - classFile.toPath, - zincClassName, - srcClassName - ) - } - } else () + } + val zincClassName = names.className + val srcClassName = classNameAsString(symbol) + sourceVF foreach { source => + callback.generatedNonLocalClass( + source, + classFile.toPath, + zincClassName, + srcClassName + ) + } } val names = FlattenedNames( diff --git a/internal/compiler-bridge/src/main/scala/xsbt/CallbackGlobal.scala b/internal/compiler-bridge/src/main/scala/xsbt/CallbackGlobal.scala index 0458b9866c..89adb455dc 100644 --- a/internal/compiler-bridge/src/main/scala/xsbt/CallbackGlobal.scala +++ b/internal/compiler-bridge/src/main/scala/xsbt/CallbackGlobal.scala @@ -19,6 +19,7 @@ import io.AbstractFile import java.nio.file.{ Files, Path } import scala.reflect.io.PlainFile +import scala.reflect.ReflectAccess._ /** Defines the interface of the incremental compiler hiding implementation details. */ sealed abstract class CallbackGlobal( @@ -225,19 +226,21 @@ sealed class ZincCompiler(settings: Settings, dreporter: DelegatingReporter, out ): String = { var b: java.lang.StringBuffer = null def loop(size: Int, sym: Symbol): Unit = { - val symName = sym.name + val symName = flattenedName(this)(sym) // Use of encoded to produce correct paths for names that have symbols val encodedName = symName.encode val nSize = encodedName.length - (if (symName.endsWith(nme.LOCAL_SUFFIX_STRING)) 1 else 0) - if (sym.isRoot || sym.isRootPackage || sym == NoSymbol || sym.owner.isEffectiveRoot) { + val owner = flattenedOwner(this)(sym) + if (sym.isRoot || sym.isRootPackage || sym == NoSymbol || owner.isEffectiveRoot) { val capacity = size + nSize b = new java.lang.StringBuffer(capacity) b.append(chrs, encodedName.start, nSize) } else { - val next = if (sym.owner.isPackageObjectClass) sym.owner else sym.effectiveOwner.enclClass + val next = if (owner.isPackageObjectClass) owner else owner.skipPackageObject.enclClass loop(size + nSize + 1, next) // Addition to normal `fullName` to produce correct names for nested non-local classes - if (sym.isNestedClass) b.append(nme.MODULE_SUFFIX_STRING) else b.append(separator) + val isNestedClass = sym.isClass && !owner.isPackageClass + if (isNestedClass) b.append(nme.MODULE_SUFFIX_STRING) else b.append(separator) b.append(chrs, encodedName.start, nSize) } () diff --git a/internal/compiler-bridge/src/main/scala/xsbt/trace.scala b/internal/compiler-bridge/src/main/scala/xsbt/trace.scala new file mode 100644 index 0000000000..c219b1a444 --- /dev/null +++ b/internal/compiler-bridge/src/main/scala/xsbt/trace.scala @@ -0,0 +1,26 @@ +package xsbt + +object trace extends TraceSyntax { + object disable extends TraceSyntax { + override def apply[T](q: String)(op: => T): T = op + } + object enable extends TraceSyntax +} + +object TraceSyntax { + private var indent = 0 +} +trait TraceSyntax { + import TraceSyntax._ + def apply[T](q: String)(op: => T): T = { + val margin = " " * indent + println(s"$margin$q?") + val res = + try { + indent += 1 + op + } finally indent -= 1 + println(s"$margin$q = $res") + res + } +} diff --git a/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala b/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala index a856727211..29305fb7cb 100644 --- a/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala +++ b/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala @@ -232,6 +232,9 @@ class IncHandler(directory: Path, cacheDir: Path, scriptedLog: ManagedLogger, co onArgs("checkProducts") { case (p, src :: products, i) => p.checkProducts(i, dropRightColon(src), products) }, + onArgs("checkProductsExists") { + case (p, src :: Nil, i) => p.checkProductsExist(i, src) + }, onArgs("checkDependencies") { case (p, cls :: dependencies, i) => p.checkDependencies(i, dropRightColon(cls), dependencies) }, @@ -441,28 +444,32 @@ case class ProjectStructure( () } + def getProducts(analysis: Analysis)(srcFile: String): Set[VirtualFileRef] = + analysis.relations.products(converter.toVirtualFile(baseDirectory / srcFile)) + def checkProducts(i: IncState, src: String, expected: List[String]): Future[Unit] = compile(i).map { analysis => - // def isWindows: Boolean = sys.props("os.name").toLowerCase.startsWith("win") - // def relativeClassDir(f: File): File = f.relativeTo(classesDir) getOrElse f - // def normalizePath(path: String): String = { - // if (isWindows) path.replace('\\', '/') else path - // } - def products(srcFile: String): Set[String] = { - val productFiles = analysis.relations.products(converter.toVirtualFile(baseDirectory / src)) - productFiles.map { file: VirtualFileRef => - // if (JarUtils.isClassInJar(file)) { - // JarUtils.ClassInJar.fromPath(output.toPath).toClassFilePath - // } else { - // normalizePath(relativeClassDir(file).getPath) - // } - file.id - } - } def assertClasses(expected: Set[String], actual: Set[String]) = - assert(expected == actual, s"Expected $expected products, got $actual") + assert( + expected == actual, + s"""Mismatch: + |Expected:${expected.toList.sorted.map("\n " + _).mkString} + |Obtained:${actual.toList.sorted.map("\n " + _).mkString}""".stripMargin + ) + + assertClasses(expected.toSet, getProducts(analysis)(src).map(_.id)) + () + } - assertClasses(expected.toSet, products(src)) + def checkProductsExist(i: IncState, src: String): Future[Unit] = + compile(i).map { analysis => + val missing = + for (p <- getProducts(analysis)(src).toList if !Files.exists(converter.toPath(p))) yield p + assert( + missing.isEmpty, + s"""Missing ${missing.size} products:${missing.map("\n " + _).mkString} + |Generated:${generatedClassFiles.get.toList.map("\n " + _).mkString}""".stripMargin + ) () } @@ -829,6 +836,14 @@ case class ProjectStructure( } () } + + def assertShort(assertion: Boolean, message: => Any) = { + if (!assertion) { + val err = new AssertionError("assertion failed: " + message) + err.setStackTrace(Array()) + throw err + } + } } object IncHandler { diff --git a/zinc/src/sbt-test/source-dependencies/compactify-nested-class/main.scala b/zinc/src/sbt-test/source-dependencies/compactify-nested-class/main.scala new file mode 100644 index 0000000000..a7c36134b8 --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/compactify-nested-class/main.scala @@ -0,0 +1,11 @@ +package p1.p2 + +abstract class B { + def m(): L + abstract class L { + def i: I + abstract class I + } +} +class D extends B { override def m() = new L { override case object i extends I } } +class G extends B { override def m() = new L { override case object i extends I } } diff --git a/zinc/src/sbt-test/source-dependencies/compactify-nested-class/test b/zinc/src/sbt-test/source-dependencies/compactify-nested-class/test new file mode 100644 index 0000000000..d8e9c4509e --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/compactify-nested-class/test @@ -0,0 +1,2 @@ +> checkProducts main.scala: ${BASE}/target/classes/p1/p2/B$L$I.class ${BASE}/target/classes/p1/p2/B$L.class ${BASE}/target/classes/p1/p2/B.class ${BASE}/target/classes/p1/p2/D$$anon$1$i$.class ${BASE}/target/classes/p1/p2/D$$anon$1.class ${BASE}/target/classes/p1/p2/D.class ${BASE}/target/classes/p1/p2/G$$anon$2$i$.class ${BASE}/target/classes/p1/p2/G$$anon$2.class ${BASE}/target/classes/p1/p2/G.class +> checkProductsExists main.scala diff --git a/zinc/src/sbt-test/source-dependencies/compactify-nested/main.scala b/zinc/src/sbt-test/source-dependencies/compactify-nested/main.scala new file mode 100644 index 0000000000..8441d985ed --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/compactify-nested/main.scala @@ -0,0 +1,15 @@ +package p1.p2 + +class OuterLevelWithVeryVeryVeryLongClassName1 { + class OuterLevelWithVeryVeryVeryLongClassName2 { + class OuterLevelWithVeryVeryVeryLongClassName3 { + class OuterLevelWithVeryVeryVeryLongClassName4 { + class OuterLevelWithVeryVeryVeryLongClassName5 { + class OuterLevelWithVeryVeryVeryLongClassName6 { + class OuterLevelWithVeryVeryVeryLongClassName7 + } + } + } + } + } +} diff --git a/zinc/src/sbt-test/source-dependencies/compactify-nested/test b/zinc/src/sbt-test/source-dependencies/compactify-nested/test new file mode 100644 index 0000000000..bb8ca18162 --- /dev/null +++ b/zinc/src/sbt-test/source-dependencies/compactify-nested/test @@ -0,0 +1,2 @@ +> checkProducts main.scala: ${BASE}/target/classes/p1/p2/OuterLevelWithVeryVeryVeryLongClassName1.class ${BASE}/target/classes/p1/p2/OuterLevelWithVeryVeryVeryLongClassName1$OuterLevelWithVeryVeryVeryLongClassName2.class ${BASE}/target/classes/p1/p2/OuterLevelWithVeryVeryVeryLongClassName1$OuterLevelWithVeryVeryVeryLongClassName2$OuterLevelWithVeryVeryVeryLongClassName3.class ${BASE}/target/classes/p1/p2/OuterLevelWithVeryVeryVeryLongClassName1$OuterLevelWithVeryVeryVeryLongClassName2$OuterLevelWithVeryVeryVeryLongClassName3$OuterLevelWithVeryVeryVeryLongClassName4.class ${BASE}/target/classes/p1/p2/OuterLevelWithVeryVeryVeryLongClassName1$OuterLevelWithVeryVeryVeryLongClassName2$OuterLevelWithVeryVeryVeryLongClassName3$OuterLevelWithVeryVeryVeryLongClassName4$OuterLevelWithVeryVeryVeryLongClassName5.class ${BASE}/target/classes/p1/p2/OuterLevelWithVeryVeryVeryLongClassName1$OuterLevelWithVeryVeryVeryLongClassName2$OuterLevelWithVeryVeryVeryLongClassName3$OuterLevelWithVeryVeryVeryLongClassName4$OuterLevelWithVeryVeryVeryLongClassName5$OuterLevelWithVeryVeryVeryLongClassName6.class ${BASE}/target/classes/p1/p2/OuterLevelWithVeryVeryVeryLongClassName1$OuterLevelWithVeryVe$$$$6facba931fe42f8a8c3cee88c4087$$$$ryVeryLongClassName6$OuterLevelWithVeryVeryVeryLongClassName7.class +> checkProductsExists main.scala