Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SI-6640 Better reporting of deficient classpaths. #1607

Merged
merged 3 commits into from Nov 19, 2012
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/compiler/scala/tools/nsc/reporters/Reporter.scala
Expand Up @@ -20,9 +20,15 @@ abstract class Reporter {
class Severity(val id: Int) extends severity.Value {
var count: Int = 0
}
val INFO = new Severity(0)
val WARNING = new Severity(1)
val ERROR = new Severity(2)
val INFO = new Severity(0) {
override def toString: String = "INFO"
}
val WARNING = new Severity(1) {
override def toString: String = "WARNING"
}
val ERROR = new Severity(2) {
override def toString: String = "ERROR"
}

/** Whether very long lines can be truncated. This exists so important
* debugging information (like printing the classpath) is not rendered
Expand Down
8 changes: 6 additions & 2 deletions src/partest/scala/tools/partest/DirectTest.scala
Expand Up @@ -8,6 +8,7 @@ package scala.tools.partest
import scala.tools.nsc._
import io.Directory
import util.{BatchSourceFile, CommandLineParser}
import reporters.{Reporter, ConsoleReporter}

/** A class for testing code which is embedded as a string.
* It allows for more complete control over settings, compiler
Expand Down Expand Up @@ -38,9 +39,12 @@ abstract class DirectTest extends App {
// new compiler
def newCompiler(args: String*): Global = {
val settings = newSettings((CommandLineParser tokenize ("-d \"" + testOutput.path + "\" " + extraSettings)) ++ args.toList)
if (settings.Yrangepos.value) new Global(settings) with interactive.RangePositions
else new Global(settings)
if (settings.Yrangepos.value) new Global(settings, reporter(settings)) with interactive.RangePositions
else new Global(settings, reporter(settings))
}

def reporter(settings: Settings): Reporter = new ConsoleReporter(settings)

def newSources(sourceCodes: String*) = sourceCodes.toList.zipWithIndex map {
case (src, idx) => new BatchSourceFile("newSource" + (idx + 1), src)
}
Expand Down
15 changes: 15 additions & 0 deletions src/partest/scala/tools/partest/StoreReporterDirectTest.scala
@@ -0,0 +1,15 @@
package scala.tools.partest

import scala.tools.nsc.Settings
import scala.tools.nsc.reporters.StoreReporter
import scala.collection.mutable

trait StoreReporterDirectTest extends DirectTest {
lazy val storeReporter: StoreReporter = new scala.tools.nsc.reporters.StoreReporter()

/** Discards all but the first message issued at a given position. */
def filteredInfos: Seq[storeReporter.Info] = storeReporter.infos.groupBy(_.pos).map(_._2.head).toList

/** Hook into [[scala.tools.partest.DirectTest]] to install the custom reporter */
override def reporter(settings: Settings) = storeReporter
}
31 changes: 18 additions & 13 deletions src/reflect/scala/reflect/internal/Symbols.scala
Expand Up @@ -423,9 +423,9 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
* failure to the point when that name is used for something, which is
* often to the point of never.
*/
def newStubSymbol(name: Name): Symbol = name match {
case n: TypeName => new StubClassSymbol(this, n)
case _ => new StubTermSymbol(this, name.toTermName)
def newStubSymbol(name: Name, missingMessage: String): Symbol = name match {
case n: TypeName => new StubClassSymbol(this, n, missingMessage)
case _ => new StubTermSymbol(this, name.toTermName, missingMessage)
}

@deprecated("Use the other signature", "2.10.0")
Expand Down Expand Up @@ -1344,6 +1344,9 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
}
}

/** Raises a `MissingRequirementError` if this symbol is a `StubSymbol` */
def failIfStub() {}

