From e872aeec6d725c38a0f8f6433d165c2cba7f5dce Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 19 Feb 2018 17:25:31 +0100 Subject: [PATCH 1/9] Fix #3989: Check that members of concrete classes are type-correct This fixes the second half of #3989. Some tests had to be updated or re-classified because they were unsound before. --- .../dotty/tools/dotc/typer/RefChecks.scala | 49 +++++++++++++++++++ tests/neg/i1240b.scala | 2 +- tests/neg/i3989.scala | 7 +++ tests/{pos => neg}/t4731.scala | 6 ++- .../CollectionStrawMan6.scala | 4 +- .../run/colltest6/CollectionStrawMan6_1.scala | 4 +- 6 files changed, 65 insertions(+), 7 deletions(-) create mode 100644 tests/neg/i3989.scala rename tests/{pos => neg}/t4731.scala (51%) diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index 2a0bafe31a2d..aeb3bd81f65c 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -594,12 +594,61 @@ object RefChecks { checkNoAbstractDecls(bc.asClass.superClass) } + // Check that every term member of this concrete class has a symbol that matches the member's type + // Member types are computed by intersecting the types of all members that have the same name + // and signature. But a member selection will pick one particular implementation, according to + // the rules of overriding and linearization. This method checks that the implementation has indeed + // a type that subsumes the full member type. + def checkMemberTypesOK() = { + + // First compute all member names we need to check in `membersToCheck`. + // We do not check + // - types + // - synthetic members or bridges + // - members in other concrete classes, since these have been checked before + // (this is done for efficiency) + // - members in a prefix of inherited parents that all come from Java or Scala2 + // (this is done to avoid false negatives since Scala2's rules for checking are different) + val membersToCheck = mutable.Set[Name]() + val seenClasses = mutable.Set[Symbol]() + def addDecls(cls: Symbol): Unit = + if (!seenClasses.contains(cls)) { + seenClasses.+=(cls) + for (mbr <- cls.info.decls) + if (mbr.isTerm && !mbr.is(Synthetic | Bridge) && mbr.memberCanMatchInheritedSymbols && + !membersToCheck.contains(mbr.name)) + membersToCheck.+=(mbr.name) + cls.info.parents.map(_.classSymbol) + .filter(_.is(AbstractOrTrait)) + .dropWhile(_.is(JavaDefined | Scala2x)) + .foreach(addDecls) + } + addDecls(clazz) + + // For each member, check that the type of its symbol, as seen from `self` + // can override the info of this member + for (name <- membersToCheck) { + for (mbrd <- self.member(name).alternatives) { + val mbr = mbrd.symbol + val mbrType = mbr.info.asSeenFrom(self, mbr.owner) + if (!mbrType.overrides(mbrd.info, matchLoosely = true)) + ctx.errorOrMigrationWarning( + em"""${mbr.showLocated} is not a legal implementation of `$name' in $clazz + | its type $mbrType + | does not conform to ${mbrd.info}""", + (if (mbr.owner == clazz) mbr else clazz).pos) + } + } + } + checkNoAbstractMembers() if (abstractErrors.isEmpty) checkNoAbstractDecls(clazz) if (abstractErrors.nonEmpty) ctx.error(abstractErrorMessage, clazz.pos) + + checkMemberTypesOK() } else if (clazz.is(Trait) && !(clazz derivesFrom defn.AnyValClass)) { // For non-AnyVal classes, prevent abstract methods in interfaces that override // final members in Object; see #4431 diff --git a/tests/neg/i1240b.scala b/tests/neg/i1240b.scala index 2d23db61470b..8f2ec81d4702 100644 --- a/tests/neg/i1240b.scala +++ b/tests/neg/i1240b.scala @@ -9,4 +9,4 @@ abstract class A[X] extends T[X] { trait U[X] extends T[X] { abstract override def foo(x: X): X = super.foo(x) } -object Test extends A[String] with U[String] // error: accidental override +object Test extends A[String] with U[String] // error: accidental override // error: merge error diff --git a/tests/neg/i3989.scala b/tests/neg/i3989.scala new file mode 100644 index 000000000000..6cdcad568561 --- /dev/null +++ b/tests/neg/i3989.scala @@ -0,0 +1,7 @@ +object Test extends App { + trait A[+X] { def get: X } + case class B[X](x: X) extends A[X] { def get: X = x } + class C[X](x: Any) extends B[Any](x) with A[X] // error: not a legal implementation of `get' + def g(a: A[Int]): Int = a.get + g(new C[Int]("foo")) +} \ No newline at end of file diff --git a/tests/pos/t4731.scala b/tests/neg/t4731.scala similarity index 51% rename from tests/pos/t4731.scala rename to tests/neg/t4731.scala index d457543c1f4c..99bea6a8fde4 100644 --- a/tests/pos/t4731.scala +++ b/tests/neg/t4731.scala @@ -1,14 +1,16 @@ import java.util.Comparator -trait Trait1[T] { def foo(arg: Comparator[T]): Unit } +trait Trait1[T] { def foo(arg: Comparator[T]): String } trait Trait2[T] extends Trait1[T] { def foo(arg: Comparator[String]): Int = 0 } -class Class1 extends Trait2[String] { } +class Class1 extends Trait2[String] { } // error: not a legal implementation of `foo' object Test { def main(args: Array[String]): Unit = { val c = new Class1 c.foo(Ordering[String]) + val t: Trait1[String] = c + val x: String = t.foo(Ordering[String]) } } diff --git a/tests/pos-special/strawman-collections/CollectionStrawMan6.scala b/tests/pos-special/strawman-collections/CollectionStrawMan6.scala index c812a85f442a..14b2bbae0d1b 100644 --- a/tests/pos-special/strawman-collections/CollectionStrawMan6.scala +++ b/tests/pos-special/strawman-collections/CollectionStrawMan6.scala @@ -415,7 +415,7 @@ object CollectionStrawMan6 extends LowPriority { /** Concrete collection type: List */ sealed trait List[+A] extends LinearSeq[A] - with SeqLike[A, List] + with LinearSeqLike[A, List] with Buildable[A, List[A]] { def fromIterable[B](c: Iterable[B]): List[B] = List.fromIterable(c) @@ -604,7 +604,7 @@ object CollectionStrawMan6 extends LowPriority { } class LazyList[+A](expr: => LazyList.Evaluated[A]) - extends LinearSeq[A] with SeqLike[A, LazyList] { + extends LinearSeq[A] with LinearSeqLike[A, LazyList] { private[this] var evaluated = false private[this] var result: LazyList.Evaluated[A] = _ diff --git a/tests/run/colltest6/CollectionStrawMan6_1.scala b/tests/run/colltest6/CollectionStrawMan6_1.scala index 8f5c38104dae..17c28d74ef0f 100644 --- a/tests/run/colltest6/CollectionStrawMan6_1.scala +++ b/tests/run/colltest6/CollectionStrawMan6_1.scala @@ -416,7 +416,7 @@ object CollectionStrawMan6 extends LowPriority { /** Concrete collection type: List */ sealed trait List[+A] extends LinearSeq[A] - with SeqLike[A, List] + with LinearSeqLike[A, List] with Buildable[A, List[A]] { def fromIterable[B](c: Iterable[B]): List[B] = List.fromIterable(c) @@ -605,7 +605,7 @@ object CollectionStrawMan6 extends LowPriority { } class LazyList[+A](expr: => LazyList.Evaluated[A]) - extends LinearSeq[A] with SeqLike[A, LazyList] { + extends LinearSeq[A] with LinearSeqLike[A, LazyList] { private[this] var evaluated = false private[this] var result: LazyList.Evaluated[A] = _ From faae4da6385f5014ce68e5ca7c1e02029c4863da Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 19 Feb 2018 18:00:38 +0100 Subject: [PATCH 2/9] Use util.HashSet for member checking It's more efficient. Let's see whether that makes a difference. --- compiler/src/dotty/tools/dotc/typer/RefChecks.scala | 10 +++++----- compiler/src/dotty/tools/dotc/util/HashSet.scala | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index aeb3bd81f65c..bf4f7ce75d79 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -608,16 +608,16 @@ object RefChecks { // - members in other concrete classes, since these have been checked before // (this is done for efficiency) // - members in a prefix of inherited parents that all come from Java or Scala2 - // (this is done to avoid false negatives since Scala2's rules for checking are different) - val membersToCheck = mutable.Set[Name]() - val seenClasses = mutable.Set[Symbol]() + // (this is done to avoid false positives since Scala2's rules for checking are different) + val membersToCheck = new util.HashSet[Name](4096) + val seenClasses = new util.HashSet[Symbol](256) def addDecls(cls: Symbol): Unit = if (!seenClasses.contains(cls)) { - seenClasses.+=(cls) + seenClasses.addEntry(cls) for (mbr <- cls.info.decls) if (mbr.isTerm && !mbr.is(Synthetic | Bridge) && mbr.memberCanMatchInheritedSymbols && !membersToCheck.contains(mbr.name)) - membersToCheck.+=(mbr.name) + membersToCheck.addEntry(mbr.name) cls.info.parents.map(_.classSymbol) .filter(_.is(AbstractOrTrait)) .dropWhile(_.is(JavaDefined | Scala2x)) diff --git a/compiler/src/dotty/tools/dotc/util/HashSet.scala b/compiler/src/dotty/tools/dotc/util/HashSet.scala index 9ffdeac132de..3dca9efadc4e 100644 --- a/compiler/src/dotty/tools/dotc/util/HashSet.scala +++ b/compiler/src/dotty/tools/dotc/util/HashSet.scala @@ -7,6 +7,7 @@ class HashSet[T >: Null <: AnyRef](powerOfTwoInitialCapacity: Int, loadFactor: F private[this] var limit: Int = _ private[this] var table: Array[AnyRef] = _ + assert(Integer.bitCount(powerOfTwoInitialCapacity) == 1) protected def isEqual(x: T, y: T): Boolean = x.equals(y) // Counters for Stats From 08cef8ca20cab461b81c78f17b64417588b8fb6b Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 20 Feb 2018 13:09:20 +0100 Subject: [PATCH 3/9] Disable strawman tests Plugging the soundness hole in #3989 unveiled problems in the strawman. These need to be fixed before we can test it again in dotty. --- compiler/test/dotty/tools/dotc/IdempotencyTests.scala | 6 ++++-- compiler/test/dotty/tools/dotc/LinkTests.scala | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/compiler/test/dotty/tools/dotc/IdempotencyTests.scala b/compiler/test/dotty/tools/dotc/IdempotencyTests.scala index 2e7d8e8c58b9..8c969d6ff011 100644 --- a/compiler/test/dotty/tools/dotc/IdempotencyTests.scala +++ b/compiler/test/dotty/tools/dotc/IdempotencyTests.scala @@ -64,12 +64,14 @@ class IdempotencyTests extends ParallelTesting { } val allChecks = { check("CheckOrderIdempotency") + - check("CheckStrawmanIdempotency") + + // Disabled until strawman is fixed + // check("CheckStrawmanIdempotency") + check("CheckPosIdempotency") } val allTests = { - strawmanIdempotency + + // Disabled until strawman is fixed + // strawmanIdempotency + orderIdempotency + posIdempotency } diff --git a/compiler/test/dotty/tools/dotc/LinkTests.scala b/compiler/test/dotty/tools/dotc/LinkTests.scala index 6c2ed2628ea3..29a81aed3593 100644 --- a/compiler/test/dotty/tools/dotc/LinkTests.scala +++ b/compiler/test/dotty/tools/dotc/LinkTests.scala @@ -26,7 +26,9 @@ class LinkTests extends ParallelTesting { def testFilter = Properties.testsFilter - @Test def linkTest: Unit = { + // Disabled until strawman is fixed + // @Test + def linkTest: Unit = { // Setup and compile libraries val strawmanLibGroup = TestGroup("linkTest/strawmanLibrary") val strawmanLibTestGroup = TestGroup(strawmanLibGroup + "/tests") From 07ced9d9f2bb304ea40f254a8ac40768f08502e5 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 20 Feb 2018 13:50:09 +0100 Subject: [PATCH 4/9] Be more careful when constraining parameter types wrt scrutinees i3989a.scala gives an example which is rejected under the new scheme. This would pass and fail at runtime under the previous scheme. --- .../dotty/tools/dotc/typer/Inferencing.scala | 53 +++++++++++++++++++ .../src/dotty/tools/dotc/typer/Typer.scala | 2 +- tests/neg/i3989a.scala | 10 ++++ tests/pos/t3880.scala | 5 +- tests/pos/t6084.scala | 5 +- 5 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 tests/neg/i3989a.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala index bc90ec7df8ae..fcc75ace61d1 100644 --- a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala +++ b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala @@ -154,6 +154,59 @@ object Inferencing { tree } + /** Derive information about a pattern type by comparing it with some variant of the + * static scrutinee type. We have the following situation in case of a (dynamic) pattern match: + * + * StaticScrutineeType PatternType + * \ / + * DynamicScrutineeType + * + * If `PatternType` is not a subtype of `StaticScrutineeType, there's no information to be gained. + * Now let's say we can prove that `PatternType <: StaticScrutineeType`. + * + * StaticScrutineeType + * | \ + * | \ + * | \ + * | PatternType + * | / + * DynamicScrutineeType + * + * What can we say about the relationship of parameter types between `PatternType` and + * `DynamicScrutineeType`? + * + * - If `DynamicScrutineeType` refines the type parameters of `StaticScrutineeType` + * in the same way as `PatternType`, the subtype test `PatternType <:< StaticScrutineeType` + * tells us all we need to know. + * - Otherwise, if variant refinement is a possibility we can only make predictions + * about invariant parameters of `StaticScrutineeType`. Hence we do a subtype test + * where `PatternType <: widenVariantParams(StaticScrutineeType)`, where `widenVariantParams` + * replaces all type argument of variant parameters with empty bounds. + */ + def constrainPatternType(tp: Type, pt: Type)(implicit ctx: Context) = { + def refinementIsInvariant(tp: Type): Boolean = tp match { + case tp: ClassInfo => tp.cls.is(Final) || tp.cls.is(Case) + case tp: TypeProxy => refinementIsInvariant(tp.underlying) + case tp: AndOrType => refinementIsInvariant(tp.tp1) && refinementIsInvariant(tp.tp2) + case _ => false + } + + def widenVariantParams = new TypeMap { + def apply(tp: Type) = mapOver(tp) match { + case tp @ AppliedType(tycon, args) => + val args1 = args.zipWithConserve(tycon.typeParams)((arg, tparam) => + if (tparam.paramVariance != 0) TypeBounds.empty else arg + ) + tp.derivedAppliedType(tycon, args1) + case tp => + tp + } + } + + val widePt = if (ctx.scala2Mode || refinementIsInvariant(tp)) pt else widenVariantParams(pt) + (tp <:< widePt)(ctx.addMode(Mode.GADTflexible)) + } + /** The list of uninstantiated type variables bound by some prefix of type `T` which * occur in at least one formal parameter type of a prefix application. * Considered prefixes are: diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 438b3ef9b279..4b9ce6bc57a2 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -569,7 +569,7 @@ class Typer extends Namer def typedTpt = checkSimpleKinded(typedType(tree.tpt)) def handlePattern: Tree = { val tpt1 = typedTpt - if (!ctx.isAfterTyper) tpt1.tpe.<:<(pt)(ctx.addMode(Mode.GADTflexible)) + if (!ctx.isAfterTyper) constrainPatternType(tpt1.tpe, pt) // special case for an abstract type that comes with a class tag tryWithClassTag(ascription(tpt1, isWildcard = true), pt) } diff --git a/tests/neg/i3989a.scala b/tests/neg/i3989a.scala new file mode 100644 index 000000000000..8ff02ac0c2f1 --- /dev/null +++ b/tests/neg/i3989a.scala @@ -0,0 +1,10 @@ +object Test extends App { + trait A[+X] + class B[+X](val x: X) extends A[X] + class C[+X](x: Any) extends B[Any](x) with A[X] + def f(a: A[Int]): Int = a match { + case a: B[_] => a.x // error + case _ => 0 + } + f(new C[Int]("foo")) +} diff --git a/tests/pos/t3880.scala b/tests/pos/t3880.scala index f778eb71a81a..d7b567e1901a 100644 --- a/tests/pos/t3880.scala +++ b/tests/pos/t3880.scala @@ -1,11 +1,12 @@ abstract class Bar[+B] { } -abstract class C1[+B] extends Bar[B] { +final class C1[+B] extends Bar[B] { private[this] def g(x: C1[B]): Unit = () // this method is fine: notice that it allows the call to g, // which requires C1[B], even though we matched on C1[_]. - // (That is good news.) + // (That is good news, but is sound only because C1 is final; see #3989 + // and compare with i3989a.scala. private[this] def f1(x: Bar[B]): Unit = x match { case x: C1[_] => g(x) } diff --git a/tests/pos/t6084.scala b/tests/pos/t6084.scala index 1aa1fed391e8..1107d9a03885 100644 --- a/tests/pos/t6084.scala +++ b/tests/pos/t6084.scala @@ -1,11 +1,14 @@ package object foo { type X[T, U] = (T => U) } package foo { - abstract class Foo[T, U](val d: T => U) extends (T => U) { + // Note that Foo must be final because of #3989. + final class Foo[T, U](val d: T => U) extends (T => U) { def f1(r: X[T, U]) = r match { case x: Foo[_,_] => x.d } // inferred ok def f2(r: X[T, U]): (T => U) = r match { case x: Foo[_,_] => x.d } // dealiased ok def f3(r: X[T, U]): X[T, U] = r match { case x: Foo[_,_] => x.d } // alias not ok + def apply(x: T): U = d(x) + // x.d : foo.this.package.type.X[?scala.reflect.internal.Types$NoPrefix$?.T, ?scala.reflect.internal.Types$NoPrefix$?.U] ~>scala.this.Function1[?scala.reflect.internal.Types$NoPrefix$?.T, ?scala.reflect.internal.Types$NoPrefix$?.U] // at scala.Predef$.assert(Predef.scala:170) // at scala.tools.nsc.Global.assert(Global.scala:235) From 7bac0a2e40bc53be48719247e3899a4e09cb7825 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 20 Feb 2018 14:38:48 +0100 Subject: [PATCH 5/9] Ensure invariant refinement for classes extended case classes --- .../dotty/tools/dotc/typer/Inferencing.scala | 7 ++++-- .../dotty/tools/dotc/typer/RefChecks.scala | 22 +++++++++++++++++++ tests/neg/i3989b.scala | 10 +++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 tests/neg/i3989b.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala index fcc75ace61d1..49cb38cbb0a9 100644 --- a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala +++ b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala @@ -176,12 +176,15 @@ object Inferencing { * `DynamicScrutineeType`? * * - If `DynamicScrutineeType` refines the type parameters of `StaticScrutineeType` - * in the same way as `PatternType`, the subtype test `PatternType <:< StaticScrutineeType` - * tells us all we need to know. + * in the same way as `PatternType` ("invariant refinement"), the subtype test + * `PatternType <:< StaticScrutineeType` tells us all we need to know. * - Otherwise, if variant refinement is a possibility we can only make predictions * about invariant parameters of `StaticScrutineeType`. Hence we do a subtype test * where `PatternType <: widenVariantParams(StaticScrutineeType)`, where `widenVariantParams` * replaces all type argument of variant parameters with empty bounds. + * + * Invariant refinement can be assumed if `PatternType`'s class(es) are final or + * case classes (because of `RefChecks#checkCaseClassInheritanceInvariant`). */ def constrainPatternType(tp: Type, pt: Type)(implicit ctx: Context) = { def refinementIsInvariant(tp: Type): Boolean = tp match { diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index bf4f7ce75d79..1f1b12d8e211 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -641,6 +641,27 @@ object RefChecks { } } + /** Check that inheriting a case class does not constitute a variant refinement + * of a base type of the case class. It is because of this restriction that we + * can assume invariant refinement for case classes in `constrainPatternType`. + */ + def checkCaseClassInheritanceInvariant() = { + for (caseCls <- clazz.info.baseClasses.tail.find(_.is(Case))) + for (bc <- caseCls.info.baseClasses.tail) + if (bc.typeParams.exists(_.paramVariance != 0)) { + val caseBT = self.baseType(caseCls) + val thisBT = self.baseType(bc) + val combinedBT = caseBT.baseType(bc) + if (!(thisBT =:= combinedBT)) + ctx.errorOrMigrationWarning( + em"""illegal inheritance: $clazz inherits case $caseCls + |but the two have different base type instances for $bc. + | + | Basetype for $clazz: $thisBT + | Basetype via $caseCls: $combinedBT""", clazz.pos) + } + } + checkNoAbstractMembers() if (abstractErrors.isEmpty) checkNoAbstractDecls(clazz) @@ -649,6 +670,7 @@ object RefChecks { ctx.error(abstractErrorMessage, clazz.pos) checkMemberTypesOK() + checkCaseClassInheritanceInvariant() } else if (clazz.is(Trait) && !(clazz derivesFrom defn.AnyValClass)) { // For non-AnyVal classes, prevent abstract methods in interfaces that override // final members in Object; see #4431 diff --git a/tests/neg/i3989b.scala b/tests/neg/i3989b.scala new file mode 100644 index 000000000000..82935fab6daa --- /dev/null +++ b/tests/neg/i3989b.scala @@ -0,0 +1,10 @@ +object Test extends App { + trait A[+X] + case class B[+X](val x: X) extends A[X] + class C[+X](x: Any) extends B[Any](x) with A[X] // error + def f(a: A[Int]): Int = a match { + case a: B[_] => a.x + case _ => 0 + } + f(new C[Int]("foo")) +} From f00ac9549ddd891403461e7e8c7ee7535eb3babc Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 20 Feb 2018 16:37:45 +0100 Subject: [PATCH 6/9] Use constrainPatternType also for unapply patterns --- .../src/dotty/tools/dotc/typer/Applications.scala | 2 +- .../src/dotty/tools/dotc/typer/Inferencing.scala | 4 ++-- compiler/src/dotty/tools/dotc/typer/Typer.scala | 2 +- tests/neg/i3989b.scala | 2 +- tests/neg/i3989c.scala | 15 +++++++++++++++ 5 files changed, 20 insertions(+), 5 deletions(-) create mode 100644 tests/neg/i3989c.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index fc2d4eae6422..11a77ec449ad 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -933,7 +933,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic => * - If a type proxy P is not a reference to a class, P's supertype is in G */ def isSubTypeOfParent(subtp: Type, tp: Type)(implicit ctx: Context): Boolean = - if (subtp <:< tp) true + if (constrainPatternType(subtp, tp)) true else tp match { case tp: TypeRef if tp.symbol.isClass => isSubTypeOfParent(subtp, tp.firstParent) case tp: TypeProxy => isSubTypeOfParent(subtp, tp.superType) diff --git a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala index 49cb38cbb0a9..c5caf8d8a0ba 100644 --- a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala +++ b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala @@ -186,7 +186,7 @@ object Inferencing { * Invariant refinement can be assumed if `PatternType`'s class(es) are final or * case classes (because of `RefChecks#checkCaseClassInheritanceInvariant`). */ - def constrainPatternType(tp: Type, pt: Type)(implicit ctx: Context) = { + def constrainPatternType(tp: Type, pt: Type)(implicit ctx: Context): Boolean = { def refinementIsInvariant(tp: Type): Boolean = tp match { case tp: ClassInfo => tp.cls.is(Final) || tp.cls.is(Case) case tp: TypeProxy => refinementIsInvariant(tp.underlying) @@ -207,7 +207,7 @@ object Inferencing { } val widePt = if (ctx.scala2Mode || refinementIsInvariant(tp)) pt else widenVariantParams(pt) - (tp <:< widePt)(ctx.addMode(Mode.GADTflexible)) + tp <:< widePt } /** The list of uninstantiated type variables bound by some prefix of type `T` which diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 4b9ce6bc57a2..f4ae59e887cb 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -569,7 +569,7 @@ class Typer extends Namer def typedTpt = checkSimpleKinded(typedType(tree.tpt)) def handlePattern: Tree = { val tpt1 = typedTpt - if (!ctx.isAfterTyper) constrainPatternType(tpt1.tpe, pt) + if (!ctx.isAfterTyper) constrainPatternType(tpt1.tpe, pt)(ctx.addMode(Mode.GADTflexible)) // special case for an abstract type that comes with a class tag tryWithClassTag(ascription(tpt1, isWildcard = true), pt) } diff --git a/tests/neg/i3989b.scala b/tests/neg/i3989b.scala index 82935fab6daa..fc44f40f8a81 100644 --- a/tests/neg/i3989b.scala +++ b/tests/neg/i3989b.scala @@ -3,7 +3,7 @@ object Test extends App { case class B[+X](val x: X) extends A[X] class C[+X](x: Any) extends B[Any](x) with A[X] // error def f(a: A[Int]): Int = a match { - case a: B[_] => a.x + case B(i) => i case _ => 0 } f(new C[Int]("foo")) diff --git a/tests/neg/i3989c.scala b/tests/neg/i3989c.scala new file mode 100644 index 000000000000..1d631ea1273d --- /dev/null +++ b/tests/neg/i3989c.scala @@ -0,0 +1,15 @@ +import scala.Option +object Test extends App { + trait A[+X] + class B[+X](val x: X) extends A[X] + object B { + def unapply[X](b: B[X]): Option[X] = Some(b.x) + } + + class C[+X](x: Any) extends B[Any](x) with A[X] + def f(a: A[Int]): Int = a match { + case B(i) => i // error + case _ => 0 + } + f(new C[Int]("foo")) +} From f89d2073ce8af77c6e6e3b1cbe49d486457aae96 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 20 Feb 2018 21:51:42 +0100 Subject: [PATCH 7/9] Close the protected[local] escape hatch --- .../tools/dotc/typer/VarianceChecker.scala | 12 ++++++---- tests/neg/i3989d.scala | 18 +++++++++++++++ tests/neg/t3272.scala | 8 +++++++ tests/{pos => neg}/t7093.scala | 22 +++++++++---------- .../CollectionStrawMan5.scala | 8 +++---- .../CollectionStrawMan6.scala | 8 +++---- tests/pos/GenericTraversableTemplate.scala | 2 +- tests/pos/t3272.scala | 8 ------- tests/pos/t4345.scala | 7 ------ tests/pos/variances-local.scala | 7 ------ tests/run/CollectionTests.scala | 6 ++--- .../run/colltest5/CollectionStrawMan5_1.scala | 8 +++---- .../run/colltest6/CollectionStrawMan6_1.scala | 8 +++---- 13 files changed, 64 insertions(+), 58 deletions(-) create mode 100644 tests/neg/i3989d.scala create mode 100644 tests/neg/t3272.scala rename tests/{pos => neg}/t7093.scala (62%) delete mode 100644 tests/pos/t3272.scala delete mode 100644 tests/pos/t4345.scala delete mode 100644 tests/pos/variances-local.scala diff --git a/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala b/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala index 39d635863426..0ba36c2ac306 100644 --- a/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala +++ b/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala @@ -32,7 +32,7 @@ class VarianceChecker()(implicit ctx: Context) { def ignoreVarianceIn(base: Symbol): Boolean = ( base.isTerm || base.is(Package) - || base.is(Local) + || base.is(PrivateLocal) ) /** The variance of a symbol occurrence of `tvar` seen at the level of the definition of `base`. @@ -112,9 +112,13 @@ class VarianceChecker()(implicit ctx: Context) { def checkVariance(sym: Symbol, pos: Position) = Validator.validateDefinition(sym) match { case Some(VarianceError(tvar, required)) => def msg = i"${varianceString(tvar.flags)} $tvar occurs in ${varianceString(required)} position in type ${sym.info} of $sym" - if (ctx.scala2Mode && sym.owner.isConstructor) { + if (ctx.scala2Mode && + (sym.owner.isConstructor || sym.ownersIterator.exists(_.is(ProtectedLocal)))) { ctx.migrationWarning(s"According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:\n$msg", pos) - patch(Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance") // TODO use an import or shorten if possible + // patch(Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance") + // Patch is disabled until two TODOs are solved: + // TODO use an import or shorten if possible + // TODO need to use a `:' if annotation is on term } else ctx.error(msg, pos) case None => @@ -125,7 +129,7 @@ class VarianceChecker()(implicit ctx: Context) { // No variance check for private/protected[this] methods/values. def skip = !sym.exists || - sym.is(Local) || // !!! watch out for protected local! + sym.is(PrivateLocal) || sym.is(TypeParam) && sym.owner.isClass // already taken care of in primary constructor of class tree match { case defn: MemberDef if skip => diff --git a/tests/neg/i3989d.scala b/tests/neg/i3989d.scala new file mode 100644 index 000000000000..e9260bc900ff --- /dev/null +++ b/tests/neg/i3989d.scala @@ -0,0 +1,18 @@ +trait A[+_X] { + protected[this] type X = _X // error: variance + def f: X +} + +trait B extends A[B] { + def f: X = new B {} +} + +class C extends B with A[C] { + // should be required because the inherited f is of type B, not C + // override def f: X = new C +} + +object Test extends App { + val c1 = new C + val c2: C = c1.f +} diff --git a/tests/neg/t3272.scala b/tests/neg/t3272.scala new file mode 100644 index 000000000000..b1c90cae5f90 --- /dev/null +++ b/tests/neg/t3272.scala @@ -0,0 +1,8 @@ +trait A { + trait C[+T] { + protected[this] def f(t: T): Unit = {} // error: covariant type T occurs in contravariant position in type T of value t + } + trait D[T] extends C[T] { + def g(t: T): Unit = { f(t) } + } +} diff --git a/tests/pos/t7093.scala b/tests/neg/t7093.scala similarity index 62% rename from tests/pos/t7093.scala rename to tests/neg/t7093.scala index 287b7a78c726..dc5c25ac4f2f 100644 --- a/tests/pos/t7093.scala +++ b/tests/neg/t7093.scala @@ -1,12 +1,10 @@ -object Test { - - trait A[+X] { - protected[this] def f(x: X): X = x - } +trait A[+X] { + protected[this] def f(x: X): X = x // error: variance +} - trait B extends A[B] { - def kaboom = f(new B {}) - } +trait B extends A[B] { + def kaboom = f(new B {}) +} // protected[this] disables variance checking // of the signature of `f`. @@ -16,12 +14,12 @@ object Test { // The protected[this] loophole is widely used // in the collections, every newBuilder method // would fail variance checking otherwise. - class C extends B with A[C] { - override protected[this] def f(c: C) = c - } +class C extends B with A[C] { + override protected[this] def f(c: C) = c +} // java.lang.ClassCastException: B$$anon$1 cannot be cast to C // at C.f(:15) +object Test extends App { new C().kaboom } - diff --git a/tests/pos-special/strawman-collections/CollectionStrawMan5.scala b/tests/pos-special/strawman-collections/CollectionStrawMan5.scala index e97051d2bccf..8fea155ed708 100644 --- a/tests/pos-special/strawman-collections/CollectionStrawMan5.scala +++ b/tests/pos-special/strawman-collections/CollectionStrawMan5.scala @@ -56,7 +56,7 @@ object CollectionStrawMan5 { /** Base trait for strict collections */ trait Buildable[+A, +To <: Iterable[A]] extends Iterable[A] { - protected[this] def newBuilder: Builder[A, To] + protected[this] def newBuilder: Builder[A @uncheckedVariance, To] override def partition(p: A => Boolean): (To, To) = { val l, r = newBuilder iterator.foreach(x => (if (p(x)) l else r) += x) @@ -95,7 +95,7 @@ object CollectionStrawMan5 { with IterableOps[A] with IterableMonoTransforms[A, C[A @uncheckedVariance]] // sound bcs of VarianceNote with IterablePolyTransforms[A, C] { - protected[this] def fromLikeIterable(coll: Iterable[A]): C[A] = fromIterable(coll) + protected[this] def fromLikeIterable(coll: Iterable[A @uncheckedVariance]): C[A @uncheckedVariance] = fromIterable(coll) } /** Base trait for Seq operations */ @@ -115,7 +115,7 @@ object CollectionStrawMan5 { trait IterableMonoTransforms[+A, +Repr] extends Any { protected def coll: Iterable[A] - protected[this] def fromLikeIterable(coll: Iterable[A]): Repr + protected[this] def fromLikeIterable(coll: Iterable[A] @scala.annotation.unchecked.uncheckedVariance): Repr def filter(p: A => Boolean): Repr = fromLikeIterable(View.Filter(coll, p)) def partition(p: A => Boolean): (Repr, Repr) = { val pn = View.Partition(coll, p) @@ -166,7 +166,7 @@ object CollectionStrawMan5 { } def length: Int = if (isEmpty) 0 else 1 + tail.length - protected[this] def newBuilder = new ListBuffer[A] + protected[this] def newBuilder = new ListBuffer[A @uncheckedVariance] def ++:[B >: A](prefix: List[B]): List[B] = if (prefix.isEmpty) this else Cons(prefix.head, prefix.tail ++: this) diff --git a/tests/pos-special/strawman-collections/CollectionStrawMan6.scala b/tests/pos-special/strawman-collections/CollectionStrawMan6.scala index 14b2bbae0d1b..a2ff42ba783e 100644 --- a/tests/pos-special/strawman-collections/CollectionStrawMan6.scala +++ b/tests/pos-special/strawman-collections/CollectionStrawMan6.scala @@ -168,7 +168,7 @@ object CollectionStrawMan6 extends LowPriority { trait Buildable[+A, +Repr] extends Any with IterableMonoTransforms[A, Repr] { /** Creates a new builder. */ - protected[this] def newBuilder: Builder[A, Repr] + protected[this] def newBuilder: Builder[A, Repr] @uncheckedVariance /** Optimized, push-based version of `partition`. */ override def partition(p: A => Boolean): (Repr, Repr) = { @@ -226,7 +226,7 @@ object CollectionStrawMan6 extends LowPriority { /** Create a collection of type `C[A]` from the elements of `coll`, which has * the same element type as this collection. Overridden in StringOps and ArrayOps. */ - protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): C[A] = fromIterable(coll) + protected[this] def fromIterableWithSameElemType(coll: Iterable[A] @uncheckedVariance): C[A]@uncheckedVariance = fromIterable(coll) } /** Base trait for Seq operations */ @@ -349,7 +349,7 @@ object CollectionStrawMan6 extends LowPriority { */ trait IterableMonoTransforms[+A, +Repr] extends Any { protected def coll: Iterable[A] - protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): Repr + protected[this] def fromIterableWithSameElemType(coll: Iterable[A] @uncheckedVariance): Repr /** All elements satisfying predicate `p` */ def filter(p: A => Boolean): Repr = fromIterableWithSameElemType(View.Filter(coll, p)) @@ -420,7 +420,7 @@ object CollectionStrawMan6 extends LowPriority { def fromIterable[B](c: Iterable[B]): List[B] = List.fromIterable(c) - protected[this] def newBuilder = new ListBuffer[A].mapResult(_.toList) + protected[this] def newBuilder = new ListBuffer[A].mapResult(_.toList): @uncheckedVariance /** Prepend element */ def :: [B >: A](elem: B): List[B] = new ::(elem, this) diff --git a/tests/pos/GenericTraversableTemplate.scala b/tests/pos/GenericTraversableTemplate.scala index 70e2376d066f..e9120e33cb69 100644 --- a/tests/pos/GenericTraversableTemplate.scala +++ b/tests/pos/GenericTraversableTemplate.scala @@ -62,7 +62,7 @@ trait GenericTraversableTemplate[+A, +CC[X] <: GenTraversable[X]] extends HasNew /** The builder that builds instances of type $Coll[A] */ - protected[this] def newBuilder: Builder[A, CC[A]] = companion.newBuilder[A] + protected[this] def newBuilder: Builder[A, CC[A]] @uncheckedVariance = companion.newBuilder[A] /** The generic builder that builds instances of $Coll * at arbitrary element types. diff --git a/tests/pos/t3272.scala b/tests/pos/t3272.scala deleted file mode 100644 index cf54d6a848fe..000000000000 --- a/tests/pos/t3272.scala +++ /dev/null @@ -1,8 +0,0 @@ -trait A { - trait C[+T] { - protected[this] def f(t: T): Unit = {} - } - trait D[T] extends C[T] { - def g(t: T): Unit = { f(t) } - } -} diff --git a/tests/pos/t4345.scala b/tests/pos/t4345.scala deleted file mode 100644 index b0131d5fa5f5..000000000000 --- a/tests/pos/t4345.scala +++ /dev/null @@ -1,7 +0,0 @@ -trait C1[+A, +CC[X]] { - protected[this] def f: A => CC[A] = sys.error("") -} - -trait C2[+A, +CC[X]] extends C1[A, CC] { - override protected[this] def f = super.f -} diff --git a/tests/pos/variances-local.scala b/tests/pos/variances-local.scala deleted file mode 100644 index 35e395095cf1..000000000000 --- a/tests/pos/variances-local.scala +++ /dev/null @@ -1,7 +0,0 @@ -class Foo1[+T] { - private[this] type MyType = T -} - -class Foo2[+T] { - protected[this] type MyType = T -} diff --git a/tests/run/CollectionTests.scala b/tests/run/CollectionTests.scala index 3c08bee0e874..c9d2a2bbb51d 100644 --- a/tests/run/CollectionTests.scala +++ b/tests/run/CollectionTests.scala @@ -55,7 +55,7 @@ object CollectionStrawMan5 { /** Base trait for strict collections */ trait Buildable[+A, +To <: Iterable[A]] extends Iterable[A] { - protected[this] def newBuilder: Builder[A, To] + protected[this] def newBuilder: Builder[A @uncheckedVariance, To] override def partition(p: A => Boolean): (To, To) = { val l, r = newBuilder iterator.foreach(x => (if (p(x)) l else r) += x) @@ -94,7 +94,7 @@ object CollectionStrawMan5 { with IterableOps[A] with IterableMonoTransforms[A, C[A @uncheckedVariance]] // sound bcs of VarianceNote with IterablePolyTransforms[A, C] { - protected[this] def fromLikeIterable(coll: Iterable[A]): C[A] = fromIterable(coll) + protected[this] def fromLikeIterable(coll: Iterable[A @uncheckedVariance]): C[A @uncheckedVariance] = fromIterable(coll) } /** Base trait for Seq operations */ @@ -114,7 +114,7 @@ object CollectionStrawMan5 { trait IterableMonoTransforms[+A, +Repr] extends Any { protected def coll: Iterable[A] - protected[this] def fromLikeIterable(coll: Iterable[A]): Repr + protected[this] def fromLikeIterable(coll: Iterable[A @uncheckedVariance ]): Repr def filter(p: A => Boolean): Repr = fromLikeIterable(View.Filter(coll, p)) def partition(p: A => Boolean): (Repr, Repr) = { val pn = View.Partition(coll, p) diff --git a/tests/run/colltest5/CollectionStrawMan5_1.scala b/tests/run/colltest5/CollectionStrawMan5_1.scala index 183f24bb166b..620359b5c98e 100644 --- a/tests/run/colltest5/CollectionStrawMan5_1.scala +++ b/tests/run/colltest5/CollectionStrawMan5_1.scala @@ -48,7 +48,7 @@ object CollectionStrawMan5 { /** Base trait for strict collections */ trait Buildable[+A, +To <: Iterable[A]] extends Iterable[A] { - protected[this] def newBuilder: Builder[A, To] + protected[this] def newBuilder: Builder[A, To] @uncheckedVariance override def partition(p: A => Boolean): (To, To) = { val l, r = newBuilder iterator.foreach(x => (if (p(x)) l else r) += x) @@ -87,7 +87,7 @@ object CollectionStrawMan5 { with IterableOps[A] with IterableMonoTransforms[A, C[A @uncheckedVariance]] // sound bcs of VarianceNote with IterablePolyTransforms[A, C] { - protected[this] def fromLikeIterable(coll: Iterable[A]): C[A] = fromIterable(coll) + protected[this] def fromLikeIterable(coll: Iterable[A] @uncheckedVariance): C[A] @uncheckedVariance = fromIterable(coll) } /** Base trait for Seq operations */ @@ -107,7 +107,7 @@ object CollectionStrawMan5 { trait IterableMonoTransforms[+A, +Repr] extends Any { protected def coll: Iterable[A] - protected[this] def fromLikeIterable(coll: Iterable[A]): Repr + protected[this] def fromLikeIterable(coll: Iterable[A] @uncheckedVariance): Repr def filter(p: A => Boolean): Repr = fromLikeIterable(View.Filter(coll, p)) def partition(p: A => Boolean): (Repr, Repr) = { val pn = View.Partition(coll, p) @@ -158,7 +158,7 @@ object CollectionStrawMan5 { } def length: Int = if (isEmpty) 0 else 1 + tail.length - protected[this] def newBuilder = new ListBuffer[A] + protected[this] def newBuilder = new ListBuffer[A] @uncheckedVariance def ++:[B >: A](prefix: List[B]): List[B] = if (prefix.isEmpty) this else Cons(prefix.head, prefix.tail ++: this) diff --git a/tests/run/colltest6/CollectionStrawMan6_1.scala b/tests/run/colltest6/CollectionStrawMan6_1.scala index 17c28d74ef0f..4fad57154775 100644 --- a/tests/run/colltest6/CollectionStrawMan6_1.scala +++ b/tests/run/colltest6/CollectionStrawMan6_1.scala @@ -169,7 +169,7 @@ object CollectionStrawMan6 extends LowPriority { trait Buildable[+A, +Repr] extends Any with IterableMonoTransforms[A, Repr] { /** Creates a new builder. */ - protected[this] def newBuilder: Builder[A, Repr] + protected[this] def newBuilder: Builder[A @uncheckedVariance, Repr] /** Optimized, push-based version of `partition`. */ override def partition(p: A => Boolean): (Repr, Repr) = { @@ -227,7 +227,7 @@ object CollectionStrawMan6 extends LowPriority { /** Create a collection of type `C[A]` from the elements of `coll`, which has * the same element type as this collection. Overridden in StringOps and ArrayOps. */ - protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): C[A] = fromIterable(coll) + protected[this] def fromIterableWithSameElemType(coll: Iterable[A @uncheckedVariance]): C[A @uncheckedVariance] = fromIterable(coll) } /** Base trait for Seq operations */ @@ -350,7 +350,7 @@ object CollectionStrawMan6 extends LowPriority { */ trait IterableMonoTransforms[+A, +Repr] extends Any { protected def coll: Iterable[A] - protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): Repr + protected[this] def fromIterableWithSameElemType(coll: Iterable[A @uncheckedVariance]): Repr /** All elements satisfying predicate `p` */ def filter(p: A => Boolean): Repr = fromIterableWithSameElemType(View.Filter(coll, p)) @@ -421,7 +421,7 @@ object CollectionStrawMan6 extends LowPriority { def fromIterable[B](c: Iterable[B]): List[B] = List.fromIterable(c) - protected[this] def newBuilder = new ListBuffer[A].mapResult(_.toList) + protected[this] def newBuilder = new ListBuffer[A @uncheckedVariance].mapResult(_.toList) /** Prepend element */ def :: [B >: A](elem: B): List[B] = new ::(elem, this) From 1082eb564373dc2111c5cfbf67842dd58e2fda96 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 16 Mar 2018 17:59:58 +0100 Subject: [PATCH 8/9] Disallow extending a trait twice in the same class --- compiler/src/dotty/tools/dotc/typer/Typer.scala | 4 ++++ tests/neg/i3989e.scala | 11 +++++++++++ 2 files changed, 15 insertions(+) create mode 100644 tests/neg/i3989e.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index f4ae59e887cb..5e183da83fef 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -1460,9 +1460,13 @@ class Typer extends Namer ref } + val seenParents = mutable.Set[Symbol]() + def typedParent(tree: untpd.Tree): Tree = { var result = if (tree.isType) typedType(tree)(superCtx) else typedExpr(tree)(superCtx) val psym = result.tpe.typeSymbol + if (seenParents.contains(psym)) ctx.error(i"$psym is extended twice", tree.pos) + seenParents += psym if (tree.isType) { if (psym.is(Trait) && !cls.is(Trait) && !cls.superClass.isSubClass(psym)) result = maybeCall(result, psym, psym.primaryConstructor.info) diff --git a/tests/neg/i3989e.scala b/tests/neg/i3989e.scala new file mode 100644 index 000000000000..33b5ee665815 --- /dev/null +++ b/tests/neg/i3989e.scala @@ -0,0 +1,11 @@ +object Test extends App { + trait A[+X](val x: X) + class B extends A(5) with A("hello") // error: A is extended twice + + def f(a: A[Int]): Int = a match { + case b: B => b.x + case _ => 0 + } + + f(new B) +} \ No newline at end of file From 4feda4e0161c13698cf94a84d33349db55a10d60 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 16 Mar 2018 18:10:03 +0100 Subject: [PATCH 9/9] Enforce non-variant refinement for parameterized base classes and traits --- .../dotty/tools/dotc/typer/Inferencing.scala | 3 +- .../dotty/tools/dotc/typer/RefChecks.scala | 63 ++++++++++++++----- tests/neg/i3989f.scala | 13 ++++ tests/neg/i3989g.scala | 12 ++++ 4 files changed, 76 insertions(+), 15 deletions(-) create mode 100644 tests/neg/i3989f.scala create mode 100644 tests/neg/i3989g.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala index c5caf8d8a0ba..9b6c128d023b 100644 --- a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala +++ b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala @@ -190,7 +190,8 @@ object Inferencing { def refinementIsInvariant(tp: Type): Boolean = tp match { case tp: ClassInfo => tp.cls.is(Final) || tp.cls.is(Case) case tp: TypeProxy => refinementIsInvariant(tp.underlying) - case tp: AndOrType => refinementIsInvariant(tp.tp1) && refinementIsInvariant(tp.tp2) + case tp: AndType => refinementIsInvariant(tp.tp1) && refinementIsInvariant(tp.tp2) + case tp: OrType => refinementIsInvariant(tp.tp1) && refinementIsInvariant(tp.tp2) case _ => false } diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index 1f1b12d8e211..a0ad815fa6e1 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -647,21 +647,11 @@ object RefChecks { */ def checkCaseClassInheritanceInvariant() = { for (caseCls <- clazz.info.baseClasses.tail.find(_.is(Case))) - for (bc <- caseCls.info.baseClasses.tail) - if (bc.typeParams.exists(_.paramVariance != 0)) { - val caseBT = self.baseType(caseCls) - val thisBT = self.baseType(bc) - val combinedBT = caseBT.baseType(bc) - if (!(thisBT =:= combinedBT)) - ctx.errorOrMigrationWarning( - em"""illegal inheritance: $clazz inherits case $caseCls - |but the two have different base type instances for $bc. - | - | Basetype for $clazz: $thisBT - | Basetype via $caseCls: $combinedBT""", clazz.pos) - } + for (baseCls <- caseCls.info.baseClasses.tail) + if (baseCls.typeParams.exists(_.paramVariance != 0)) + for (problem <- variantInheritanceProblems(baseCls, caseCls, "non-variant", "case ")) + ctx.errorOrMigrationWarning(problem(), clazz.pos) } - checkNoAbstractMembers() if (abstractErrors.isEmpty) checkNoAbstractDecls(clazz) @@ -684,6 +674,51 @@ object RefChecks { } } + if (!clazz.is(Trait)) { + // check that parameterized base classes and traits are typed in the same way as from the superclass + // I.e. say we have + // + // Sub extends Super extends* Base + // + // where `Base` has value parameters. Enforce that + // + // Sub.thisType.baseType(Base) =:= Sub.thisType.baseType(Super).baseType(Base) + // + // This is necessary because parameter values are determined directly or indirectly + // by `Super`. So we cannot pretend they have a different type when seen from `Sub`. + def checkParameterizedTraitsOK() = { + val mixins = clazz.mixins + for { + cls <- clazz.info.baseClasses.tail + if cls.paramAccessors.nonEmpty && !mixins.contains(cls) + problem <- variantInheritanceProblems(cls, clazz.asClass.superClass, "parameterized", "super") + } ctx.error(problem(), clazz.pos) + } + + checkParameterizedTraitsOK() + } + + /** Check that `site` does not inherit conflicting generic instances of `baseCls`, + * when doing a direct base type or going via intermediate class `middle`. I.e, we require: + * + * site.baseType(baseCls) =:= site.baseType(middle).baseType(baseCls) + * + * Return an optional by name error message if this test fails. + */ + def variantInheritanceProblems( + baseCls: Symbol, middle: Symbol, baseStr: String, middleStr: String): Option[() => String] = { + val superBT = self.baseType(middle) + val thisBT = self.baseType(baseCls) + val combinedBT = superBT.baseType(baseCls) + if (combinedBT =:= thisBT) None // ok + else + Some(() => + em"""illegal inheritance: $clazz inherits conflicting instances of $baseStr base $baseCls. + | + | Direct basetype: $thisBT + | Basetype via $middleStr$middle: $combinedBT""") + } + /* Returns whether there is a symbol declared in class `inclazz` * (which must be different from `clazz`) whose name and type * seen as a member of `class.thisType` matches `member`'s. diff --git a/tests/neg/i3989f.scala b/tests/neg/i3989f.scala new file mode 100644 index 000000000000..ce68f16b54b0 --- /dev/null +++ b/tests/neg/i3989f.scala @@ -0,0 +1,13 @@ +object Test extends App { + trait A[+X](val x: X) + class B[+X](val y: X) extends A[X](y) + class C extends B(5) with A[String] // error: illegal inheritance + + class D extends B(5) with A[Any] // ok + + def f(a: A[Int]): String = a match { + case c: C => c.x + case _ => "hello" + } + f(new C) +} \ No newline at end of file diff --git a/tests/neg/i3989g.scala b/tests/neg/i3989g.scala new file mode 100644 index 000000000000..fe42b179effb --- /dev/null +++ b/tests/neg/i3989g.scala @@ -0,0 +1,12 @@ +object Test extends App { + class A[+X](val x: X) + class B[+X](val y: X) extends A[X](y) + trait T[+X] extends A[X] + class C extends B(5) with T[String] // error: illegal inheritance + + def f(a: A[Int]): String = a match { + case c: C => c.x + case _ => "hello" + } + f(new C) +} \ No newline at end of file