Address review comments

Tighten some types (Symbol -> ClassSymbol / ModuleSymbol), use NonFatal
instead of catching Throwable.

Also don't run the classfile parser enteringPhase(phaseBeforeRefchecks)
anymore. This was added in 0ccdb15 but seems no longer required.

5 files changed, 41 insertions, 40 deletions

diff --git a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
index eb01c8dc44..d948d151a6 100644
--- a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
+++ b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
@@ -52,7 +52,7 @@ abstract class SymbolLoaders {
     })
   }
 
-  def newClass(owner: Symbol, name: String): Symbol = owner.newClass(newTypeName(name))
+  def newClass(owner: Symbol, name: String): ClassSymbol = owner.newClass(newTypeName(name))
 
   /** Enter class with given `name` into scope of `root`
    *  and give them `completer` as type.
@@ -60,12 +60,12 @@ abstract class SymbolLoaders {
   def enterClass(owner: Symbol, name: String, completer: SymbolLoader): Symbol =
     enterClass(owner, newClass(owner, name), completer)
 
-  def enterClass(owner: Symbol, clazz: Symbol, completer: SymbolLoader): Symbol = {
+  def enterClass(owner: Symbol, clazz: ClassSymbol, completer: SymbolLoader): Symbol = {
     clazz setInfo completer
     enterIfNew(owner, clazz, completer)
   }
 
-  def newModule(owner: Symbol, name: String): Symbol = owner.newModule(newTermName(name))
+  def newModule(owner: Symbol, name: String): ModuleSymbol = owner.newModule(newTermName(name))
 
   /** Enter module with given `name` into scope of `root`
    *  and give them `completer` as type.
@@ -73,7 +73,7 @@ abstract class SymbolLoaders {
   def enterModule(owner: Symbol, name: String, completer: SymbolLoader): Symbol =
     enterModule(owner, newModule(owner, name), completer)
 
-  def enterModule(owner: Symbol, module: Symbol, completer: SymbolLoader): Symbol = {
+  def enterModule(owner: Symbol, module: ModuleSymbol, completer: SymbolLoader): Symbol = {
     module setInfo completer
     module.moduleClass setInfo moduleClassLoader
     enterIfNew(owner, module, completer)
@@ -121,7 +121,7 @@ abstract class SymbolLoaders {
   /** Enter class and module with given `name` into scope of `root`
    *  and give them `completer` as type.
    */
