Merge pull request #228 from dotty-staging/fix/overriding

Fix/overriding

Author: odersky

src/dotty/tools/dotc/core/Denotations.scala

@@ -241,8 +241,7 @@ object Denotations {
             val sym2 = denot2.symbol
             val sym2Accessible = sym2.isAccessibleFrom(pre)
             def prefer(info1: Type, sym1: Symbol, info2: Type, sym2: Symbol) =
-              info1 <:< info2 &&
-              (sym1.isAsConcrete(sym2) || !(info2 <:< info1))
+              info1.overrides(info2) && (sym1.isAsConcrete(sym2) || !info2.overrides(info1))
             if (sym2Accessible && prefer(info2, sym2, info1, sym1)) denot2
             else {
               val sym1Accessible = sym1.isAccessibleFrom(pre)

src/dotty/tools/dotc/core/SymDenotations.scala

@@ -805,7 +805,13 @@ object SymDenotations {
      *  either as overrider or overridee.
      */
     final def canMatchInheritedSymbols(implicit ctx: Context): Boolean =
-      maybeOwner.isClass && !isConstructor && !is(Private)
+      maybeOwner.isClass && memberCanMatchInheritedSymbols
+
+    /** If false, this class member cannot possibly participate in an override,
+     *  either as overrider or overridee.
+     */
+    final def memberCanMatchInheritedSymbols(implicit ctx: Context): Boolean =
+      !isConstructor && !is(Private)
 
     /** The symbol, in class `inClass`, that is overridden by this denotation. */
     final def overriddenSymbol(inClass: ClassSymbol)(implicit ctx: Context): Symbol =

src/dotty/tools/dotc/core/TypeOps.scala

@@ -197,8 +197,6 @@ trait TypeOps { this: Context =>
     }
   }
 
-
-
   private def enterArgBinding(formal: Symbol, info: Type, cls: ClassSymbol, decls: Scope) = {
     val lazyInfo = new LazyType { // needed so we do not force `formal`.
       def complete(denot: SymDenotation)(implicit ctx: Context): Unit = {
@@ -207,7 +205,11 @@ trait TypeOps { this: Context =>
       }
     }
     val typeArgFlag = if (formal is Local) TypeArgument else EmptyFlags
-    val sym = ctx.newSymbol(cls, formal.name, formal.flagsUNSAFE & RetainedTypeArgFlags | typeArgFlag, lazyInfo)
+    val sym = ctx.newSymbol(
+      cls, formal.name,
+      formal.flagsUNSAFE & RetainedTypeArgFlags | typeArgFlag | Override,
+      lazyInfo,
+      coord = cls.coord)
     cls.enter(sym, decls)
   }
 

src/dotty/tools/dotc/core/Types.scala

@@ -542,6 +542,22 @@ object Types {
       ctx.typeComparer.isSameType(this, that)
     }
 
+    /** Is this type a legal type for a member that overrides another
+     *  member of type `that`? This is the same as `<:<`, except that
+     *  the types ()T and => T are identified, and T is seen as overriding
+     *  either type.
+     */
+    final def overrides(that: Type)(implicit ctx: Context) = {
+      def result(tp: Type): Type = tp match {
+        case ExprType(_) | MethodType(Nil, _) => tp.resultType
+        case _ => tp
+      }
+      this <:< that || {
+        val rthat = result(that)
+        (rthat ne that) && result(this) <:< rthat
+      }
+    }
+
     /** Is this type close enough to that type so that members
      *  with the two type would override each other?d
      *  This means:

src/dotty/tools/dotc/transform/OverridingPairs.scala

@@ -2,9 +2,9 @@ package dotty.tools.dotc
 package transform
 
 import core._
-import Flags._, Symbols._, Contexts._, Types._, Scopes._
+import Flags._, Symbols._, Contexts._, Types._, Scopes._, Decorators._
 import util.HashSet
-import collection.mutable.HashMap
+import collection.mutable
 import collection.immutable.BitSet
 import scala.annotation.tailrec
 
@@ -28,8 +28,7 @@ object OverridingPairs {
     /** Symbols to exclude: Here these are constructors and private locals.
      *  But it may be refined in subclasses.
      */
-    protected def exclude(sym: Symbol): Boolean =
-      sym.isConstructor || sym.is(PrivateLocal)
+    protected def exclude(sym: Symbol): Boolean = !sym.memberCanMatchInheritedSymbols
 
     /** The parents of base (may also be refined).
      */
@@ -70,20 +69,20 @@ object OverridingPairs {
     }
 
     private val subParents = {
-      val subParents = new HashMap[Symbol, BitSet]
+      val subParents = new mutable.HashMap[Symbol, BitSet]
       for (bc <- base.info.baseClasses)
         subParents(bc) = BitSet(parents.indices.filter(parents(_).derivesFrom(bc)): _*)
       subParents
     }
 
     private def hasCommonParentAsSubclass(cls1: Symbol, cls2: Symbol): Boolean =
-      (subParents(cls1) intersect subParents(cls2)).isEmpty
+      (subParents(cls1) intersect subParents(cls2)).nonEmpty
 
     /** The scope entries that have already been visited as overridden
      *  (maybe excluded because of hasCommonParentAsSubclass).
      *  These will not appear as overriding
      */
-    private val visited = new HashSet[ScopeEntry](64)
+    private val visited = new mutable.HashSet[Symbol]
 
     /** The current entry candidate for overriding
      */
@@ -99,48 +98,48 @@ object OverridingPairs {
     var overridden: Symbol = _
 
     //@M: note that next is called once during object initialization
-    def hasNext: Boolean = curEntry ne null
+    final def hasNext: Boolean = nextEntry ne null
 
-    @tailrec
-    final def next(): Unit = {
-      if (curEntry ne null) {
-        overriding = curEntry.sym
+    /**  @post
+     *     curEntry   = the next candidate that may override something else
+     *     nextEntry  = curEntry
+     *     overriding = curEntry.sym
+     */
+    private def nextOverriding(): Unit = {
+      @tailrec def loop(): Unit =
+        if (curEntry ne null) {
+          overriding = curEntry.sym
+          if (visited.contains(overriding)) {
+            curEntry = curEntry.prev
+            loop()
+          }
+        }
+      loop()
+      nextEntry = curEntry
+    }
+
+    /** @post
+     *    hasNext    = there is another overriding pair
+     *    overriding = overriding member of the pair, provided hasNext is true
+     *    overridden = overridden member of the pair, provided hasNext is true
+     */
+    @tailrec final def next(): Unit =
+      if (nextEntry ne null) {
+        nextEntry = decls.lookupNextEntry(nextEntry)
         if (nextEntry ne null) {
-          val overridingOwner = overriding.owner
-          do {
-            do {
-              nextEntry = decls.lookupNextEntry(nextEntry);
-              /* DEBUG
-              if ((nextEntry ne null) &&
-                  !(nextEntry.sym hasFlag PRIVATE) &&
-                  !(overriding.owner == nextEntry.sym.owner) &&
-                  !matches(overriding, nextEntry.sym))
-                println("skipping "+overriding+":"+self.memberType(overriding)+overriding.locationString+" to "+nextEntry.sym+":"+self.memberType(nextEntry.sym)+nextEntry.sym.locationString)
-              */
-              } while ((nextEntry ne null) &&
-                       (//!!!!nextEntry.sym.canMatchInheritedSymbols ||
-                        (overriding.owner == nextEntry.sym.owner) ||
-                        (!matches(overriding, nextEntry.sym)) ||
-                        (exclude(overriding))))
-            if (nextEntry ne null) visited.addEntry(nextEntry)
-            // skip nextEntry if a class in `parents` is a subclass of the owners of both
-            // overriding and nextEntry.sym
-          } while ((nextEntry ne null) &&
-                   hasCommonParentAsSubclass(overridingOwner, nextEntry.sym.owner))
-          if (nextEntry ne null) {
-            overridden = nextEntry.sym;
-            //Console.println("yield: " + overriding + overriding.locationString + " / " + overridden + overridden.locationString);//DEBUG
-          } else {
-            do {
-              curEntry = curEntry.prev
-            } while ((curEntry ne null) && visited.contains(curEntry))
-            nextEntry = curEntry
-            next
+          overridden = nextEntry.sym
+          if (overriding.owner != overridden.owner && matches(overriding, overridden)) {
+            visited += overridden
+            if (!hasCommonParentAsSubclass(overriding.owner, overridden.owner)) return
           }
+        } else {
+          curEntry = curEntry.prev
+          nextOverriding()
         }
+        next()
       }
-    }
 
-    next
+    nextOverriding()
+    next()
   }
 }

src/dotty/tools/dotc/transform/SymUtils.scala

@@ -29,6 +29,9 @@ class SymUtils(val self: Symbol) extends AnyVal {
 
   def isVolatile(implicit ctx: Context) = self.hasAnnotation(defn.VolatileAnnot)
 
+  def isAnyOverride(implicit ctx: Context) = self.is(Override) || self.is(AbsOverride)
+    // careful: AbsOverride is a term only flag. combining with Override would catch only terms.
+
   /** If this is a constructor, its owner: otherwise this. */
   final def skipConstructor(implicit ctx: Context): Symbol =
     if (self.isConstructor) self.owner else self

src/dotty/tools/dotc/typer/RefChecks.scala

@@ -5,7 +5,7 @@ import transform._
 import core._
 import config._
 import Symbols._, SymDenotations._, Types._, Contexts._, Decorators._, Flags._, Names._, NameOps._
-import StdNames._, Denotations._, Scopes._, Constants.Constant
+import StdNames._, Denotations._, Scopes._, Constants.Constant, SymUtils._
 import Annotations._
 import util.Positions._
 import scala.collection.{ mutable, immutable }
@@ -29,9 +29,6 @@ object RefChecks {
     def apply(pre: Type, name: Name)(implicit ctx: Context): Boolean = isDefaultGetter(name)
   }
 
-  private val AnyOverride = Override | AbsOverride
-  private val AnyOverrideOrSynthetic = AnyOverride | Synthetic
-
   /** Only one overloaded alternative is allowed to define default arguments */
   private def checkOverloadedRestrictions(clazz: Symbol)(implicit ctx: Context): Unit = {
     // Using the default getters (such as methodName$default$1) as a cheap way of
@@ -107,6 +104,7 @@ object RefChecks {
    */
   private def checkAllOverrides(clazz: Symbol)(implicit ctx: Context): Unit = {
     val self = clazz.thisType
+    var hasErrors = false
 
     case class MixinOverrideError(member: Symbol, msg: String)
 
@@ -135,14 +133,14 @@ object RefChecks {
 
     def infoString0(sym: Symbol, showLocation: Boolean) = {
       val sym1 = sym.underlyingSymbol
-      if (showLocation) sym1.showLocated
-      else
-        sym1.show +
-          (if (sym1.isAliasType) ", which equals " + self.memberInfo(sym1)
-          else if (sym1.isAbstractType) " with bounds" + self.memberInfo(sym1)
-          else if (sym1.is(Module)) ""
-          else if (sym1.isTerm) " of type " + self.memberInfo(sym1)
-          else "")
+      def info = self.memberInfo(sym1)
+      i"${if (showLocation) sym1.showLocated else sym1}${
+        if (sym1.isAliasType) i", which equals $info"
+        else if (sym1.isAbstractType) i" with bounds $info"
+        else if (sym1.is(Module)) ""
+        else if (sym1.isTerm) i" of type $info"
+        else ""
+      }"
     }
 
     /* Check that all conditions for overriding `other` by `member`
@@ -171,10 +169,15 @@ object RefChecks {
         "overriding %s;\n %s %s%s".format(
           infoStringWithLocation(other), infoString(member), msg, addendum)
       }
-      def emitOverrideError(fullmsg: String) = {
-        if (member.owner == clazz) ctx.error(fullmsg, member.pos)
-        else mixinOverrideErrors += new MixinOverrideError(member, fullmsg)
-      }
+
+      def emitOverrideError(fullmsg: String) =
+        if (!(hasErrors && member.is(Synthetic) && member.is(Module))) {
+          // suppress errors relating toi synthetic companion objects if other override
+          // errors (e.g. relating to the companion class) have already been reported.
+          if (member.owner == clazz) ctx.error(fullmsg, member.pos)
+          else mixinOverrideErrors += new MixinOverrideError(member, fullmsg)
+          hasErrors = true
+        }
 
       def overrideError(msg: String) = {
         if (noErrorType)
@@ -246,9 +249,11 @@ object RefChecks {
       } else if (!other.is(Deferred) && member.isClass) {
         overrideError("cannot be used here - classes can only override abstract types")
       } else if (other.isEffectivelyFinal) { // (1.2)
-        overrideError("cannot override final member")
-      } else if (!other.is(Deferred) && !isDefaultGetter(other.name) && !member.is(AnyOverrideOrSynthetic)) {
-        // (*) Synthetic exclusion for (at least) default getters, fixes SI-5178. We cannot assign the OVERRIDE flag to
+        overrideError(i"cannot override final member ${other.showLocated}")
+      } else if (!other.is(Deferred) &&
+                 !isDefaultGetter(other.name) &&
+                 !member.isAnyOverride) {
+        // (*) Exclusion for default getters, fixes SI-5178. We cannot assign the Override flag to
         // the default getter: one default getter might sometimes override, sometimes not. Example in comment on ticket.
         if (member.owner != clazz && other.owner != clazz && !(other.owner derivesFrom member.owner))
           emitOverrideError(
@@ -259,13 +264,13 @@ object RefChecks {
           overrideError("needs `override' modifier")
       } else if (other.is(AbsOverride) && other.isIncompleteIn(clazz) && !member.is(AbsOverride)) {
         overrideError("needs `abstract override' modifiers")
-      } else if (member.is(AnyOverride) && other.is(Accessor) &&
+      } else if (member.is(Override) && other.is(Accessor) &&
         other.accessedFieldOrGetter.is(Mutable, butNot = Lazy)) {
         // !?! this is not covered by the spec. We need to resolve this either by changing the spec or removing the test here.
         // !!! is there a !?! convention? I'm !!!ing this to make sure it turns up on my searches.
         if (!ctx.settings.overrideVars.value)
           overrideError("cannot override a mutable variable")
-      } else if (member.is(AnyOverride) &&
+      } else if (member.isAnyOverride &&
         !(member.owner.thisType.baseClasses exists (_ isSubClass other.owner)) &&
         !member.is(Deferred) && !other.is(Deferred) &&
         intersectionIsEmpty(member.extendedOverriddenSymbols, other.extendedOverriddenSymbols)) {
@@ -281,6 +286,10 @@ object RefChecks {
         overrideError("cannot be used here - term macros cannot override abstract methods")
       } else if (other.is(Macro) && !member.is(Macro)) { // (1.10)
         overrideError("cannot be used here - only term macros can override term macros")
+      } else if (member.isTerm && !isDefaultGetter(member.name) && !(memberTp overrides otherTp)) {
+        // types don't need to have their bounds in an overriding relationship
+        // since we automatically form their intersection when selecting.
+        overrideError("has incompatible type" + err.whyNoMatchStr(memberTp, otherTp))
       } else {
         checkOverrideDeprecated()
       }
@@ -543,7 +552,7 @@ object RefChecks {
 
     // 4. Check that every defined member with an `override` modifier overrides some other member.
     for (member <- clazz.info.decls)
-      if (member.is(AnyOverride) && !(clazz.thisType.baseClasses exists (hasMatchingSym(_, member)))) {
+      if (member.isAnyOverride && !(clazz.thisType.baseClasses exists (hasMatchingSym(_, member)))) {
         // for (bc <- clazz.info.baseClasses.tail) Console.println("" + bc + " has " + bc.info.decl(member.name) + ":" + bc.info.decl(member.name).tpe);//DEBUG
 
         val nonMatching = clazz.info.member(member.name).altsWith(alt => alt.owner != clazz && !alt.is(Final))
@@ -556,7 +565,8 @@ object RefChecks {
             val superSigs = ms.map(_.showDcl).mkString("\n")
             issueError(s".\nNote: the super classes of ${member.owner} contain the following, non final members named ${member.name}:\n${superSigs}")
         }
-        member.resetFlag(AnyOverride)
+        member.resetFlag(Override)
+        member.resetFlag(AbsOverride)
       }
   }
 

test/dotc/tests.scala

@@ -74,6 +74,8 @@ class tests extends CompilerTest {
   @Test def neg_autoTupling = compileFile(posDir, "autoTuplingTest", "-language:noAutoTupling" :: Nil, xerrors = 4)
   @Test def neg_autoTupling2 = compileFile(negDir, "autoTuplingTest", xerrors = 4)
   @Test def neg_companions = compileFile(negDir, "companions", xerrors = 1)
+  @Test def neg_over = compileFile(negDir, "over", xerrors = 1)
+  @Test def neg_overrides = compileFile(negDir, "overrides", xerrors = 5)
   @Test def neg_projections = compileFile(negDir, "projections", xerrors = 1)
   @Test def neg_i39 = compileFile(negDir, "i39", xerrors = 1)
   @Test def neg_i50_volatile = compileFile(negDir, "i50-volatile", xerrors = 4)

tests/neg/over.scala

@@ -0,0 +1,10 @@
+trait T {
+  def x = 1
+}
+
+class C extends T {
+
+  val x = 2
+  override val y = 2
+
+}