The second piece of the flags patch.

into Modifiers and Symbol, but touches as little as possible beyond
that. It also includes some lengthy commentary (see HasFlags.scala) on
the state of the flags and some of the remaining issues. One more patch
which unfortunately but unavoidably touches almost every file in the
compiler lies ahead.

The floor is still open! But no review.

4 files changed, 118 insertions, 55 deletions

diff --git a/src/library/scala/reflect/generic/HasFlags.scala b/src/library/scala/reflect/generic/HasFlags.scala
index 00566995e2..7dfb3506f6 100644
--- a/src/library/scala/reflect/generic/HasFlags.scala
+++ b/src/library/scala/reflect/generic/HasFlags.scala
@@ -1,6 +1,76 @@
 package scala.reflect
 package generic
 
+/** ISSUE #1: Flag names vs. Test method names
+ *
+ *  The following methods from Symbol have a name of
+ *  the form isFoo where FOO is the name of a flag, but where the method
+ *  body tests for more than whether the flag is set.
+ *
+ *  There are two possibilities with such methods.  Either the extra
+ *  tests are strictly to partition among overloaded flags (which is
+ *  the case we can live with in the short term, if each such flag's
+ *  partitioning assumptions are documented) or they aren't.
+ *
+ *  The second case implies that "x hasFlag FOO" and "x.isFoo" have
+ *  different semantics, and this we can't live with, because even if
+ *  we're smart enough to avoid being tripped up by that, the next guy isn't.
+ *
+ *  No extreme measures necessary, only renaming isFoo to something
+ *  which hews more closely to its implementation.  (Or renaming the flag.)
+ *
+    // Defined in the compiler Symbol
+    //
+    final def isLabel = isMethod && !hasFlag(ACCESSOR) && hasFlag(LABEL)
+    final def isLocal: Boolean = owner.isTerm
+    final def isModuleVar: Boolean = isVariable && hasFlag(MODULEVAR)
+    final def isStable =
+      isTerm &&
+      !hasFlag(MUTABLE) &&
+      (!hasFlag(METHOD | BYNAMEPARAM) || hasFlag(STABLE)) &&
+      !(tpe.isVolatile && !hasAnnotation(uncheckedStableClass))
+    final def isStatic: Boolean =
+      hasFlag(STATIC) || isRoot || owner.isStaticOwner
+    override final def isTrait: Boolean =
+      isClass && hasFlag(TRAIT | notDEFERRED)     // A virtual class becomes a trait (part of DEVIRTUALIZE)
+
+    // Defined in the library Symbol
+    //
+          def isTrait: Boolean = isClass && hasFlag(TRAIT) // refined later for virtual classes.
+    final def isContravariant = isType && hasFlag(CONTRAVARIANT)
+    final def isCovariant = isType && hasFlag(COVARIANT)
+    final def isMethod = isTerm && hasFlag(METHOD)
+    final def isModule = isTerm && hasFlag(MODULE)
+    final def isPackage = isModule && hasFlag(PACKAGE)
+ *
+ */
+
+/** ISSUE #2: Implicit flag relationships must be made explicit.
+ *
+ *  For instance, every time the MODULE flag is set, the FINAL flag is
+ *  set along with it:
+ *
+   .setFlag(FINAL | MODULE | PACKAGE | JAVA)
+   .setFlag(FINAL | MODULE | PACKAGE | JAVA).setInfo(rootLoader)
+   new ModuleSymbol(this, pos, name).setFlag(MODULE | FINAL)
+   new ModuleSymbol(this, pos, name).setFlag(MODULE | FINAL)
+   val m = new ModuleSymbol(this, pos, name).setFlag(MODULE | FINAL)
+   setFlag(module.getFlag(ModuleToClassFlags) | MODULE | FINAL)
+   sourceModule.flags = MODULE | FINAL
+
+ * However the same is not true of when the MODULE flag is cleared:
+
+    sym.resetFlag(MODULE)
+    .setFlag(sym.flags | STABLE).resetFlag(MODULE)
+    sym.resetFlag(MODULE | FINAL | CASE)
+
+ *  It's not relevant whether this example poses any issues: we must
+ *  not tolerate these uncertainties.  If the flags are to move together
+ *  then both setting and clearing have to be encapsulated.  If there
+ *  is a useful and used distinction between the various permutations
+ *  of on and off, then it must be documented.  It's the only way!
+ */
+
 import Flags._
 
 /** Common code utilized by Modifiers (which carry the flags associated
@@ -63,6 +133,10 @@ trait HasFlags {
    */
   def hasAllFlags(mask: Long): Boolean
 
