Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

SI-6640 Better reporting of deficient classpaths. #1607

Merged
merged 3 commits into from

7 participants

@retronym
Owner

In a55788e, StubSymbols were introduced to fail-slow when
the classpath was deficient. This allowed compilation to
succeed in cases when one didn't actually use the part of
class A which referred to some missing class B.

But a few problems were introduced.

Firstly, when the deferred error eventually happened, it was
signalled with abort(msg), rather than through a thrown
MissingRequirementError. The latter is desirable, as it doesn't
lead to printing a stack trace.

Second, the actual error message changed, and no longer
included the name of the class file that refers to the missing
class.

Finally, it seems that we can end up with a stub term symbol
in a situation where a class symbol is desired. An assertion
in the constructor of ThisType throws trips when calling .isClass,
before the useful error message from StubSymbol can be emitted.

This commit addresses these points, and rewords the error
a little to be more accessible. The last point is the most fragile
in this arrangement, there might be some whack-a-mole
required to similar assertions. I don't see a clean solution
for this, but am open to suggestions.

After the change:

[info] Set current project to default-821d14 (in build file:/Users/jason/code/scratch1/)
> compile

[info] Compiling 1 Scala source to /Users/jason/code/scratch1/target/scala-2.10/classes...
[error]
[error]      while compiling: /Users/jason/code/scratch1/test.scala
[error]         during phase: typer
[error]      library version: version 2.10.0-RC2
[error]     compiler version: version 2.10.0-RC2
...
[error]   last tree to typer: Ident(SwingWorker)
[error]               symbol: <none> (flags: )
[error]    symbol definition: <none>
[error]        symbol owners:
[error]       context owners: object Test -> package <empty>
 ...
[error] uncaught exception during compilation: java.lang.AssertionError
[trace] Stack trace suppressed: run last compile:compile for the full output.
[error] (compile:compile) java.lang.AssertionError: assertion failed: value actors
[error] Total time: 2 s, completed Nov 10, 2012 3:18:34 PM
>
> set scalaHome := Some(file("/Users/jason/code/scala/build/pack"))
[info] Defining *:scala-home
[info] The new value will be used by no settings or tasks.
[info] Reapplying settings...
[info] Set current project to default-821d14 (in build file:/Users/jason/code/scratch1/)
^[compile
[info] Compiling 1 Scala source to /Users/jason/code/scratch1/target/scala-2.10/classes...
[error] /Users/jason/code/scratch1/test.scala:4: A signature in SwingWorker.class refers to term actors in package scala which is missing from the classpath.
[error] object Test extends SwingWorker
[error]                     ^
[error] one error found
[error] (compile:compile) Compilation failed
[error] Total time: 2 s, completed Nov 10, 2012 3:18:45 PM

Review by @paulp @jsuereth

@xeno-by
Owner

Nice error message!

@xeno-by xeno-by commented on the diff
src/reflect/scala/reflect/internal/Symbols.scala
@@ -3124,12 +3121,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)
@xeno-by Owner
xeno-by added a note

I wonder why Paul wrote locally here.

@retronym Owner

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

@paulp
paulp added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/reflect/scala/reflect/internal/Types.scala
@@ -1385,7 +1385,7 @@ 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)
+ assert(sym.isClass, {sym.info; sym}) // call .info to allow StubSymbols to reveal what's missing from the classpath
@xeno-by Owner
xeno-by added a note

Mmmm. This asks for cyclic reference errors. Are you sure?

@retronym Owner

Maybe sym.forceStubFailure() would be clearer/safer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...flect/scala/reflect/internal/pickling/UnPickler.scala
@@ -233,7 +233,8 @@ 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"A signature in $filename refers to ${name.longString} in ${owner.fullLocationString} which is missing from the classpath."
@xeno-by Owner
xeno-by added a note

It looks like missingMessage is created here just because filename is N/A at the error throwing point.

If that's true, then probably symbol.associatedFile would help to evict this nuisance.

@retronym Owner

That was used in the previous error message, but didn't show anything.

We don't want the file of this symbol, but the file of the referring class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xeno-by xeno-by commented on the diff
...flect/scala/reflect/internal/pickling/UnPickler.scala
@@ -827,11 +828,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)
- 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}"
- )
-
@xeno-by Owner
xeno-by added a note

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

@retronym Owner

It was used before Paul's patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@retronym
Owner

