Skip to content

Commit

Permalink
Refix compact names w/o breaking local names
Browse files Browse the repository at this point in the history
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?
  • Loading branch information
dwijnand committed Sep 19, 2023
1 parent 42e124e commit 85e0e2c
Show file tree
Hide file tree
Showing 9 changed files with 146 additions and 45 deletions.
Original file line number Diff line number Diff line change
@@ -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)
}
}
44 changes: 21 additions & 23 deletions internal/compiler-bridge/src/main/scala/xsbt/API.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
}
()
Expand Down
26 changes: 26 additions & 0 deletions internal/compiler-bridge/src/main/scala/xsbt/trace.scala
Original file line number Diff line number Diff line change
@@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
Expand Down Expand Up @@ -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
)
()
}

Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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 } }
Original file line number Diff line number Diff line change
@@ -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
15 changes: 15 additions & 0 deletions zinc/src/sbt-test/source-dependencies/compactify-nested/main.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package p1.p2

class OuterLevelWithVeryVeryVeryLongClassName1 {
class OuterLevelWithVeryVeryVeryLongClassName2 {
class OuterLevelWithVeryVeryVeryLongClassName3 {
class OuterLevelWithVeryVeryVeryLongClassName4 {
class OuterLevelWithVeryVeryVeryLongClassName5 {
class OuterLevelWithVeryVeryVeryLongClassName6 {
class OuterLevelWithVeryVeryVeryLongClassName7
}
}
}
}
}
}
2 changes: 2 additions & 0 deletions zinc/src/sbt-test/source-dependencies/compactify-nested/test
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 85e0e2c

Please sign in to comment.