From c7f824198352e0740a979a8022afc1a6e128dd1c Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Nov 2014 16:01:04 +0100 Subject: [PATCH 1/9] Rename productArity in pattern matcher to prodArity productArity is a method defined in ProductN which is inherited from case classes. It is possible that it will be usewd in the implementation of pattern matching in the future. The previous implementation used the same name to mean something else: Not the arity of the case class itself but the arity of the pattern/type it represented. Renaming to prodArity avoids the confusion. --- .../tools/dotc/transform/PatternMatcher.scala | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/dotty/tools/dotc/transform/PatternMatcher.scala b/src/dotty/tools/dotc/transform/PatternMatcher.scala index 7631c99c8613..9be8e5e19e63 100644 --- a/src/dotty/tools/dotc/transform/PatternMatcher.scala +++ b/src/dotty/tools/dotc/transform/PatternMatcher.scala @@ -1376,8 +1376,8 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans def subPatTypes: List[Type] = typedPatterns map (_.tpe) - // there are `productArity` non-seq elements in the tuple. - protected def firstIndexingBinder = productArity + // there are `prodArity` non-seq elements in the tuple. + protected def firstIndexingBinder = prodArity protected def expectedLength = elementArity protected def lastIndexingBinder = totalArity - starArity - 1 @@ -1535,7 +1535,7 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans // the trees that select the subpatterns on the extractor's result, referenced by `binder` // require (totalArity > 0 && (!lastIsStar || isSeq)) protected def subPatRefs(binder: Symbol, subpatBinders: List[Symbol], binderTypeTested: Type): List[Tree] = { - if(aligner.isSingle && aligner.extractor.productArity == 1 && defn.isTupleType(binder.info)) { + if(aligner.isSingle && aligner.extractor.prodArity == 1 && defn.isTupleType(binder.info)) { // special case for extractor // comparing with scalac additional assertions added val subpw = subpatBinders.head.info.widen @@ -1577,13 +1577,13 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans * * Here Pm/Fi is the last pattern to match the fixed arity section. * - * productArity: the value of i, i.e. the number of non-sequence types in the extractor + * prodArity: the value of i, i.e. the number of non-sequence types in the extractor * nonStarArity: the value of j, i.e. the number of non-star patterns in the case definition * elementArity: j - i, i.e. the number of non-star patterns which must match sequence elements * starArity: 1 or 0 based on whether there is a star (sequence-absorbing) pattern * totalArity: nonStarArity + starArity, i.e. the number of patterns in the case definition * - * Note that productArity is a function only of the extractor, and + * Note that prodArity is a function only of the extractor, and * nonStar/star/totalArity are all functions of the patterns. The key * value for aligning and typing the patterns is elementArity, as it * is derived from both sets of information. @@ -1651,7 +1651,7 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans final case class Extractor(whole: Type, fixed: List[Type], repeated: Repeated) { require(whole != NoType, s"expandTypes($whole, $fixed, $repeated)") - def productArity = fixed.length + def prodArity = fixed.length def hasSeq = repeated.exists def elementType = repeated.elementType def sequenceType = repeated.sequenceType @@ -1681,8 +1681,8 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans * < 0: There are more products than patterns: compile time error. */ final case class Aligned(patterns: Patterns, extractor: Extractor) { - def elementArity = patterns.nonStarArity - productArity - def productArity = extractor.productArity + def elementArity = patterns.nonStarArity - prodArity + def prodArity = extractor.prodArity def starArity = patterns.starArity def totalArity = patterns.totalArity @@ -1693,15 +1693,15 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans def typedNonStarPatterns = products ::: elements def typedPatterns = typedNonStarPatterns ::: stars - def isBool = !isSeq && productArity == 0 + def isBool = !isSeq && prodArity == 0 def isSingle = !isSeq && totalArity == 1 def isStar = patterns.hasStar def isSeq = extractor.hasSeq private def typedAsElement(pat: Pattern) = TypedPat(pat, extractor.elementType) private def typedAsSequence(pat: Pattern) = TypedPat(pat, extractor.sequenceType) - private def productPats = patterns.fixed take productArity - private def elementPats = patterns.fixed drop productArity + private def productPats = patterns.fixed take prodArity + private def elementPats = patterns.fixed drop prodArity private def products = (productPats, productTypes).zipped map TypedPat private def elements = elementPats map typedAsElement private def stars = patterns.starPatterns map typedAsSequence @@ -1710,7 +1710,7 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans |Aligned { | patterns $patterns | extractor $extractor - | arities $productArity/$elementArity/$starArity // product/element/star + | arities $prodArity/$elementArity/$starArity // product/element/star | typed ${typedPatterns mkString ", "} |}""".stripMargin.trim } @@ -1826,7 +1826,7 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans def offering = extractor.offeringString def symString = tree.symbol.showLocated def offerString = if (extractor.isErroneous) "" else s" offering $offering" - def arityExpected = ( if (extractor.hasSeq) "at least " else "" ) + productArity + def arityExpected = ( if (extractor.hasSeq) "at least " else "" ) + prodArity def err(msg: String) = ctx.error(msg, tree.pos) def warn(msg: String) = ctx.warning(msg, tree.pos) @@ -1871,12 +1871,12 @@ class PatternMatcher extends MiniPhaseTransform with DenotTransformer {thisTrans * process, we will tuple the extractor before creation Aligned so that * it contains known good values. */ - def productArity = extractor.productArity + def prodArity = extractor.prodArity def acceptMessage = if (extractor.isErroneous) "" else s" to hold ${extractor.offeringString}" - val requiresTupling = isUnapply && patterns.totalArity == 1 && productArity > 1 + val requiresTupling = isUnapply && patterns.totalArity == 1 && prodArity > 1 //if (requiresTupling && effectivePatternArity(args) == 1) - // currentUnit.deprecationWarning(sel.pos, s"${sel.symbol.owner} expects $productArity patterns$acceptMessage but crushing into $productArity-tuple to fit single pattern (SI-6675)") + // currentUnit.deprecationWarning(sel.pos, s"${sel.symbol.owner} expects $prodArity patterns$acceptMessage but crushing into $prodArity-tuple to fit single pattern (SI-6675)") val normalizedExtractor = if (requiresTupling) From 9dae49b8994f72f2b4b83665c53a63b49896c5de Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Nov 2014 16:05:18 +0100 Subject: [PATCH 2/9] Refine merge of nullary and parameterless denotations. Previously, two denotations with types => T and ()T could not be merged, only their types could be, but then the denotation would no longer be a SymDenotation. We now treat the two types as equivalent so that it will select the symbol in a subclass. This fixes a problem where, once overrding pairs are made to work (see following commit), in core/Types we get an error "overriding final method "hashCode". --- src/dotty/tools/dotc/core/Denotations.scala | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/core/Denotations.scala b/src/dotty/tools/dotc/core/Denotations.scala index ce11759ceb5c..5ed0ebfb29bc 100644 --- a/src/dotty/tools/dotc/core/Denotations.scala +++ b/src/dotty/tools/dotc/core/Denotations.scala @@ -240,8 +240,19 @@ object Denotations { val sym1 = denot1.symbol val sym2 = denot2.symbol val sym2Accessible = sym2.isAccessibleFrom(pre) + def resultType(tp: Type) = tp match { + case tp @ MethodType(Nil, _) => tp.resultType + case ExprType(rt) => rt + case _ => NoType + } + def isAsGood(tp1: Type, tp2: Type) = + tp1 <:< tp2 || { + val rtp1 = resultType(tp1) + val rtp2 = resultType(tp2) + rtp1 <:< rtp2 + } def prefer(info1: Type, sym1: Symbol, info2: Type, sym2: Symbol) = - info1 <:< info2 && + isAsGood(info1, info2) && (sym1.isAsConcrete(sym2) || !(info2 <:< info1)) if (sym2Accessible && prefer(info2, sym2, info1, sym1)) denot2 else { From 4d5a901d10a28c286f8754134f5030daae0d239b Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Nov 2014 16:11:31 +0100 Subject: [PATCH 3/9] Fix OverridingPairs OverridingPairs had several bugs which caused pairs to be lost, which caused missing overrides checks. Fixing OverridingPairs revealed several test failures (and a problem in Synthetics generation which was fixed in the last commit). Tests that became negative are all moved into neg/overrides.scala, and the original versions in pos were fixed. --- .../dotc/transform/OverridingPairs.scala | 30 +++++----- src/dotty/tools/dotc/typer/RefChecks.scala | 2 +- test/dotc/tests.scala | 2 + tests/neg/over.scala | 10 ++++ tests/neg/overrides.scala | 59 +++++++++++++++++++ tests/pos/overrides.scala | 2 +- tests/pos/synthetics.scala | 8 +++ tests/pos/t0599.scala | 4 +- tests/pos/t2809.scala | 2 +- 9 files changed, 99 insertions(+), 20 deletions(-) create mode 100644 tests/neg/over.scala create mode 100644 tests/neg/overrides.scala diff --git a/src/dotty/tools/dotc/transform/OverridingPairs.scala b/src/dotty/tools/dotc/transform/OverridingPairs.scala index d0bc90389d38..76d5d36137c5 100644 --- a/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -2,7 +2,7 @@ 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.immutable.BitSet @@ -18,6 +18,8 @@ import scala.annotation.tailrec */ object OverridingPairs { + private val ExcludedType = ExpandedName.toTypeFlags | TypeArgument + /** The cursor class * @param base the base class that contains the overriding pairs */ @@ -29,7 +31,10 @@ object OverridingPairs { * But it may be refined in subclasses. */ protected def exclude(sym: Symbol): Boolean = - sym.isConstructor || sym.is(PrivateLocal) + sym.isConstructor || + sym.is(Private) || + sym.is(Module) && sym.is(Synthetic) || + sym.is(ExcludedType) /** The parents of base (may also be refined). */ @@ -77,7 +82,7 @@ object OverridingPairs { } 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). @@ -107,21 +112,16 @@ object OverridingPairs { overriding = curEntry.sym if (nextEntry ne null) { val overridingOwner = overriding.owner + def qualifies(candidate: Symbol) = + candidate.canMatchInheritedSymbols && + overriding.owner != candidate.owner && + matches(overriding, candidate) && + !exclude(candidate) && + !exclude(overriding) 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)))) + } while ((nextEntry ne null) && !qualifies(nextEntry.sym)) 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 diff --git a/src/dotty/tools/dotc/typer/RefChecks.scala b/src/dotty/tools/dotc/typer/RefChecks.scala index f45966741b8d..2ae9a5b97af9 100644 --- a/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/src/dotty/tools/dotc/typer/RefChecks.scala @@ -246,7 +246,7 @@ 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") + overrideError(i"cannot override final member ${other.showLocated}") } 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 // the default getter: one default getter might sometimes override, sometimes not. Example in comment on ticket. diff --git a/test/dotc/tests.scala b/test/dotc/tests.scala index 966c231e4cac..fb2b747a6d27 100644 --- a/test/dotc/tests.scala +++ b/test/dotc/tests.scala @@ -72,6 +72,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 = 4) @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) diff --git a/tests/neg/over.scala b/tests/neg/over.scala new file mode 100644 index 000000000000..488d71614c3c --- /dev/null +++ b/tests/neg/over.scala @@ -0,0 +1,10 @@ +trait T { + def x = 1 +} + +class C extends T { + + val x = 2 + override val y = 2 + +} diff --git a/tests/neg/overrides.scala b/tests/neg/overrides.scala new file mode 100644 index 000000000000..4befe1623d8c --- /dev/null +++ b/tests/neg/overrides.scala @@ -0,0 +1,59 @@ + abstract class FooA { + type A <: Ax; + abstract class Ax; + abstract class InnerA { + type B <: A; + def doB : B; + } + } + trait FooB extends FooA { + type A <: Ax; + trait Ax extends super.Ax { def xxx : Int; } + abstract class InnerB extends InnerA { + // type B <: A; + val a : A = doB; + a.xxx; + doB.xxx; + } + } + +package p1 { + abstract class T1 { + protected def bug(p: Int = 1): Int // without 'protected' compiles fine + } +} +package p2 { // all being in the same package compiles fine + import p1._ + abstract class T2 extends T1 { + class A { + bug() + } + } + + abstract class T3 extends T2 { + class A { + bug() + } + } +} + +class A[T] { + + def f(x: T)(y: T = x) = y + +} + +class B extends A[Int] { + + def f(x: Int)(y: Int) = y + + f(2)() + +} + +class X { + def f: A[Int] = ??? +} +class Y extends X { + def f: A[Int] = ??? +} diff --git a/tests/pos/overrides.scala b/tests/pos/overrides.scala index 3d254ea70a28..97402f773082 100644 --- a/tests/pos/overrides.scala +++ b/tests/pos/overrides.scala @@ -6,7 +6,7 @@ class A[T] { class B extends A[Int] { - def f(x: Int)(y: Int) = y + override def f(x: Int)(y: Int) = y f(2)() diff --git a/tests/pos/synthetics.scala b/tests/pos/synthetics.scala index c7d49df70de9..c870cb1a4607 100644 --- a/tests/pos/synthetics.scala +++ b/tests/pos/synthetics.scala @@ -2,3 +2,11 @@ case class C(x: Int, var y: String) { } + +class Top { + + final override def hashCode: Int = 2 + +} + +case class Sub() extends Top diff --git a/tests/pos/t0599.scala b/tests/pos/t0599.scala index 885159af664f..6445fa9fb754 100644 --- a/tests/pos/t0599.scala +++ b/tests/pos/t0599.scala @@ -7,8 +7,8 @@ abstract class FooA { } } trait FooB extends FooA { - type A <: Ax; - trait Ax extends super.Ax { def xxx : Int; } + type A <: Axx; + trait Axx extends super.Ax { def xxx : Int; } abstract class InnerB extends InnerA { // type B <: A; val a : A = doB; diff --git a/tests/pos/t2809.scala b/tests/pos/t2809.scala index 1f68b0b07a90..1e9ec60d2e86 100644 --- a/tests/pos/t2809.scala +++ b/tests/pos/t2809.scala @@ -12,7 +12,7 @@ package p2 { // all being in the same package compiles fine } abstract class T3 extends T2 { - class A { + class A2 { bug() } } From 53e9fd65fd6cff6f46cbe0e18732cd8a0ebea001 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Nov 2014 18:36:12 +0100 Subject: [PATCH 4/9] Refactored OverridingPairs to make it easier to understand. --- .../dotc/transform/OverridingPairs.scala | 79 ++++++++++--------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/src/dotty/tools/dotc/transform/OverridingPairs.scala b/src/dotty/tools/dotc/transform/OverridingPairs.scala index 76d5d36137c5..561b5fe3118b 100644 --- a/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -4,7 +4,7 @@ package transform import core._ 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 @@ -33,7 +33,7 @@ object OverridingPairs { protected def exclude(sym: Symbol): Boolean = sym.isConstructor || sym.is(Private) || - sym.is(Module) && sym.is(Synthetic) || + sym.is(Module) && sym.is(Synthetic) || // TODO: move to refchecks sym.is(ExcludedType) /** The parents of base (may also be refined). @@ -75,7 +75,7 @@ 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 @@ -88,7 +88,7 @@ object OverridingPairs { * (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 */ @@ -104,43 +104,50 @@ 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) || exclude(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 - def qualifies(candidate: Symbol) = - candidate.canMatchInheritedSymbols && - overriding.owner != candidate.owner && - matches(overriding, candidate) && - !exclude(candidate) && - !exclude(overriding) - do { - do { - nextEntry = decls.lookupNextEntry(nextEntry); - } while ((nextEntry ne null) && !qualifies(nextEntry.sym)) - 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) && + !exclude(overridden)) { + visited += overridden + if (!hasCommonParentAsSubclass(overriding.owner, overridden.owner)) return } + } else { + curEntry = curEntry.prev + nextOverriding() } + next() } - } - next + nextOverriding() + next() } } From 387bdf9fa0bafd76b06d91d5e88f74e29143fc06 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Nov 2014 18:53:40 +0100 Subject: [PATCH 5/9] Fine-tuning override errors for syntehtic companion objects These may raise real errors, so we cannot exclude them from overriding pairs a priori. But we can avoid reporting any errors if other override errors were reported previously for the same class. --- .../dotc/transform/OverridingPairs.scala | 1 - src/dotty/tools/dotc/typer/RefChecks.scala | 25 ++++++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/dotty/tools/dotc/transform/OverridingPairs.scala b/src/dotty/tools/dotc/transform/OverridingPairs.scala index 561b5fe3118b..f076fbc947d9 100644 --- a/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -33,7 +33,6 @@ object OverridingPairs { protected def exclude(sym: Symbol): Boolean = sym.isConstructor || sym.is(Private) || - sym.is(Module) && sym.is(Synthetic) || // TODO: move to refchecks sym.is(ExcludedType) /** The parents of base (may also be refined). diff --git a/src/dotty/tools/dotc/typer/RefChecks.scala b/src/dotty/tools/dotc/typer/RefChecks.scala index 2ae9a5b97af9..785998549770 100644 --- a/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/src/dotty/tools/dotc/typer/RefChecks.scala @@ -107,6 +107,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 +136,15 @@ object RefChecks { def infoString0(sym: Symbol, showLocation: Boolean) = { val sym1 = sym.underlyingSymbol + def info = self.memberInfo(sym1) if (showLocation) sym1.showLocated else - sym1.show + - (if (sym1.isAliasType) ", which equals " + self.memberInfo(sym1) - else if (sym1.isAbstractType) " with bounds" + self.memberInfo(sym1) + i"$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) " of type " + self.memberInfo(sym1) - else "") + else if (sym1.isTerm) i" of type $info" + else ""}" } /* Check that all conditions for overriding `other` by `member` @@ -171,10 +173,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) From c1b0ab6f6ed36fed7cc4bfa710704ff197c12b31 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 15 Nov 2014 12:07:31 +0100 Subject: [PATCH 6/9] Fixes for type argument handling. (1) Type arguments now get a coordinate. (2) They are labeled Override (2) avoids having to special case TypeArgs in OverridingPairs. --- src/dotty/tools/dotc/core/SymDenotations.scala | 8 +++++++- src/dotty/tools/dotc/core/TypeOps.scala | 8 +++++--- src/dotty/tools/dotc/transform/OverridingPairs.scala | 5 +---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/dotty/tools/dotc/core/SymDenotations.scala b/src/dotty/tools/dotc/core/SymDenotations.scala index dfb58f68b20d..9801cb629dc7 100644 --- a/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/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 = diff --git a/src/dotty/tools/dotc/core/TypeOps.scala b/src/dotty/tools/dotc/core/TypeOps.scala index efd7fcca3cd1..7a853ae12c95 100644 --- a/src/dotty/tools/dotc/core/TypeOps.scala +++ b/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) } diff --git a/src/dotty/tools/dotc/transform/OverridingPairs.scala b/src/dotty/tools/dotc/transform/OverridingPairs.scala index f076fbc947d9..813082f2e920 100644 --- a/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -30,10 +30,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(Private) || - sym.is(ExcludedType) + protected def exclude(sym: Symbol): Boolean = !sym.memberCanMatchInheritedSymbols /** The parents of base (may also be refined). */ From 222e9a478f7b851582550973df6a9d141766e49a Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 15 Nov 2014 12:08:28 +0100 Subject: [PATCH 7/9] Some fixes to override checking in RefChecks. `override` was not recognized at all on types. --- src/dotty/tools/dotc/transform/SymUtils.scala | 3 +++ src/dotty/tools/dotc/typer/RefChecks.scala | 20 +++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/dotty/tools/dotc/transform/SymUtils.scala b/src/dotty/tools/dotc/transform/SymUtils.scala index 449affb9e3d3..7d485f64cc9e 100644 --- a/src/dotty/tools/dotc/transform/SymUtils.scala +++ b/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 diff --git a/src/dotty/tools/dotc/typer/RefChecks.scala b/src/dotty/tools/dotc/typer/RefChecks.scala index 785998549770..29ac33498c90 100644 --- a/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/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 @@ -254,8 +251,10 @@ object RefChecks { overrideError("cannot be used here - classes can only override abstract types") } else if (other.isEffectivelyFinal) { // (1.2) overrideError(i"cannot override final member ${other.showLocated}") - } 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 + } 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( @@ -266,13 +265,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)) { @@ -550,7 +549,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)) @@ -563,7 +562,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) } } From b60f085e543e71577e0132b938facd0b6d544e81 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 17 Nov 2014 17:47:03 +0100 Subject: [PATCH 8/9] Fixes of reviewers comments. --- src/dotty/tools/dotc/transform/OverridingPairs.scala | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/dotty/tools/dotc/transform/OverridingPairs.scala b/src/dotty/tools/dotc/transform/OverridingPairs.scala index 813082f2e920..bc3c085a92a6 100644 --- a/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -18,8 +18,6 @@ import scala.annotation.tailrec */ object OverridingPairs { - private val ExcludedType = ExpandedName.toTypeFlags | TypeArgument - /** The cursor class * @param base the base class that contains the overriding pairs */ @@ -111,7 +109,7 @@ object OverridingPairs { @tailrec def loop(): Unit = if (curEntry ne null) { overriding = curEntry.sym - if (visited.contains(overriding) || exclude(overriding)) { + if (visited.contains(overriding)) { curEntry = curEntry.prev loop() } @@ -130,9 +128,7 @@ object OverridingPairs { nextEntry = decls.lookupNextEntry(nextEntry) if (nextEntry ne null) { overridden = nextEntry.sym - if (overriding.owner != overridden.owner && - matches(overriding, overridden) && - !exclude(overridden)) { + if (overriding.owner != overridden.owner && matches(overriding, overridden)) { visited += overridden if (!hasCommonParentAsSubclass(overriding.owner, overridden.owner)) return } From ea06d6618f63339fec0af8ca6835a3f34a100d0f Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 18 Nov 2014 15:18:19 +0100 Subject: [PATCH 9/9] Check that overriding members refine the types of overridden ones. Somehow this was lost in porting (or was this done somewhere else in scalac?). --- src/dotty/tools/dotc/core/Denotations.scala | 14 +------------- src/dotty/tools/dotc/core/Types.scala | 16 ++++++++++++++++ src/dotty/tools/dotc/typer/RefChecks.scala | 19 +++++++++++-------- test/dotc/tests.scala | 2 +- tests/neg/overrides.scala | 11 +++++++++++ 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/dotty/tools/dotc/core/Denotations.scala b/src/dotty/tools/dotc/core/Denotations.scala index 5ed0ebfb29bc..236bdb7f42f7 100644 --- a/src/dotty/tools/dotc/core/Denotations.scala +++ b/src/dotty/tools/dotc/core/Denotations.scala @@ -240,20 +240,8 @@ object Denotations { val sym1 = denot1.symbol val sym2 = denot2.symbol val sym2Accessible = sym2.isAccessibleFrom(pre) - def resultType(tp: Type) = tp match { - case tp @ MethodType(Nil, _) => tp.resultType - case ExprType(rt) => rt - case _ => NoType - } - def isAsGood(tp1: Type, tp2: Type) = - tp1 <:< tp2 || { - val rtp1 = resultType(tp1) - val rtp2 = resultType(tp2) - rtp1 <:< rtp2 - } def prefer(info1: Type, sym1: Symbol, info2: Type, sym2: Symbol) = - isAsGood(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) diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala index 5228c077eb65..a09c0cd7124b 100644 --- a/src/dotty/tools/dotc/core/Types.scala +++ b/src/dotty/tools/dotc/core/Types.scala @@ -533,6 +533,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: diff --git a/src/dotty/tools/dotc/typer/RefChecks.scala b/src/dotty/tools/dotc/typer/RefChecks.scala index 29ac33498c90..6995076c5cbe 100644 --- a/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/src/dotty/tools/dotc/typer/RefChecks.scala @@ -134,14 +134,13 @@ object RefChecks { def infoString0(sym: Symbol, showLocation: Boolean) = { val sym1 = sym.underlyingSymbol def info = self.memberInfo(sym1) - if (showLocation) sym1.showLocated - else - i"$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 ""}" + 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` @@ -287,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() } diff --git a/test/dotc/tests.scala b/test/dotc/tests.scala index fb2b747a6d27..8a8c162c6c22 100644 --- a/test/dotc/tests.scala +++ b/test/dotc/tests.scala @@ -73,7 +73,7 @@ class tests extends CompilerTest { @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 = 4) + @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) diff --git a/tests/neg/overrides.scala b/tests/neg/overrides.scala index 4befe1623d8c..d502af8ddb7d 100644 --- a/tests/neg/overrides.scala +++ b/tests/neg/overrides.scala @@ -57,3 +57,14 @@ class X { class Y extends X { def f: A[Int] = ??? } + + +class A1 +class B1 + +class X1 { + def f(): A1 = ??? +} +class Y1 extends X1 { + override def f(): B1 = ??? +}