From b781e25afea36e2839d207125d3b91b35571d8ec Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 12 Aug 2010 09:03:30 +0000 Subject: Fixed type soundness problem someone raised on ... Fixed type soundness problem someone raised on hackers news. Test in override.scala. Review by moors. --- .../scala/tools/nsc/typechecker/RefChecks.scala | 117 +++++++++++++-------- test/files/neg/override.check | 5 + test/files/neg/override.scala | 15 +++ test/files/pos/t3688.scala | 9 ++ 4 files changed, 100 insertions(+), 46 deletions(-) create mode 100644 test/files/neg/override.check create mode 100755 test/files/neg/override.scala create mode 100644 test/files/pos/t3688.scala diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index c0510dd33d..5c577d8af4 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -80,6 +80,7 @@ abstract class RefChecks extends InfoTransform { var localTyper: analyzer.Typer = typer; var currentApplication: Tree = EmptyTree var inPattern: Boolean = false + var checkedCombinations = Set[List[Type]]() // only one overloaded alternative is allowed to define default arguments private def checkDefaultsInOverloaded(clazz: Symbol) { @@ -186,7 +187,7 @@ abstract class RefChecks extends InfoTransform { * 4. Check that every member with an `override' modifier * overrides some other member. */ - private def checkAllOverrides(clazz: Symbol) { + private def checkAllOverrides(clazz: Symbol, typesOnly: Boolean = false) { case class MixinOverrideError(member: Symbol, msg: String) @@ -299,56 +300,65 @@ abstract class RefChecks extends InfoTransform { def intersectionIsEmpty(syms1: List[Symbol], syms2: List[Symbol]) = !(syms1 exists (syms2 contains)) - if (member hasFlag PRIVATE) { // (1.1) - overrideError("has weaker access privileges; it should not be private") - } - val mb = member.accessBoundary(member.owner) - val ob = other.accessBoundary(member.owner) - if (mb != RootClass && mb != NoSymbol && // todo: change - (ob == RootClass || ob == NoSymbol || !ob.hasTransOwner(mb) || - (other hasFlag PROTECTED) && !(member hasFlag PROTECTED))) { - overrideAccessError() + if (typesOnly) checkOverrideTypes() + else { + if (member hasFlag PRIVATE) { // (1.1) + overrideError("has weaker access privileges; it should not be private") + } + val mb = member.accessBoundary(member.owner) + val ob = other.accessBoundary(member.owner) + if (mb != RootClass && mb != NoSymbol && // todo: change + (ob == RootClass || ob == NoSymbol || !ob.hasTransOwner(mb) || + (other hasFlag PROTECTED) && !(member hasFlag PROTECTED))) { + overrideAccessError() + } + else if (other.isClass || other.isModule) { + overrideError("cannot be used here - classes and objects cannot be overridden"); + } else if (!other.isDeferred && (member.isClass || member.isModule)) { + overrideError("cannot be used here - classes and objects can only override abstract types"); + } else if (other hasFlag FINAL) { // (1.2) + overrideError("cannot override final member"); + } else if (!other.isDeferred && !(member hasFlag (OVERRIDE | ABSOVERRIDE | SYNTHETIC))) { // (1.3), SYNTHETIC because of DEVIRTUALIZE + overrideError("needs `override' modifier"); + } else if ((other hasFlag ABSOVERRIDE) && other.isIncompleteIn(clazz) && !(member hasFlag ABSOVERRIDE)) { + overrideError("needs `abstract override' modifiers") + } else if ((member hasFlag (OVERRIDE | ABSOVERRIDE)) && + (other hasFlag ACCESSOR) && other.accessed.isVariable && !other.accessed.hasFlag(LAZY)) { + overrideError("cannot override a mutable variable") + } else if ((member hasFlag (OVERRIDE | ABSOVERRIDE)) && + !(member.owner.thisType.baseClasses exists (_ isSubClass other.owner)) && + !member.isDeferred && !other.isDeferred && + intersectionIsEmpty(member.allOverriddenSymbols, other.allOverriddenSymbols)) { + overrideError("cannot override a concrete member without a third member that's overridden by both "+ + "(this rule is designed to prevent ``accidental overrides'')") + } else if (other.isStable && !member.isStable) { // (1.4) + overrideError("needs to be a stable, immutable value") + } else if (member.isValue && (member hasFlag LAZY) && + other.isValue && !other.isSourceMethod && !other.isDeferred && !(other hasFlag LAZY)) { + overrideError("cannot override a concrete non-lazy value") + } else if (other.isValue && (other hasFlag LAZY) && !other.isSourceMethod && !other.isDeferred && + member.isValue && !(member hasFlag LAZY)) { + overrideError("must be declared lazy to override a concrete lazy value") + } else { + checkOverrideTypes() + } } - else if (other.isClass || other.isModule) { - overrideError("cannot be used here - classes and objects cannot be overridden"); - } else if (!other.isDeferred && (member.isClass || member.isModule)) { - overrideError("cannot be used here - classes and objects can only override abstract types"); - } else if (other hasFlag FINAL) { // (1.2) - overrideError("cannot override final member"); - } else if (!other.isDeferred && !(member hasFlag (OVERRIDE | ABSOVERRIDE | SYNTHETIC))) { // (1.3), SYNTHETIC because of DEVIRTUALIZE - overrideError("needs `override' modifier"); - } else if ((other hasFlag ABSOVERRIDE) && other.isIncompleteIn(clazz) && !(member hasFlag ABSOVERRIDE)) { - overrideError("needs `abstract override' modifiers") - } else if ((member hasFlag (OVERRIDE | ABSOVERRIDE)) && - (other hasFlag ACCESSOR) && other.accessed.isVariable && !other.accessed.hasFlag(LAZY)) { - overrideError("cannot override a mutable variable") - } else if ((member hasFlag (OVERRIDE | ABSOVERRIDE)) && - !(member.owner.thisType.baseClasses exists (_ isSubClass other.owner)) && - !member.isDeferred && !other.isDeferred && - intersectionIsEmpty(member.allOverriddenSymbols, other.allOverriddenSymbols)) { - overrideError("cannot override a concrete member without a third member that's overridden by both "+ - "(this rule is designed to prevent ``accidental overrides'')") - } else if (other.isStable && !member.isStable) { // (1.4) - overrideError("needs to be a stable, immutable value") - } else if (member.isValue && (member hasFlag LAZY) && - other.isValue && !other.isSourceMethod && !other.isDeferred && !(other hasFlag LAZY)) { - overrideError("cannot override a concrete non-lazy value") - } else if (other.isValue && (other hasFlag LAZY) && !other.isSourceMethod && !other.isDeferred && - member.isValue && !(member hasFlag LAZY)) { - overrideError("must be declared lazy to override a concrete lazy value") - } else { + + def checkOverrideTypes() { if (other.isAliasType) { - //if (!member.typeParams.isEmpty) // (1.5) @MAT + //if (!member.typeParams.isEmpty) (1.5) @MAT // overrideError("may not be parameterized"); - //if (!other.typeParams.isEmpty) // (1.5) @MAT + //if (!other.typeParams.isEmpty) (1.5) @MAT // overrideError("may not override parameterized type"); // @M: substSym + if (!(self.memberType(member).substSym(member.typeParams, other.typeParams) =:= self.memberType(other))) // (1.6) overrideTypeError(); } else if (other.isAbstractType) { //if (!member.typeParams.isEmpty) // (1.7) @MAT // overrideError("may not be parameterized"); - var memberTp = self.memberType(member) + + val memberTp = self.memberType(member) val otherTp = self.memberInfo(other) if (!(otherTp.bounds containsType memberTp)) { // (1.7.1) overrideTypeError(); // todo: do an explaintypes with bounds here @@ -369,7 +379,7 @@ abstract class RefChecks extends InfoTransform { // this overlaps somewhat with validateVariance if(member.isAliasType) { val kindErrors = typer.infer.checkKindBounds(List(member), List(memberTp.normalize), self, member.owner) - +2 if(!kindErrors.isEmpty) unit.error(member.pos, "The kind of the right-hand side "+memberTp.normalize+" of "+member.keyString+" "+ @@ -381,10 +391,25 @@ abstract class RefChecks extends InfoTransform { } } else if (other.isTerm) { other.cookJavaRawInfo() // #2454 - - if (!overridesType(self.memberInfo(member), self.memberInfo(other))) { // 8 + val memberTp = self.memberType(member) + val otherTp = self.memberType(other) + if (!overridesType(memberTp, otherTp)) { // 8 overrideTypeError() - explainTypes(self.memberInfo(member), self.memberInfo(other)) + explainTypes(memberTp, otherTp) + } + + if (member.isStable && !otherTp.isVolatile) { + if (memberTp.isVolatile) + overrideError("has a volatile type; cannot override a member with non-volatile type") + else memberTp.normalize.resultType match { + case rt: RefinedType if !(rt =:= otherTp) && !(checkedCombinations contains rt.parents) => + // might mask some inconsistencies -- check overrides + checkedCombinations += rt.parents + val tsym = rt.typeSymbol; + if (tsym.pos == NoPosition) tsym setPos member.pos + checkAllOverrides(tsym, typesOnly = true) + case _ => + } } } } @@ -400,7 +425,7 @@ abstract class RefChecks extends InfoTransform { printMixinOverrideErrors() // Verifying a concrete class has nothing unimplemented. - if (clazz.isClass && !clazz.isTrait && !(clazz hasFlag ABSTRACT)) { + if (clazz.isClass && !clazz.isTrait && !(clazz hasFlag ABSTRACT) && !typesOnly) { val abstractErrors = new ListBuffer[String] def abstractErrorMessage = // a little formatting polish diff --git a/test/files/neg/override.check b/test/files/neg/override.check new file mode 100644 index 0000000000..0336fb2b11 --- /dev/null +++ b/test/files/neg/override.check @@ -0,0 +1,5 @@ +override.scala:9: error: overriding type T in trait A with bounds >: Int <: Int; + type T in trait B with bounds >: String <: String has incompatible type + lazy val x : A with B = x + ^ +one error found diff --git a/test/files/neg/override.scala b/test/files/neg/override.scala new file mode 100755 index 0000000000..764b06603a --- /dev/null +++ b/test/files/neg/override.scala @@ -0,0 +1,15 @@ +trait X { + trait A { type T >: Int <: Int } + val x : A + var n : x.T = 3 +} + +trait Y extends X { + trait B { type T >: String <: String } + lazy val x : A with B = x + n = "foo" +} + +object Test extends Application { + new Y {} +} diff --git a/test/files/pos/t3688.scala b/test/files/pos/t3688.scala new file mode 100644 index 0000000000..0ac1cfe514 --- /dev/null +++ b/test/files/pos/t3688.scala @@ -0,0 +1,9 @@ +import collection.mutable +import collection.JavaConversions._ +import java.{util => ju} + +object Test { + + implicitly[mutable.Map[Int, String] => ju.Dictionary[Int, String]] + +} -- cgit v1.2.3