Skip to content

Commit

Permalink
Fix scala#3989: Check that members of concrete classes are type-correct
Browse files Browse the repository at this point in the history
This fixes the second half of scala#3989. Some tests had to be updated or
re-classified because they were unsound before.
  • Loading branch information
odersky committed Feb 20, 2018
1 parent 9053964 commit 69b1547
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 7 deletions.
49 changes: 49 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/neg/i1240b.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
7 changes: 7 additions & 0 deletions tests/neg/i3989.scala
Original file line number Diff line number Diff line change
@@ -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"))
}
6 changes: 4 additions & 2 deletions tests/pos/t4731.scala → tests/neg/t4731.scala
Original file line number Diff line number Diff line change
@@ -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])
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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] = _

Expand Down
4 changes: 2 additions & 2 deletions tests/run/colltest6/CollectionStrawMan6_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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] = _

Expand Down

0 comments on commit 69b1547

Please sign in to comment.