I'm also thinking I've narrowed the error message too far. I suppose you could get the same error when using an incorrect version of a classfile, not just when it is missing completely.

@retronym
Owner

@xeno-by I've tried to make the trip-wire for this error more clear, and I've also expanded the error message with other possibilities.

src/reflect/scala/reflect/internal/Types.scala
@@ -1385,7 +1385,11 @@ 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 {
+ // SI-6440 allow StubSymbols to reveal what's missing from the classpath
@gkossakowski Owner

You meant SI-6640 here, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gkossakowski

How do we know that call to sym.failIfStub() is only needed in ThisType?

retronym added some commits
@retronym retronym SI-6640 Better reporting of deficient classpaths.
In a55788e, StubSymbols were introduced to fail-slow when
the classpath was deficient. This allowed compilation to
succeed in cases when one didn't actually use the part of
class A which referred to some missing class B.

But a few problems were introduced.

Firstly, when the deferred error eventually happened, it was
signalled with abort(msg), rather than through a thrown
MissingRequirementError. The latter is desirable, as it doesn't
lead to printing a stack trace.

Second, the actual error message changed, and no longer
included the name of the class file that refers to the missing
class.

Finally, it seems that we can end up with a stub term symbol
in a situation where a class symbol is desired. An assertion
in the constructor of ThisType throws trips when calling .isClass,
before the useful error message from StubSymbol can be emitted.

This commit addresses these points, and rewords the error
a little to be more accessible. The last point is the most fragile
in this arrangement, there might be some whack-a-mole
required to find other places that also need this.
I don't see a clean solution for this, but am open to suggestions.

We should really build a facility in partest to delete
specified classfiles between groups in separate compilation
tests, in order to have tests for this. I'll work on that as a followup.

For now, here's the result of my manual testing:

[info] Set current project to default-821d14 (in build file:/Users/jason/code/scratch1/)
> compile

[info] Compiling 1 Scala source to /Users/jason/code/scratch1/target/scala-2.10/classes...
[error]
[error]      while compiling: /Users/jason/code/scratch1/test.scala
[error]         during phase: typer
[error]      library version: version 2.10.0-RC2
[error]     compiler version: version 2.10.0-RC2
...
[error]   last tree to typer: Ident(SwingWorker)
[error]               symbol: <none> (flags: )
[error]    symbol definition: <none>
[error]        symbol owners:
[error]       context owners: object Test -> package <empty>
 ...
[error] uncaught exception during compilation: java.lang.AssertionError
[trace] Stack trace suppressed: run last compile:compile for the full output.
[error] (compile:compile) java.lang.AssertionError: assertion failed: value actors
[error] Total time: 2 s, completed Nov 10, 2012 3:18:34 PM
>
> set scalaHome := Some(file("/Users/jason/code/scala/build/pack"))
[info] Defining *:scala-home
[info] The new value will be used by no settings or tasks.
[info] Reapplying settings...
[info] Set current project to default-821d14 (in build file:/Users/jason/code/scratch1/)
^[compile
[info] Compiling 1 Scala source to /Users/jason/code/scratch1/target/scala-2.10/classes...
[error] /Users/jason/code/scratch1/test.scala:4: A signature in SwingWorker.class refers to term actors in package scala which is missing from the classpath.
[error] object Test extends SwingWorker
[error]                     ^
[error] one error found
[error] (compile:compile) Compilation failed
[error] Total time: 2 s, completed Nov 10, 2012 3:18:45 PM
0b59b46
@retronym retronym Refine the message and triggering of MissingRequirementError.
 - To force a failure of the stub, call a new method `failIfStub`
   rather than `info`.
 - Offer a broader range of potential root causes in the
   error message.
86e045e
@retronym
Owner

Comment fixed.

We don't know if it's the only place; that's the "whack-a-mole" i referred to in the commit comment. The stub symbols intercept calls to info which catches the vast majority of places; I looked around for other calls to isClass assertions in the vicinity and couldn't find others.

To flush more out, we could take a big codebase with two modules (lets call them "lib" an "app"), compile it all, delete some classfiles at random from "lib", and recompile "app". That should never crash.

But the crash only comes in place of an otherwise failed compilation, so I think we can take some risk here.

@scala-jenkins

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1614/

@scala-jenkins

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1614/

@retronym retronym SI-6440 Address regressions around MissingRequirementError
 Go back to using globalError to report when a stub's info is referenced,
 and only throw the MissingRequirementError when compilation really
 must abort due to having a StubTermSymbol in a place where a
 StubClassSymbol would have been a better choice.

 This situation arises when an entire package is missing from the
 classpath, as was the case in the reported bug.

 Adds `StoreReporterDirectTest`, which buffers messages issued
 during compilation for more structured interrogation. Use this
 in two test for manifests -- these tests were using a crude means
 of grepping compiler console output to focus on the relevant output,
 but this approach was insufficient with the new multi-line error
 message emitted as part of this change.

 Also used that base test class to add two new tests: one for
 the reported error (package missing), and another for a simpler
 error (class missing). The latter test shows how stub symbols
 allow code to compile if it doesn't the subset of signatures
 in some type that refer to a missing class.

 Gave the INFO/WARNING/ERROR members of Reporter sensible
 toString implementations; they inherit from Enumeration#Value
 in an unusual manner (why?) that means the built in toString of
 Enumeration printed `Severity@0`.
0792966
@retronym
Owner

This was more subtle than I originally thought. I've addressed the failing tests and added some new ones in the latest commit.

Review by @xeno-by @paulp

@paulp paulp commented on the diff
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
@paulp
paulp added a note

Ending with ERROR has a bit of a DOS feel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xeno-by

Another option would be using toolboxes

@scala-jenkins

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1628/

@scala-jenkins

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1628/

@adriaanm adriaanm commented on the diff
src/reflect/scala/reflect/internal/Types.scala
@@ -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)
@adriaanm Owner

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.

@retronym Owner

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.

@adriaanm Owner

I imagined isClass on a StubSymbol would throw the MissingRequirementException

@retronym Owner

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gkossakowski

@retronym: I think your approach of using DirectTest and capturing reported errors make perfect sense. I have an impression that you are looking for better alternative. What's the problem with this approach?

@retronym
Owner

@gkossakowski I push -f-ed, but forgot to update the PR description. The DirectTest approach is workable, although quite slow.

@adriaanm
Owner

LGTM, thanks @retronym -- it's great this now comes with tests

@jsuereth jsuereth merged commit aef7912 into scala:2.10.0-wip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 10, 2012
  1. @retronym

    SI-6640 Better reporting of deficient classpaths.

    retronym authored
    In a55788e, StubSymbols were introduced to fail-slow when
    the classpath was deficient. This allowed compilation to
    succeed in cases when one didn't actually use the part of
    class A which referred to some missing class B.
    
    But a few problems were introduced.
    
    Firstly, when the deferred error eventually happened, it was
    signalled with abort(msg), rather than through a thrown
    MissingRequirementError. The latter is desirable, as it doesn't
    lead to printing a stack trace.
    
    Second, the actual error message changed, and no longer
    included the name of the class file that refers to the missing
    class.
    
    Finally, it seems that we can end up with a stub term symbol
    in a situation where a class symbol is desired. An assertion
    in the constructor of ThisType throws trips when calling .isClass,
    before the useful error message from StubSymbol can be emitted.
    
    This commit addresses these points, and rewords the error
    a little to be more accessible. The last point is the most fragile
    in this arrangement, there might be some whack-a-mole
    required to find other places that also need this.
    I don't see a clean solution for this, but am open to suggestions.
    
    We should really build a facility in partest to delete
    specified classfiles between groups in separate compilation
    tests, in order to have tests for this. I'll work on that as a followup.
    
    For now, here's the result of my manual testing:
    
    [info] Set current project to default-821d14 (in build file:/Users/jason/code/scratch1/)
    > compile
    
    [info] Compiling 1 Scala source to /Users/jason/code/scratch1/target/scala-2.10/classes...
    [error]
    [error]      while compiling: /Users/jason/code/scratch1/test.scala
    [error]         during phase: typer
    [error]      library version: version 2.10.0-RC2
    [error]     compiler version: version 2.10.0-RC2
    ...
    [error]   last tree to typer: Ident(SwingWorker)
    [error]               symbol: <none> (flags: )
    [error]    symbol definition: <none>
    [error]        symbol owners:
    [error]       context owners: object Test -> package <empty>
     ...
    [error] uncaught exception during compilation: java.lang.AssertionError
    [trace] Stack trace suppressed: run last compile:compile for the full output.
    [error] (compile:compile) java.lang.AssertionError: assertion failed: value actors
    [error] Total time: 2 s, completed Nov 10, 2012 3:18:34 PM
    >
    > set scalaHome := Some(file("/Users/jason/code/scala/build/pack"))
    [info] Defining *:scala-home
    [info] The new value will be used by no settings or tasks.
    [info] Reapplying settings...
    [info] Set current project to default-821d14 (in build file:/Users/jason/code/scratch1/)
    ^[compile
    [info] Compiling 1 Scala source to /Users/jason/code/scratch1/target/scala-2.10/classes...
    [error] /Users/jason/code/scratch1/test.scala:4: A signature in SwingWorker.class refers to term actors in package scala which is missing from the classpath.
    [error] object Test extends SwingWorker
    [error]                     ^
    [error] one error found
    [error] (compile:compile) Compilation failed
    [error] Total time: 2 s, completed Nov 10, 2012 3:18:45 PM
  2. @retronym

    Refine the message and triggering of MissingRequirementError.

    retronym authored
     - To force a failure of the stub, call a new method `failIfStub`
       rather than `info`.
     - Offer a broader range of potential root causes in the
       error message.
Commits on Nov 13, 2012
  1. @retronym

    SI-6440 Address regressions around MissingRequirementError

    retronym authored
     Go back to using globalError to report when a stub's info is referenced,
     and only throw the MissingRequirementError when compilation really
     must abort due to having a StubTermSymbol in a place where a
     StubClassSymbol would have been a better choice.
    
     This situation arises when an entire package is missing from the
     classpath, as was the case in the reported bug.
    
     Adds `StoreReporterDirectTest`, which buffers messages issued
     during compilation for more structured interrogation. Use this
     in two test for manifests -- these tests were using a crude means
     of grepping compiler console output to focus on the relevant output,
     but this approach was insufficient with the new multi-line error
     message emitted as part of this change.
    
     Also used that base test class to add two new tests: one for
     the reported error (package missing), and another for a simpler
     error (class missing). The latter test shows how stub symbols
     allow code to compile if it doesn't the subset of signatures
     in some type that refer to a missing class.
    
     Gave the INFO/WARNING/ERROR members of Reporter sensible
     toString implementations; they inherit from Enumeration#Value
     in an unusual manner (why?) that means the built in toString of
     Enumeration printed `Severity@0`.
This page is out of date. Refresh to see the latest.
View
12 src/compiler/scala/tools/nsc/reporters/Reporter.scala
@@ -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
View
8 src/partest/scala/tools/partest/DirectTest.scala
@@ -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
@@ -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)
}
View
15 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
+}
View
31 src/reflect/scala/reflect/internal/Symbols.scala
@@ -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")
@@ -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
@@ -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
@@ -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)
@xeno-by Owner
xeno-by added a note