-  def enterClassAndModule(root: Symbol, name: String, getCompleter: (Symbol, Symbol) => SymbolLoader) {
+  def enterClassAndModule(root: Symbol, name: String, getCompleter: (ClassSymbol, ModuleSymbol) => SymbolLoader) {
     val clazz = newClass(root, name)
     val module = newModule(root, name)
     val completer = getCompleter(clazz, module)
@@ -281,7 +281,7 @@ abstract class SymbolLoaders {
     }
   }
 
-  class ClassfileLoader(val classfile: AbstractFile, clazz: Symbol, module: Symbol) extends SymbolLoader with FlagAssigningCompleter {
+  class ClassfileLoader(val classfile: AbstractFile, clazz: ClassSymbol, module: ModuleSymbol) extends SymbolLoader with FlagAssigningCompleter {
     private object classfileParser extends {
       val symbolTable: SymbolLoaders.this.symbolTable.type = SymbolLoaders.this.symbolTable
     } with ClassfileParser {
@@ -308,13 +308,7 @@ abstract class SymbolLoaders {
 
     protected def doComplete(root: Symbol) {
       val start = if (Statistics.canEnable) Statistics.startTimer(classReadNanos) else null
-
-      // Running the classfile parser after refchecks can lead to "illegal class file dependency"
-      // errors. More concretely, the classfile parser calls "sym.companionModule", which calls
-      // "isModuleNotMethod" on the companion. After refchecks, this method forces the info, which
-      // may run the classfile parser. This produces the error.
-      enteringPhase(phaseBeforeRefchecks)(classfileParser.parse(classfile, clazz, module))
-
+      classfileParser.parse(classfile, clazz, module)
       if (root.associatedFile eq NoAbstractFile) {
         root match {
           // In fact, the ModuleSymbol forwards its setter to the module class
diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
index e136fdf6d1..7e81fad606 100644
--- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
+++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
@@ -10,6 +10,7 @@ package classfile
 
 import java.io.{File, IOException}
 import java.lang.Integer.toHexString
+
 import scala.collection.{immutable, mutable}
 import scala.collection.mutable.{ArrayBuffer, ListBuffer}
 import scala.annotation.switch
@@ -18,6 +19,7 @@ import scala.reflect.internal.pickling.{ByteCodecs, PickleBuffer}
 import scala.reflect.io.NoAbstractFile
 import scala.tools.nsc.util.ClassPath
 import scala.tools.nsc.io.AbstractFile
+import scala.util.control.NonFatal
 
 /** This abstract class implements a class file parser.
  *
@@ -53,18 +55,18 @@ abstract class ClassfileParser {
   protected type ThisConstantPool <: ConstantPool
   protected def newConstantPool: ThisConstantPool
 
-  protected var file: AbstractFile     = _  // the class file
-  protected var in: AbstractFileReader = _  // the class file reader
-  protected var clazz: Symbol = _           // the class symbol containing dynamic members
-  protected var staticModule: Symbol = _    // the module symbol containing static members
-  protected var instanceScope: Scope = _    // the scope of all instance definitions
-  protected var staticScope: Scope = _      // the scope of all static definitions
-  protected var pool: ThisConstantPool = _  // the classfile's constant pool
-  protected var isScala: Boolean = _        // does class file describe a scala class?
-  protected var isScalaAnnot: Boolean = _   // does class file describe a scala class with its pickled info in an annotation?
-  protected var isScalaRaw: Boolean = _     // this class file is a scala class with no pickled info
-  protected var busy: Symbol = _            // lock to detect recursive reads
-  protected var currentClass: Name = _      // JVM name of the current class
+  protected var file: AbstractFile     = _     // the class file
+  protected var in: AbstractFileReader = _     // the class file reader
+  protected var clazz: ClassSymbol = _         // the class symbol containing dynamic members
+  protected var staticModule: ModuleSymbol = _ // the module symbol containing static members
+  protected var instanceScope: Scope = _       // the scope of all instance definitions
+  protected var staticScope: Scope = _         // the scope of all static definitions
+  protected var pool: ThisConstantPool = _     // the classfile's constant pool
+  protected var isScala: Boolean = _           // does class file describe a scala class?
+  protected var isScalaAnnot: Boolean = _      // does class file describe a scala class with its pickled info in an annotation?
+  protected var isScalaRaw: Boolean = _        // this class file is a scala class with no pickled info
+  protected var busy: Symbol = _               // lock to detect recursive reads
+  protected var currentClass: Name = _         // JVM name of the current class
   protected var classTParams = Map[Name,Symbol]()
   protected var srcfile0 : Option[AbstractFile] = None
   protected def moduleClass: Symbol = staticModule.moduleClass
@@ -132,7 +134,16 @@ abstract class ClassfileParser {
     finally loaders.parentsLevel -= 1
   }
 
-  def parse(file: AbstractFile, clazz: Symbol, module: Symbol): Unit = {
+  /**
+   * `clazz` and `module` are the class and module symbols corresponding to the classfile being
+   * parsed. Note that the ClassfileLoader unconditionally creates both of these symbols, they may
+   * may get invalidated later on (.exists).
+   *
+   * Note that using `companionModule` / `companionClass` does not always work to navigate between
+   * those two symbols, namely when they are shadowed by a type / value in the a package object
+   * (scala-dev#248).
+   */
+  def parse(file: AbstractFile, clazz: ClassSymbol, module: ModuleSymbol): Unit = {
     this.file = file
     pushBusy(clazz) {
       this.in           = new AbstractFileReader(file)
@@ -973,11 +984,9 @@ abstract class ClassfileParser {
       }
       if (hasError) None
       else Some(AnnotationInfo(attrType, List(), nvpairs.toList))
-    }
-    catch {
-      case f: FatalError       => throw f  // don't eat fatal errors, they mean a class was not found
-      case ex: java.lang.Error => throw ex
-      case ex: Throwable       =>
+    } catch {
+      case f: FatalError => throw f  // don't eat fatal errors, they mean a class was not found
+      case NonFatal(ex)  =>
         // We want to be robust when annotations are unavailable, so the very least
         // we can do is warn the user about the exception
         // There was a reference to ticket 1135, but that is outdated: a reference to a class not on
@@ -986,7 +995,6 @@ abstract class ClassfileParser {
         // and that should never be swallowed silently.
         warning(s"Caught: $ex while parsing annotations in ${in.file}")
         if (settings.debug) ex.printStackTrace()
-
         None // ignore malformed annotations
     }
 
@@ -1093,8 +1101,6 @@ abstract class ClassfileParser {
       val attrName = readTypeName()
       val attrLen = u4
       attrName match {
-        case tpnme.SignatureATTR =>
-          in.skip(attrLen)
         case tpnme.ScalaSignatureATTR =>
           isScala = true
           val pbuf = new PickleBuffer(in.buf, in.bp, in.bp + attrLen)
diff --git a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala
index 016351c639..6dea184826 100644
--- a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala
+++ b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala
@@ -17,6 +17,7 @@ import PickleFormat._
 import scala.collection.mutable
 import scala.collection.mutable.ListBuffer
 import scala.annotation.switch
+import scala.util.control.NonFatal
 
 /** @author Martin Odersky
  *  @version 1.0
@@ -33,18 +34,18 @@ abstract class UnPickler {
    *  @param moduleRoot the top-level module which is unpickled
    *  @param filename   filename associated with bytearray, only used for error messages
    */
-  def unpickle(bytes: Array[Byte], offset: Int, classRoot: Symbol, moduleRoot: Symbol, filename: String) {
+  def unpickle(bytes: Array[Byte], offset: Int, classRoot: ClassSymbol, moduleRoot: ModuleSymbol, filename: String) {
     try {
       assert(classRoot != NoSymbol && moduleRoot != NoSymbol, s"The Unpickler expects a class and module symbol: $classRoot - $moduleRoot")
       new Scan(bytes, offset, classRoot, moduleRoot, filename).run()
     } catch {
-      case ex: Throwable =>
+      case NonFatal(ex) =>
         /*if (settings.debug.value)*/ ex.printStackTrace()
         throw new RuntimeException("error reading Scala signature of "+filename+": "+ex.getMessage())
     }
   }
 
-  class Scan(_bytes: Array[Byte], offset: Int, classRoot: Symbol, moduleRoot: Symbol, filename: String) extends PickleBuffer(_bytes, offset, -1) {
+  class Scan(_bytes: Array[Byte], offset: Int, classRoot: ClassSymbol, moduleRoot: ModuleSymbol, filename: String) extends PickleBuffer(_bytes, offset, -1) {
     //println("unpickle " + classRoot + " and " + moduleRoot)//debug
 
     protected def debug = settings.debug.value
diff --git a/src/reflect/scala/reflect/runtime/JavaMirrors.scala b/src/reflect/scala/reflect/runtime/JavaMirrors.scala
index 9b0d66f41c..95440ebc00 100644
--- a/src/reflect/scala/reflect/runtime/JavaMirrors.scala
+++ b/src/reflect/scala/reflect/runtime/JavaMirrors.scala
@@ -578,7 +578,7 @@ private[scala] trait JavaMirrors extends internal.SymbolTable with api.JavaUnive
      *  @param   jclazz  The Java class which contains the unpickled information in a
      *                   ScalaSignature or ScalaLongSignature annotation.
      */
-    def unpickleClass(clazz: Symbol, module: Symbol, jclazz: jClass[_]): Unit = {
+    def unpickleClass(clazz: ClassSymbol, module: ModuleSymbol, jclazz: jClass[_]): Unit = {
       def markAbsent(tpe: Type) = setAllInfos(clazz, module, tpe)
       def handleError(ex: Exception) = {
         markAbsent(ErrorType)
diff --git a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
index 768a3d5ce5..3f2864ee7b 100644
--- a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
+++ b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
@@ -14,7 +14,7 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
    *  by unpickling information from the corresponding Java class. If no Java class
    *  is found, a package is created instead.
    */
-  class TopClassCompleter(clazz: Symbol, module: Symbol) extends SymLoader with FlagAssigningCompleter {
+  class TopClassCompleter(clazz: ClassSymbol, module: ModuleSymbol) extends SymLoader with FlagAssigningCompleter {
     markFlagsCompleted(clazz, module)(mask = ~TopLevelPickledFlags)
     override def complete(sym: Symbol) = {
       debugInfo("completing "+sym+"/"+clazz.fullName)
@@ -36,7 +36,7 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
    *  @param name    The simple name of the newly created class
    *  @param completer  The completer to be used to set the info of the class and the module
    */
-  protected def initAndEnterClassAndModule(owner: Symbol, name: TypeName, completer: (Symbol, Symbol) => LazyType) = {
+  protected def initAndEnterClassAndModule(owner: Symbol, name: TypeName, completer: (ClassSymbol, ModuleSymbol) => LazyType) = {
     assert(!(name.toString endsWith "[]"), name)
     val clazz = owner.newClass(name)
     val module = owner.newModule(name.toTermName)