/** Initialize the symbol */
final def initialize: this.type = {
if (!isInitialized) info
Expand Down Expand Up @@ -3100,14 +3103,18 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
)
}
trait StubSymbol extends Symbol {
protected def stubWarning = {
val from = if (associatedFile == null) "" else s" - referenced from ${associatedFile.canonicalPath}"
s"$kindString $nameString$locationString$from (a classfile may be missing)"
}
protected def missingMessage: String

/** Fail the stub by throwing a [[scala.reflect.internal.MissingRequirementError]]. */
override final def failIfStub() = {MissingRequirementError.signal(missingMessage)} //

/** Fail the stub by reporting an error to the reporter, setting the IS_ERROR flag
* on this symbol, and returning the dummy value `alt`.
*/
private def fail[T](alt: T): T = {
// Avoid issuing lots of redundant errors
if (!hasFlag(IS_ERROR)) {
globalError(s"bad symbolic reference to " + stubWarning)
globalError(missingMessage)
if (settings.debug.value)
(new Throwable).printStackTrace

Expand All @@ -3124,12 +3131,10 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
override def rawInfo = fail(NoType)
override def companionSymbol = fail(NoSymbol)

locally {
debugwarn("creating stub symbol for " + stubWarning)
}
debugwarn("creating stub symbol to defer error: " + missingMessage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why Paul wrote locally here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd wager that as he was working on this he few vals before that line that he didn't want retained as fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the way constructor bodies blend seamlessly with member definitions and intersperse arbitrarily; sometimes I use locally only to bring emphasis to the fact that a constructor exists.

}
class StubClassSymbol(owner0: Symbol, name0: TypeName) extends ClassSymbol(owner0, owner0.pos, name0) with StubSymbol
class StubTermSymbol(owner0: Symbol, name0: TermName) extends TermSymbol(owner0, owner0.pos, name0) with StubSymbol
class StubClassSymbol(owner0: Symbol, name0: TypeName, protected val missingMessage: String) extends ClassSymbol(owner0, owner0.pos, name0) with StubSymbol
class StubTermSymbol(owner0: Symbol, name0: TermName, protected val missingMessage: String) extends TermSymbol(owner0, owner0.pos, name0) with StubSymbol

trait FreeSymbol extends Symbol {
def origin: String
Expand Down
7 changes: 6 additions & 1 deletion src/reflect/scala/reflect/internal/Types.scala
Expand Up @@ -1385,7 +1385,12 @@ trait Types extends api.Types { self: SymbolTable =>
/** A class for this-types of the form <sym>.this.type
*/
abstract case class ThisType(sym: Symbol) extends SingletonType with ThisTypeApi {
assert(sym.isClass, sym)
if (!sym.isClass) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one way to whack the mole more reliably and get more encapsulation to boot:
leave this intact and provide failing implementations of all Symbol methods in the StubSymbol subclasses

I'm figuring out whether this is doable. There's also risk with missing an override of course.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other stub methods return NoType / NoSymbol etc., but what do we do here?

We need to know what the callers expectation is.

def checkIsClass(expectation: Boolean) = if (expectation != isClass) abort(...)

So... tricky.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagined isClass on a StubSymbol would throw the MissingRequirementException

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would nicely encapsulate the trigger, but it would (potentially) blow up more often that it does now. Because we don't have tests for all the variations, I erred on the side of "crash rather than fail with a nice error" rather than "fail with a nice error rather than soldier on."

While I could be persuaded to the other side, I just wanted to point out we can't encapsulate without changing behaviour.

For sure we should revisit this. But right now, the most valuable input for the PR would be additional test cases (even ideas for them, I'm happy to implement them.)

// SI-6640 allow StubSymbols to reveal what's missing from the classpath before we trip the assertion.
sym.failIfStub()
assert(false, sym)
}

//assert(sym.isClass && !sym.isModuleClass || sym.isRoot, sym)
override def isTrivial: Boolean = sym.isPackageClass
override def isNotNull = true
Expand Down
14 changes: 7 additions & 7 deletions src/reflect/scala/reflect/internal/pickling/UnPickler.scala
Expand Up @@ -20,7 +20,7 @@ import scala.annotation.switch
/** @author Martin Odersky
* @version 1.0
*/
abstract class UnPickler /*extends scala.reflect.generic.UnPickler*/ {
abstract class UnPickler {
val global: SymbolTable
import global._

Expand Down Expand Up @@ -233,7 +233,12 @@ abstract class UnPickler /*extends scala.reflect.generic.UnPickler*/ {
// (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.
owner.newStubSymbol(name)
val missingMessage =
s"""|bad symbolic reference. A signature in $filename refers to ${name.longString}
|in ${owner.kindString} ${owner.fullName} which is not available.
|It may be completely missing from the current classpath, or the version on
|the classpath might be incompatible with the version used when compiling $filename.""".stripMargin
owner.newStubSymbol(name, missingMessage)
}
}
}
Expand Down Expand Up @@ -827,11 +832,6 @@ abstract class UnPickler /*extends scala.reflect.generic.UnPickler*/ {
protected def errorBadSignature(msg: String) =
throw new RuntimeException("malformed Scala signature of " + classRoot.name + " at " + readIndex + "; " + msg)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has this method been removed? upd. Interesting... Apparently it wasn't used at all!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was used before Paul's patch.

protected def errorMissingRequirement(name: Name, owner: Symbol): Symbol =
mirrorThatLoaded(owner).missingHook(owner, name) orElse MissingRequirementError.signal(
s"bad reference while unpickling $filename: ${name.longString} not found in ${owner.tpe.widen}"
)

def inferMethodAlternative(fun: Tree, argtpes: List[Type], restpe: Type) {} // can't do it; need a compiler for that.

def newLazyTypeRef(i: Int): LazyType = new LazyTypeRef(i)
Expand Down
10 changes: 8 additions & 2 deletions test/files/neg/t5148.check
@@ -1,3 +1,9 @@
error: bad symbolic reference to value global in class IMain - referenced from t5148.scala (a classfile may be missing)
error: bad symbolic reference to value memberHandlers in class IMain - referenced from t5148.scala (a classfile may be missing)
error: bad symbolic reference. A signature in Imports.class refers to term global
in class scala.tools.nsc.interpreter.IMain which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling Imports.class.
error: bad symbolic reference. A signature in Imports.class refers to term memberHandlers
in class scala.tools.nsc.interpreter.IMain which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling Imports.class.
two errors found
5 changes: 4 additions & 1 deletion test/files/run/t6440.check
@@ -1 +1,4 @@
Stream((), ?)
pos: source-newSource1,line-9,offset=109 bad symbolic reference. A signature in U.class refers to term pack1
in package <root> which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling U.class. ERROR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ending with ERROR has a bit of a DOS feel.

47 changes: 44 additions & 3 deletions test/files/run/t6440.scala
@@ -1,7 +1,48 @@
object Test {
import scala.tools.partest._
import java.io.File

def main(args: Array[String]): Unit = {
println(Stream.continually(()).filterNot(_ => false).take(2))
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 library1 = """
package pack1
trait T
"""

def library2 = """
package pack2
trait U extends pack1.T
"""

def app = """
package pack3
object X {
trait U
}
import X._
import pack2._

trait V extends U
"""

def show(): Unit = {
Seq(library1, library2) foreach compileCode
assert(filteredInfos.isEmpty, filteredInfos)

// blow away the entire package
val pack1 = new File(testOutput.path, "pack1")
val tClass = new File(pack1, "T.class")
assert(tClass.exists)
assert(tClass.delete())
assert(pack1.delete())

// bad symbolic reference error expected (but no stack trace!)
compileCode(app)
println(filteredInfos.mkString("\n"))
}
}
4 changes: 4 additions & 0 deletions test/files/run/t6440b.check
@@ -0,0 +1,4 @@
pos: NoPosition bad symbolic reference. A signature in U.class refers to type T
in package pack1 which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling U.class. ERROR
61 changes: 61 additions & 0 deletions test/files/run/t6440b.scala
@@ -0,0 +1,61 @@
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 library1 = """
package pack1
trait T
class U {
def t = new T {}
def one = 1
}
"""

def library2 = """
package pack2
object V {
def u = new pack1.U
}
"""

def app1 = """
package pack3
object Test {
pack2.V.u.one // okay
}
"""

def app2 = """
package pack3
object Test {
pack2.V.u.t // we have to fail if T.class is misisng
}
"""

def show(): Unit = {
compileCode(library1)
val pack1 = new File(testOutput.path, "pack1")
val tClass = new File(pack1, "T.class")
assert(tClass.exists)
assert(tClass.delete())

// allowed to compile, no direct reference to `T`
compileCode(library2)
assert(filteredInfos.isEmpty, filteredInfos)

// allowed to compile, no direct reference to `T`
compileCode(app1)
assert(filteredInfos.isEmpty, filteredInfos)

// bad symbolic reference error expected (but no stack trace!)
compileCode(app2)
println(filteredInfos.mkString("\n"))
}
}
@@ -1,3 +1,2 @@
newSource1:9: error: could not find implicit value for evidence parameter of type reflect.runtime.package.universe.TypeTag[Int]
Library.foo[Int]
^

pos: source-newSource1,line-9,offset=466 could not find implicit value for evidence parameter of type reflect.runtime.package.universe.TypeTag[Int] ERROR
@@ -1,6 +1,6 @@
import scala.tools.partest._

object Test extends DirectTest {
object Test extends StoreReporterDirectTest {
def code = ???

def library = """
Expand Down Expand Up @@ -32,14 +32,12 @@ object Test extends DirectTest {
}

def show(): Unit = {
val prevErr = System.err
val baos = new java.io.ByteArrayOutputStream();
System.setErr(new java.io.PrintStream(baos));
compileLibrary();
println(filteredInfos.mkString("\n"))
storeReporter.infos.clear()
compileApp();
// we should get bad symbolic reference errors, because we're trying to call a method that can't be unpickled
// we should get bad symbolic reference errors, because we're trying to use an implicit that can't be unpickled
// but we don't know the number of these errors and their order, so I just ignore them all
baos.toString.split("\n") filter (!_.startsWith("error: bad symbolic reference")) foreach println
System.setErr(prevErr)
println(filteredInfos.filterNot(_.msg.contains("bad symbolic reference")).mkString("\n"))
}
}
}
@@ -1,3 +1,2 @@
newSource1:9: error: No Manifest available for App.this.T.
manifest[T]
^

pos: source-newSource1,line-9,offset=479 No Manifest available for App.this.T. ERROR
@@ -1,6 +1,7 @@
import scala.tools.partest._
import scala.tools.nsc.Settings

object Test extends DirectTest {
object Test extends StoreReporterDirectTest {
def code = ???

def library = """
Expand Down Expand Up @@ -29,18 +30,18 @@ object Test extends DirectTest {
"""
def compileApp() = {
val classpath = List(sys.props("partest.lib"), testOutput.path) mkString sys.props("path.separator")
val global = newCompiler("-cp", classpath, "-d", testOutput.path)
compileString(newCompiler("-cp", classpath, "-d", testOutput.path))(app)
//global.reporter.ERROR.foreach(println)
}

def show(): Unit = {
val prevErr = System.err
val baos = new java.io.ByteArrayOutputStream();
System.setErr(new java.io.PrintStream(baos));
compileLibrary();
println(filteredInfos.mkString("\n"))
storeReporter.infos.clear()
compileApp();
// we should get bad symbolic reference errors, because we're trying to use an implicit that can't be unpickled
// but we don't know the number of these errors and their order, so I just ignore them all
baos.toString.split("\n") filter (!_.startsWith("error: bad symbolic reference")) foreach println
System.setErr(prevErr)
println(filteredInfos.filterNot (_.msg.contains("bad symbolic reference")).mkString("\n"))
}
}
}