I wonder why Paul wrote locally here.

@retronym Owner

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

@paulp
paulp added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
- 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
View
7 src/reflect/scala/reflect/internal/Types.scala
@@ -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)
@adriaanm Owner

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.

@retronym Owner

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.

@adriaanm Owner

I imagined isClass on a StubSymbol would throw the MissingRequirementException

@retronym Owner

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (!sym.isClass) {
+ // 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
View
14 src/reflect/scala/reflect/internal/pickling/UnPickler.scala
@@ -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._
@@ -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)
}
}
}
@@ -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)
- 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}"
- )
-
@xeno-by Owner
xeno-by added a note

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

@retronym Owner

It was used before Paul's patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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)
View
10 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
View
5 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
@paulp
paulp added a note

Ending with ERROR has a bit of a DOS feel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
47 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"))
+ }
}
View
4 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
View
61 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"))
+ }
+}
View
5 test/files/run/typetags_without_scala_reflect_typetag_lookup.check
@@ -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
View
14 test/files/run/typetags_without_scala_reflect_typetag_lookup.scala
@@ -1,6 +1,6 @@
import scala.tools.partest._
-object Test extends DirectTest {
+object Test extends StoreReporterDirectTest {
def code = ???
def library = """
@@ -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"))
}
-}
+}
View
5 test/files/run/typetags_without_scala_reflect_typetag_manifest_interop.check
@@ -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
View
15 test/files/run/typetags_without_scala_reflect_typetag_manifest_interop.scala
@@ -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 = """
@@ -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"))
}
-}
+}
Something went wrong with that request. Please try again.