From b47aaf6445afe4a6818c31a0ed10e680e6b82c24 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 29 Nov 2016 14:40:59 +1000 Subject: SI-8502 Rework handling of stub symbols in unpickler - Rework previous fixes for SI-8502 to move the creation of a term or type stub symbol during unpickling to the initial point of stub creation, based on the tag. - Just set the PACKAGE flag on class stub symbols created during unpickling `ThisType`, rather than bothering with a different subclass of `StubSymbol` for (assumed) packages. --- src/reflect/scala/reflect/internal/Symbols.scala | 3 +-- src/reflect/scala/reflect/internal/pickling/UnPickler.scala | 7 +++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala index 56b6dc078d..6a7b3fc9a2 100644 --- a/src/reflect/scala/reflect/internal/Symbols.scala +++ b/src/reflect/scala/reflect/internal/Symbols.scala @@ -488,7 +488,7 @@ trait Symbols extends api.Symbols { self: SymbolTable => * often to the point of never. */ def newStubSymbol(name: Name, missingMessage: String, isPackage: Boolean = false): Symbol = name match { - case n: TypeName => if (isPackage) new StubPackageClassSymbol(this, n, missingMessage) else new StubClassSymbol(this, n, missingMessage) + case n: TypeName => new StubClassSymbol(this, n, missingMessage) case _ => new StubTermSymbol(this, name.toTermName, missingMessage) } @@ -3423,7 +3423,6 @@ trait Symbols extends api.Symbols { self: SymbolTable => override def companionSymbol = fail(NoSymbol) } class StubClassSymbol(owner0: Symbol, name0: TypeName, val missingMessage: String) extends ClassSymbol(owner0, owner0.pos, name0) with StubSymbol - class StubPackageClassSymbol(owner0: Symbol, name0: TypeName, val missingMessage: String) extends PackageClassSymbol(owner0, owner0.pos, name0) with StubSymbol class StubTermSymbol(owner0: Symbol, name0: TermName, val missingMessage: String) extends TermSymbol(owner0, owner0.pos, name0) with StubSymbol trait FreeSymbol extends Symbol { diff --git a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala index 6dea184826..3bc845557f 100644 --- a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala +++ b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala @@ -267,7 +267,8 @@ abstract class UnPickler { |because it (or its dependencies) are missing. Check your build definition for |missing or conflicting dependencies. (Re-run with `-Ylog-classpath` to see the problematic classpath.) |A full rebuild may help if '$filename' was compiled against an incompatible version of ${owner.fullName}.$advice""".stripMargin - owner.newStubSymbol(name, missingMessage) + val stubName = if (tag == EXTref) name else name.toTypeName + owner.newStubSymbol(stubName, missingMessage) } } } @@ -392,9 +393,7 @@ abstract class UnPickler { def readThisType(): Type = { val sym = readSymbolRef() match { - case stub: StubSymbol if !stub.isClass => - // SI-8502 This allows us to create a stub for a unpickled reference to `missingPackage.Foo`. - stub.owner.newStubSymbol(stub.name.toTypeName, stub.missingMessage, isPackage = true) + case stub: StubSymbol => stub.setFlag(PACKAGE) case sym => sym } ThisType(sym) -- cgit v1.2.3 From 51d851ce2e403201d99d236a4bafd5728edc754f Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 29 Nov 2016 14:42:01 +1000 Subject: SD-275 Further harden against refs to absentee classes - Limit the strategy of unpickling an external reference to a module class to a lookup of the module var to non-stub owners in order to enable fall through to stub symbol creation. Fixes scala/scala-dev#275 --- .../reflect/internal/pickling/UnPickler.scala | 5 +- test/files/run/sd275-java/A.java | 5 ++ test/files/run/sd275-java/DeleteMe.java | 4 ++ test/files/run/sd275-java/LeaveMe.java | 3 ++ test/files/run/sd275-java/Test.scala | 39 ++++++++++++++ test/files/run/sd275.scala | 60 ++++++++++++++++++++++ 6 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 test/files/run/sd275-java/A.java create mode 100644 test/files/run/sd275-java/DeleteMe.java create mode 100644 test/files/run/sd275-java/LeaveMe.java create mode 100644 test/files/run/sd275-java/Test.scala create mode 100644 test/files/run/sd275.scala diff --git a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala index 3bc845557f..fd7d2d9dd6 100644 --- a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala +++ b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala @@ -224,10 +224,9 @@ abstract class UnPickler { if (owner.isOverloaded) return NoSymbol - if (tag == EXTMODCLASSref) { + if (tag == EXTMODCLASSref && !owner.isInstanceOf[StubSymbol]) owner.info.decl(nme.moduleVarName(name.toTermName)) - } - NoSymbol + else NoSymbol } def moduleAdvice(missing: String): String = { diff --git a/test/files/run/sd275-java/A.java b/test/files/run/sd275-java/A.java new file mode 100644 index 0000000000..b293cf6dab --- /dev/null +++ b/test/files/run/sd275-java/A.java @@ -0,0 +1,5 @@ +package sample; +public class A { + public void irrelevant(p1.p2.p3.DeleteMe arg) {} + public static class A_Inner {} +} diff --git a/test/files/run/sd275-java/DeleteMe.java b/test/files/run/sd275-java/DeleteMe.java new file mode 100644 index 0000000000..ccff2951d0 --- /dev/null +++ b/test/files/run/sd275-java/DeleteMe.java @@ -0,0 +1,4 @@ +package p1.p2.p3; + +public class DeleteMe {} + diff --git a/test/files/run/sd275-java/LeaveMe.java b/test/files/run/sd275-java/LeaveMe.java new file mode 100644 index 0000000000..cb58f0080f --- /dev/null +++ b/test/files/run/sd275-java/LeaveMe.java @@ -0,0 +1,3 @@ +package p1; + +public class LeaveMe {} diff --git a/test/files/run/sd275-java/Test.scala b/test/files/run/sd275-java/Test.scala new file mode 100644 index 0000000000..84187527d2 --- /dev/null +++ b/test/files/run/sd275-java/Test.scala @@ -0,0 +1,39 @@ +import scala.tools.partest._ +import java.io.File + +object Test extends StoreReporterDirectTest { + def code = ??? + + def compileCode(code: String) = { + val classpath = List(sys.props("partest.lib"), testOutput.path) mkString sys.props("path.separator") + compileString(newCompiler("-cp", classpath, "-d", testOutput.path))(code) + } + + def show(): Unit = { + deletePackage("p1/p2/p3") + deletePackage("p1/p2") + + compileCode(""" +package sample + +class Test { + final class Inner extends A.A_Inner { + def foo = 42 + } + + def test = new Inner().foo +} + """) + assert(storeReporter.infos.isEmpty, storeReporter.infos.mkString("\n")) + } + + def deletePackage(name: String) { + val directory = new File(testOutput.path, name) + for (f <- directory.listFiles()) { + assert(f.getName.endsWith(".class")) + assert(f.delete()) + } + assert(directory.listFiles().isEmpty) + assert(directory.delete()) + } +} diff --git a/test/files/run/sd275.scala b/test/files/run/sd275.scala new file mode 100644 index 0000000000..8cdee3ae15 --- /dev/null +++ b/test/files/run/sd275.scala @@ -0,0 +1,60 @@ +import scala.tools.partest._ +import java.io.File + +object Test extends StoreReporterDirectTest { + def code = ??? + + def compileCode(code: String) = { + val classpath = List(sys.props("partest.lib"), testOutput.path) mkString sys.props("path.separator") + compileString(newCompiler("-cp", classpath, "-d", testOutput.path))(code) + } + + def show(): Unit = { + compileCode(""" +package sample { + + class A1 { + def irrelevant: p1.p2.p3.DeleteMe = null + } + object A1 { + class A1_Inner + } +} + +package p1 { + class LeaveMe + package p2 { + package p3 { + class DeleteMe + } + } +} + """) + assert(filteredInfos.isEmpty, filteredInfos) + deletePackage("p1/p2/p3") + deletePackage("p1/p2") + + compileCode(""" +package sample + +class Test { + final class Inner extends A1.A1_Inner { + def foo = 42 + } + + def test = new Inner().foo +} + """) + assert(storeReporter.infos.isEmpty, storeReporter.infos.mkString("\n")) // Included a MissingRequirementError before. + } + + def deletePackage(name: String) { + val directory = new File(testOutput.path, name) + for (f <- directory.listFiles()) { + assert(f.getName.endsWith(".class")) + assert(f.delete()) + } + assert(directory.listFiles().isEmpty) + assert(directory.delete()) + } +} -- cgit v1.2.3 From 31723573b3a25cd27f2b5c1079dfb04c5b10910a Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 29 Nov 2016 22:12:51 +1000 Subject: SD-275 Remove obsolete code from the unpickler AFAICT, this was only needed to support pickle compatibility after the fix for SI-1591. We don't need to maintain the compatibility after incrementing our major version. --- .../reflect/internal/pickling/UnPickler.scala | 39 +++++++--------------- 1 file changed, 12 insertions(+), 27 deletions(-) diff --git a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala index fd7d2d9dd6..fe1de91662 100644 --- a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala +++ b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala @@ -216,18 +216,6 @@ abstract class UnPickler { } adjust(decl) } - def nestedObjectSymbol: Symbol = { - // If the owner is overloaded (i.e. a method), it's not possible to select the - // right member, so return NoSymbol. This can only happen when unpickling a tree. - // the "case Apply" in readTree() takes care of selecting the correct alternative - // after parsing the arguments. - if (owner.isOverloaded) - return NoSymbol - - if (tag == EXTMODCLASSref && !owner.isInstanceOf[StubSymbol]) - owner.info.decl(nme.moduleVarName(name.toTermName)) - else NoSymbol - } def moduleAdvice(missing: String): String = { val module = @@ -254,21 +242,18 @@ abstract class UnPickler { // symbols are read from outside: for instance when checking the children // of a class. See #1722. fromName(nme.expandedName(name.toTermName, owner)) orElse { - // (3) Try as a nested object symbol. - nestedObjectSymbol orElse { - // (4) Call the mirror's "missing" hook. - adjust(mirrorThatLoaded(owner).missingHook(owner, name)) orElse { - // (5) Create a stub symbol to defer hard failure a little longer. - val advice = moduleAdvice(s"${owner.fullName}.$name") - val missingMessage = - s"""|missing or invalid dependency detected while loading class file '$filename'. - |Could not access ${name.longString} in ${owner.kindString} ${owner.fullName}, - |because it (or its dependencies) are missing. Check your build definition for - |missing or conflicting dependencies. (Re-run with `-Ylog-classpath` to see the problematic classpath.) - |A full rebuild may help if '$filename' was compiled against an incompatible version of ${owner.fullName}.$advice""".stripMargin - val stubName = if (tag == EXTref) name else name.toTypeName - owner.newStubSymbol(stubName, missingMessage) - } + // (3) Call the mirror's "missing" hook. + adjust(mirrorThatLoaded(owner).missingHook(owner, name)) orElse { + // (4) Create a stub symbol to defer hard failure a little longer. + val advice = moduleAdvice(s"${owner.fullName}.$name") + val missingMessage = + s"""|missing or invalid dependency detected while loading class file '$filename'. + |Could not access ${name.longString} in ${owner.kindString} ${owner.fullName}, + |because it (or its dependencies) are missing. Check your build definition for + |missing or conflicting dependencies. (Re-run with `-Ylog-classpath` to see the problematic classpath.) + |A full rebuild may help if '$filename' was compiled against an incompatible version of ${owner.fullName}.$advice""".stripMargin + val stubName = if (tag == EXTref) name else name.toTypeName + owner.newStubSymbol(stubName, missingMessage) } } } -- cgit v1.2.3