+  /** Whether this entity has NONE of the flags in the given mask.
+   */
+  def hasNoFlags(mask: Long): Boolean = !hasFlag(mask)
+
   // Tests which come through cleanly: both Symbol and Modifiers use these
   // identically, testing for a single flag.
   def isCase          = hasFlag(CASE     )
@@ -97,11 +171,16 @@ trait HasFlags {
 
   // Problematic:
   // DEFAULTPARAM overloaded with TRAIT
-  def hasDefault       = isParameter && hasFlag(DEFAULTPARAM)
-  def hasDefaultFlag   = hasFlag(DEFAULTPARAM)
+  def hasDefault     = isParameter && hasFlag(DEFAULTPARAM)
+  def hasDefaultFlag = hasFlag(DEFAULTPARAM)
   // def isTrait          = hasFlag(TRAIT    )
   // def isTrait: Boolean = isClass && hasFlag(TRAIT) // refined later for virtual classes.
 
+  // Straightforwardly named accessors already being used differently
+  def hasStaticFlag  = hasFlag(STATIC)
+  def hasLocalFlag   = hasFlag(LOCAL)
+  def hasModuleFlag  = hasFlag(MODULE)
+
   // Problematic:
   // ABSTRACT and DEFERRED too easy to confuse, and
   // ABSTRACT + OVERRIDE ==> ABSOVERRIDE adds to it.
diff --git a/src/library/scala/reflect/generic/Symbols.scala b/src/library/scala/reflect/generic/Symbols.scala
index a1c9a0c18d..75e1c05d49 100755
--- a/src/library/scala/reflect/generic/Symbols.scala
+++ b/src/library/scala/reflect/generic/Symbols.scala
@@ -7,7 +7,12 @@ trait Symbols { self: Universe =>
 
   type Symbol >: Null <: AbsSymbol
 
-  abstract class AbsSymbol { this: Symbol =>
+  abstract class AbsSymbol extends HasFlags {
+    this: Symbol =>
+
+    type FlagsType          = Long
+    type AccessBoundaryType = Symbol
+    type AnnotationType     = AnnotationInfo
 
     /** The owner of this symbol.
      */
@@ -56,6 +61,8 @@ trait Symbols { self: Universe =>
      */
     def privateWithin: Symbol
 
+    final def hasAccessBoundary = (privateWithin != null) && (privateWithin != NoSymbol)
+
     /** The raw info of the type
      */
     def rawInfo: Type
@@ -122,47 +129,27 @@ trait Symbols { self: Universe =>
     private[scala] def isSkolem = false // to be overridden
 
           def isTrait: Boolean = isClass && hasFlag(TRAIT) // refined later for virtual classes.
-    final def hasDefault = isParameter && hasFlag(DEFAULTPARAM)
     final def isAbstractClass = isClass && hasFlag(ABSTRACT)
-    // XXX This is unlikely to be correct: it's not looking for the ABSOVERRIDE flag?
-    final def isAbstractOverride = isTerm && hasFlag(ABSTRACT) && hasFlag(OVERRIDE)
+    final def isAbstractOverride = isTerm && hasFlag(ABSOVERRIDE)
     final def isBridge = hasFlag(BRIDGE)
-    final def isCase = hasFlag(CASE)
-    final def isCaseAccessor = hasFlag(CASEACCESSOR)
     final def isContravariant = isType && hasFlag(CONTRAVARIANT)
     final def isCovariant = isType && hasFlag(COVARIANT)
-    final def isDeferred = hasFlag(DEFERRED) && !isClass
     final def isEarlyInitialized: Boolean = isTerm && hasFlag(PRESUPER)
     final def isExistentiallyBound = isType && hasFlag(EXISTENTIAL)
-    final def isFinal = hasFlag(FINAL)
     final def isGetterOrSetter = hasFlag(ACCESSOR)
     final def isImplClass = isClass && hasFlag(IMPLCLASS) // Is this symbol an implementation class for a mixin?
-    final def isImplicit = hasFlag(IMPLICIT)
     final def isInterface = hasFlag(INTERFACE)
     final def isJavaDefined = hasFlag(JAVA)
-    final def isLazy = hasFlag(LAZY)
+    final def isLazyAccessor = isLazy && lazyAccessor != NoSymbol
     final def isMethod = isTerm && hasFlag(METHOD)
     final def isModule = isTerm && hasFlag(MODULE)
     final def isModuleClass = isClass && hasFlag(MODULE)
-    final def isMutable = hasFlag(MUTABLE)
     final def isOverloaded = hasFlag(OVERLOADED)
-    final def isOverride = hasFlag(OVERRIDE)
-    final def isParamAccessor = hasFlag(PARAMACCESSOR)
-    final def isParameter = hasFlag(PARAM)
     final def isRefinementClass = isClass && name == mkTypeName(nme.REFINE_CLASS_NAME)
-    final def isSealed = isClass && (hasFlag(SEALED) || definitions.isValueClass(this))
-    final def isSourceMethod = isTerm && (flags & (METHOD | STABLE)) == METHOD // exclude all accessors!!!
+    final def isSourceMethod = isMethod && !hasFlag(STABLE) // exclude all accessors!!!
     final def isSuperAccessor = hasFlag(SUPERACCESSOR)
-    final def isSynthetic = hasFlag(SYNTHETIC)
     final def isTypeParameter = isType && isParameter && !isSkolem
 
-    /** Access tests */
-    final def isPrivate = hasFlag(PRIVATE)
-    final def isPrivateLocal = hasFlag(PRIVATE) && hasFlag(LOCAL)
-    final def isProtected = hasFlag(PROTECTED)
-    final def isProtectedLocal = hasFlag(PROTECTED) && hasFlag(LOCAL)
-    final def isPublic = !hasFlag(PRIVATE | PROTECTED) && privateWithin == NoSymbol
-
     /** Package tests */
     final def isEmptyPackage = isPackage && name == nme.EMPTY_PACKAGE_NAME
     final def isEmptyPackageClass = isPackageClass && name == mkTypeName(nme.EMPTY_PACKAGE_NAME)
diff --git a/src/library/scala/reflect/generic/Trees.scala b/src/library/scala/reflect/generic/Trees.scala
index 87ce15dd24..7ab4cf882b 100755
--- a/src/library/scala/reflect/generic/Trees.scala
+++ b/src/library/scala/reflect/generic/Trees.scala
@@ -1,7 +1,7 @@
 package scala.reflect
 package generic
 
-import java.io.{PrintWriter, StringWriter}
+import java.io.{ PrintWriter, StringWriter }
 import Flags._
 
 trait Trees { self: Universe =>
@@ -15,33 +15,29 @@ trait Trees { self: Universe =>
 
   private[scala] var nodeCount = 0
 
+  protected def flagsIntoString(flags: Long, privateWithin: String): String
+
   /** @param privateWithin the qualifier for a private (a type name)
    *    or nme.EMPTY.toTypeName, if none is given.
    *  @param annotations the annotations for the definition.
    *    <strong>Note:</strong> the typechecker drops these annotations,
    *    use the AnnotationInfo's (Symbol.annotations) in later phases.
    */
-  case class Modifiers(flags: Long, privateWithin: Name, annotations: List[Tree], positions: Map[Long, Position]) {
-    def isAbstract      = hasFlag(ABSTRACT )
-    def isAccessor      = hasFlag(ACCESSOR )
-    def isArgument      = hasFlag(PARAM    )
-    def isCase          = hasFlag(CASE     )
-    def isContravariant = hasFlag(CONTRAVARIANT)  // marked with `-'
-    def isCovariant     = hasFlag(COVARIANT    )  // marked with `+'
-    def isDeferred      = hasFlag(DEFERRED )
-    def isFinal         = hasFlag(FINAL    )
-    def isImplicit      = hasFlag(IMPLICIT )
-    def isLazy          = hasFlag(LAZY     )
-    def isOverride      = hasFlag(OVERRIDE )
-    def isPrivate       = hasFlag(PRIVATE  )
-    def isProtected     = hasFlag(PROTECTED)
-    def isPublic        = !isPrivate && !isProtected
-    def isSealed        = hasFlag(SEALED   )
-    def isSynthetic     = hasFlag(SYNTHETIC)
-    def isTrait         = hasFlag(TRAIT    )
-    def isVariable      = hasFlag(MUTABLE  )
+  case class Modifiers(flags: Long, privateWithin: Name, annotations: List[Tree], positions: Map[Long, Position]) extends HasFlags {
+    /* Abstract types from HasFlags. */
+    type FlagsType          = Long
+    type AccessBoundaryType = Name
+    type AnnotationType     = Tree
+
+    private val emptyTypeName = mkTypeName(nme.EMPTY)
 
+    def hasAccessBoundary = privateWithin != emptyTypeName
+    def hasAllFlags(mask: Long): Boolean = (flags & mask) == mask
     def hasFlag(flag: Long) = (flag & flags) != 0L
+    def hasFlagsToString(mask: Long): String = flagsToString(
+      flags & mask,
+      if (hasAccessBoundary) privateWithin.toString else ""
+    )
     def & (flag: Long): Modifiers = {
       val flags1 = flags & flag
       if (flags1 == flags) this
@@ -62,6 +58,8 @@ trait Trees { self: Universe =>
       else copy(annotations = annotations ::: annots)
     def withPosition(flag: Long, position: Position) =
       copy(positions = positions + (flag -> position))
+
+    override def toString = "Modifiers(%s, %s, %s)".format(hasFlagsToString(-1L), annotations mkString ", ", positions)
   }
 
   def Modifiers(flags: Long, privateWithin: Name): Modifiers = Modifiers(flags, privateWithin, List(), Map.empty)
@@ -101,6 +99,8 @@ trait Trees { self: Universe =>
     def isDef = false
     def isEmpty = false
 
+    def hasSymbolWhich(f: Symbol => Boolean) = hasSymbol && f(symbol)
+
     /** The direct child trees of this tree
      *  EmptyTrees are always omitted. Lists are collapsed.
      */
@@ -181,14 +181,14 @@ trait Trees { self: Universe =>
     def mods: Modifiers
     def keyword: String = this match {
       case TypeDef(_, _, _, _)      => "type"
-      case ClassDef(mods, _, _, _)  => if (mods.isTrait) "trait" else "class"
+      case ClassDef(mods, _, _, _)  => if (mods hasFlag TRAIT) "trait" else "class"
       case DefDef(_, _, _, _, _, _) => "def"
       case ModuleDef(_, _, _)       => "object"
       case PackageDef(_, _)         => "package"
-      case ValDef(mods, _, _, _)    => if (mods.isVariable) "var" else "val"
+      case ValDef(mods, _, _, _)    => if (mods.isMutable) "var" else "val"
       case _ => ""
     }
-    final def hasFlag(mask: Long): Boolean = (mods.flags & mask) != 0L
+    // final def hasFlag(mask: Long): Boolean = mods hasFlag mask
   }
 
   /** Package clause
diff --git a/src/library/scala/reflect/generic/UnPickler.scala b/src/library/scala/reflect/generic/UnPickler.scala
index 80818ac62a..8a370e44fa 100755
--- a/src/library/scala/reflect/generic/UnPickler.scala
+++ b/src/library/scala/reflect/generic/UnPickler.scala
@@ -196,11 +196,8 @@ abstract class UnPickler {
             tag match {
               case EXTMODCLASSref =>
                 val moduleVar = owner.info.decl(nme.moduleVarName(name))
-                if (moduleVar.hasFlag(LAZY)) {
-                  val lazyAcc = moduleVar.lazyAccessor
-                  if (lazyAcc != NoSymbol)
-                    sym = lazyAcc.lazyAccessor
-                }
+                if (moduleVar.isLazyAccessor)
+                  sym = moduleVar.lazyAccessor.lazyAccessor
               case _ =>
             }
 
@@ -255,7 +252,7 @@ abstract class UnPickler {
           sym.flags = flags & PickledFlags
           sym.privateWithin = privateWithin
           if (readIndex != end) assert(sym hasFlag (SUPERACCESSOR | PARAMACCESSOR), sym)
-          if (sym hasFlag SUPERACCESSOR) assert(readIndex != end)
+          if (sym.isSuperAccessor) assert(readIndex != end)
           sym.info =
             if (readIndex != end) newLazyTypeRefAndAlias(inforef, readNat())
             else newLazyTypeRef(inforef)