diff options
author | Paul Phillips <paulp@improving.org> | 2012-07-21 19:49:55 -0700 |
---|---|---|
committer | Paul Phillips <paulp@improving.org> | 2012-07-27 05:42:17 -0700 |
commit | bc719cb8e4957a80e423350d8e993f1fa6a2a997 (patch) | |
tree | 2939e8688721c66fd1493bdadf9260d75faa4514 | |
parent | ad08f24448729009fc8d5ff0acf307a43b4cfe0a (diff) | |
download | scala-bc719cb8e4957a80e423350d8e993f1fa6a2a997.tar.gz scala-bc719cb8e4957a80e423350d8e993f1fa6a2a997.tar.bz2 scala-bc719cb8e4957a80e423350d8e993f1fa6a2a997.zip |
Improve unchecked warnings a lot.
Don't warn on "uncheckable" type patterns if they can be
statically guaranteed, regardless of their runtime checkability.
This covers patterns like Seq[Any] and lots more.
Review by @adriaanm.
-rw-r--r-- | src/compiler/scala/tools/nsc/typechecker/Infer.scala | 133 | ||||
-rw-r--r-- | src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala | 3 | ||||
-rw-r--r-- | src/compiler/scala/tools/nsc/typechecker/Typers.scala | 3 | ||||
-rw-r--r-- | test/files/neg/unchecked.check | 19 | ||||
-rw-r--r-- | test/files/neg/unchecked.flags | 1 | ||||
-rw-r--r-- | test/files/neg/unchecked.scala | 74 | ||||
-rw-r--r-- | test/files/neg/unchecked2.check | 12 | ||||
-rw-r--r-- | test/files/pos/unchecked-a.flags | 1 | ||||
-rw-r--r-- | test/files/pos/unchecked-a.scala | 15 |
9 files changed, 198 insertions, 63 deletions
diff --git a/src/compiler/scala/tools/nsc/typechecker/Infer.scala b/src/compiler/scala/tools/nsc/typechecker/Infer.scala index bcbcb96400..e096b75d6d 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Infer.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Infer.scala @@ -1324,66 +1324,89 @@ trait Infer { } // if top-level abstract types can be checked using a classtag extractor, don't warn about them - def checkCheckable(tree: Tree, tp: Type, inPattern: Boolean, canRemedy: Boolean = false) = { - val kind = if (inPattern) "pattern " else "" + def checkCheckable(tree: Tree, typeToTest: Type, typeEnsured: Type, inPattern: Boolean, canRemedy: Boolean = false) = { + log(s"checkCheckable($tree, $typeToTest, $typeEnsured, inPattern = $inPattern, canRemedy = $canRemedy") - def patternWarning(tp0: Type, prefix: String) = { - context.unit.uncheckedWarning(tree.pos, prefix+tp0+" in type "+kind+tp+" is unchecked since it is eliminated by erasure") + sealed abstract class TypeConformance(check: (Type, Type) => Boolean) { + def apply(t1: Type, t2: Type): Boolean = check(t1, t2) && { + log(s"Skipping unchecked for statically verifiable condition $t1 ${this} $t2") + true + } } - def check(tp: Type, bound: List[Symbol]) { - def isLocalBinding(sym: Symbol) = - sym.isAbstractType && - ((bound contains sym) || - sym.name == tpnme.WILDCARD || { + // I tried to use varianceInType to track the variance implications + // but I could not make it work. + case object =:= extends TypeConformance(_ =:= _) + case object <:< extends TypeConformance(_ <:< _) + case object >:> extends TypeConformance((t1, t2) => t2 <:< t1) + case object =!= extends TypeConformance((t1, t2) => false) + + var bound: List[Symbol] = Nil + var warningMessages: List[String] = Nil + + def isLocalBinding(sym: Symbol) = ( + sym.isAbstractType && ( + (bound contains sym) + || (sym.name == tpnme.WILDCARD) + || { val e = context.scope.lookupEntry(sym.name) (e ne null) && e.sym == sym && !e.sym.isTypeParameterOrSkolem && e.owner == context.scope - }) - tp match { - case SingleType(pre, _) => - check(pre, bound) - case TypeRef(pre, sym, args) => - if (sym.isAbstractType) { - // we only use the extractor for top-level type tests, type arguments (see below) remain unchecked - if (!isLocalBinding(sym) && !canRemedy) patternWarning(tp, "abstract type ") - } else if (sym.isAliasType) { - check(tp.normalize, bound) - } else if (sym == NothingClass || sym == NullClass || sym == AnyValClass) { - TypePatternOrIsInstanceTestError(tree, tp) - } else { - for (arg <- args) { - if (sym == ArrayClass) check(arg, bound) - // avoid spurious warnings with higher-kinded types - else if (arg.typeArgs exists (_.typeSymbol.isTypeParameterOrSkolem)) () - // no way to suppress unchecked warnings on try/catch - else if (sym == NonLocalReturnControlClass) () - else arg match { - case TypeRef(_, sym, _) if isLocalBinding(sym) => - ; - case _ => - // Want to warn about type arguments, not type parameters. Otherwise we'll - // see warnings about "invisible" types, like: val List(x0) = x1 leading to "non - // variable type-argument A in type pattern List[A]..." - if (!arg.typeSymbol.isTypeParameterOrSkolem) - patternWarning(arg, "non variable type-argument ") - } - } - } - check(pre, bound) - case RefinedType(parents, decls) => - if (decls.isEmpty) for (p <- parents) check(p, bound) - else patternWarning(tp, "refinement ") - case ExistentialType(quantified, tp1) => - check(tp1, bound ::: quantified) - case ThisType(_) => - () - case NoPrefix => - () - case _ => - patternWarning(tp, "type ") - () + } + ) + ) + def check(tp0: Type, pt: Type, conformance: TypeConformance): Boolean = { + val tp = tp0.normalize + // Set the warning message to be issued when the top-level call fails. + def warn(what: String): Boolean = { + warningMessages ::= what + false + } + def checkArg(param: Symbol, arg: Type) = { + def conforms = ( + if (param.isCovariant) <:< + else if (param.isContravariant) >:> + else =:= + ) + val TypeRef(_, sym, args) = arg + + ( isLocalBinding(sym) + || arg.typeSymbol.isTypeParameterOrSkolem + || (sym.name == tpnme.WILDCARD) // avoid spurious warnings on HK types + || check(arg, param.tpe, conforms) + || warn("non-variable type argument " + arg) + ) } + + // Checking if pt (the expected type of the pattern, and the type + // we are guaranteed) conforms to tp (the type expressed in the pattern's + // type test.) If it does, then even if the type being checked for appears + // to be uncheckable, it is not a warning situation, because it is indeed + // checked: not at runtime, but statically. + conformance.apply(pt, tp) || (tp match { + case SingleType(pre, _) => check(pre, pt, =:=) + case ExistentialType(quantified, tp1) => bound :::= quantified ; check(tp1, pt, <:<) + case ThisType(_) | NoPrefix => true + case RefinedType(parents, decls) if decls.isEmpty => parents forall (p => check(p, pt, <:<)) + case RefinedType(_, _) => warn("refinement " + tp) + case TypeRef(_, ArrayClass, arg :: Nil) => check(arg, NoType, =!=) + case TypeRef(_, NonLocalReturnControlClass, _) => true // no way to suppress unchecked warnings on try/catch + // we only use the extractor for top-level type tests, type arguments remain unchecked + case TypeRef(_, sym, _) if sym.isAbstractType => isLocalBinding(sym) || canRemedy || warn("abstract type " + tp) + case TypeRef(_, _, Nil) => false // leaf node + case TypeRef(pre, sym, args) => forall2(sym.typeParams, args)(checkArg) && check(pre, pt.prefix, =:=) + case _ => warn("type " + tp) + }) + } + typeToTest match { + // Prohibit top-level type tests for these, but they are + // acceptable nested (e.g. case Foldable[Nothing] => ... ) + case TypeRef(_, NothingClass | NullClass | AnyValClass, _) => + TypePatternOrIsInstanceTestError(tree, typeToTest) + case _ => + def where = ( if (inPattern) "pattern " else "" ) + typeToTest + if (check(typeToTest, typeEnsured, =:=)) () + else warningMessages foreach (m => + context.unit.uncheckedWarning(tree.pos, s"$m in type $where is unchecked since it is eliminated by erasure")) } - check(tp, List()) } /** Type intersection of simple type tp1 with general type tp2. @@ -1420,7 +1443,7 @@ trait Infer { return ErrorType } - checkCheckable(tree0, pattp, inPattern = true, canRemedy) + checkCheckable(tree0, pattp, pt, inPattern = true, canRemedy) if (pattp <:< pt) () else { debuglog("free type params (1) = " + tpparams) diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index 8b7c70c048..cf5c7265ad 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -412,8 +412,9 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL **/ // must treat Typed and Bind together -- we need to know the patBinder of the Bind pattern to get at the actual type case MaybeBoundTyped(subPatBinder, pt) => + val next = glb(List(patBinder.info.widen, pt)).normalize // a typed pattern never has any subtrees - noFurtherSubPats(TypeTestTreeMaker(subPatBinder, patBinder, pt, glb(List(patBinder.info.widen, pt)).normalize)(pos)) + noFurtherSubPats(TypeTestTreeMaker(subPatBinder, patBinder, pt, next)(pos)) /** A pattern binder x@p consists of a pattern variable x and a pattern p. The type of the variable x is the static type T of the pattern p. diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 51753baa4f..269ab9611a 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -3672,7 +3672,8 @@ trait Typers extends Modes with Adaptations with Tags { typedClassOf(tree, args.head, true) else { if (!isPastTyper && fun.symbol == Any_isInstanceOf && !targs.isEmpty) - checkCheckable(tree, targs.head, inPattern = false) + checkCheckable(tree, targs.head, AnyClass.tpe, inPattern = false) + val resultpe = restpe.instantiateTypeParams(tparams, targs) //@M substitution in instantiateParams needs to be careful! //@M example: class Foo[a] { def foo[m[x]]: m[a] = error("") } (new Foo[Int]).foo[List] : List[Int] diff --git a/test/files/neg/unchecked.check b/test/files/neg/unchecked.check new file mode 100644 index 0000000000..34a11db1a0 --- /dev/null +++ b/test/files/neg/unchecked.check @@ -0,0 +1,19 @@ +unchecked.scala:18: error: non-variable type argument String in type pattern Iterable[String] is unchecked since it is eliminated by erasure + case xs: Iterable[String] => xs.head // unchecked + ^ +unchecked.scala:22: error: non-variable type argument Any in type pattern Set[Any] is unchecked since it is eliminated by erasure + case xs: Set[Any] => xs.head // unchecked + ^ +unchecked.scala:26: error: non-variable type argument Any in type pattern Map[Any,Any] is unchecked since it is eliminated by erasure + case xs: Map[Any, Any] => xs.head // unchecked + ^ +unchecked.scala:35: error: non-variable type argument List[Nothing] in type pattern Test.Contra[List[Nothing]] is unchecked since it is eliminated by erasure + case xs: Contra[List[Nothing]] => xs.head // unchecked + ^ +unchecked.scala:50: error: non-variable type argument String in type pattern Test.Exp[String] is unchecked since it is eliminated by erasure + case ArrayApply(x: Exp[Array[T]], _, j: Exp[String]) => x // unchecked + ^ +unchecked.scala:55: error: non-variable type argument Array[T] in type pattern Test.Exp[Array[T]] is unchecked since it is eliminated by erasure + case ArrayApply(x: Exp[Array[T]], _, _) => x // unchecked + ^ +6 errors found diff --git a/test/files/neg/unchecked.flags b/test/files/neg/unchecked.flags new file mode 100644 index 0000000000..464cc20ea6 --- /dev/null +++ b/test/files/neg/unchecked.flags @@ -0,0 +1 @@ +-Xfatal-warnings -unchecked
\ No newline at end of file diff --git a/test/files/neg/unchecked.scala b/test/files/neg/unchecked.scala new file mode 100644 index 0000000000..b50cdf9d7a --- /dev/null +++ b/test/files/neg/unchecked.scala @@ -0,0 +1,74 @@ +import language.existentials + +object Test { + class Def[T] + class Exp[T] + class Contra[-T] { def head[T1 <: T] : T1 = ??? } + class Cov[+T] { } + + case class ArrayApply[T](x: Exp[Array[T]], i: Exp[Int], j: Exp[_]) extends Def[T] + + val IntArrayApply = ArrayApply[Int](new Exp[Array[Int]], new Exp[Int], new Exp[Int]) + + def f(x: Any) = x match { + case xs: Iterable[Any] => xs.head // okay + case _ => 0 + } + def f2(x: Any) = x match { + case xs: Iterable[String] => xs.head // unchecked + case _ => 0 + } + def f3(x: Any) = x match { + case xs: Set[Any] => xs.head // unchecked + case _ => 0 + } + def f4(x: Any) = x match { + case xs: Map[Any, Any] => xs.head // unchecked + case _ => 0 + } + + def cf1(x: Any) = x match { + case xs: Contra[Nothing] => xs.head // okay + case _ => 0 + } + def cf2(x: Any) = x match { + case xs: Contra[List[Nothing]] => xs.head // unchecked + case _ => 0 + } + + def co1(x: List[Cov[List[Int]]]) = x match { + case _: Seq[Cov[Seq[Any]]] => true // okay + case _ => false + } + + def g[T](x: Def[T]) = x match { + case ArrayApply(x: Exp[Array[T]], i: Exp[Int], _) => x // okay + case _ => 0 + } + + def g2[T](x: Def[T]) = x match { + case ArrayApply(x: Exp[Array[T]], _, j: Exp[String]) => x // unchecked + case _ => 0 + } + + def g3[T](x: Any) = x match { + case ArrayApply(x: Exp[Array[T]], _, _) => x // unchecked + case _ => 0 + } + + def g4 = IntArrayApply match { + case ArrayApply(x: Exp[Array[Int]], _, _) => x // okay + case _ => () + } + def g5[T](x: ArrayApply[Int]) = x match { + case ArrayApply(x: Exp[Array[Int]], _, _) => x // okay + case _ => 0 + } + + // Nope + // + // def g5 = IntArrayApply match { + // case ArrayApply(x: Exp[Array[String]], _, _) => x // nope + // case _ => () + // } +} diff --git a/test/files/neg/unchecked2.check b/test/files/neg/unchecked2.check index 2c0be9ce00..e37865928e 100644 --- a/test/files/neg/unchecked2.check +++ b/test/files/neg/unchecked2.check @@ -1,19 +1,19 @@ -unchecked2.scala:2: error: non variable type-argument Int in type Option[Int] is unchecked since it is eliminated by erasure +unchecked2.scala:2: error: non-variable type argument Int in type Option[Int] is unchecked since it is eliminated by erasure Some(123).isInstanceOf[Option[Int]] ^ -unchecked2.scala:3: error: non variable type-argument String in type Option[String] is unchecked since it is eliminated by erasure +unchecked2.scala:3: error: non-variable type argument String in type Option[String] is unchecked since it is eliminated by erasure Some(123).isInstanceOf[Option[String]] ^ -unchecked2.scala:4: error: non variable type-argument List[String] in type Option[List[String]] is unchecked since it is eliminated by erasure +unchecked2.scala:4: error: non-variable type argument List[String] in type Option[List[String]] is unchecked since it is eliminated by erasure Some(123).isInstanceOf[Option[List[String]]] ^ -unchecked2.scala:5: error: non variable type-argument List[Int => String] in type Option[List[Int => String]] is unchecked since it is eliminated by erasure +unchecked2.scala:5: error: non-variable type argument List[Int => String] in type Option[List[Int => String]] is unchecked since it is eliminated by erasure Some(123).isInstanceOf[Option[List[Int => String]]] ^ -unchecked2.scala:6: error: non variable type-argument (String, Double) in type Option[(String, Double)] is unchecked since it is eliminated by erasure +unchecked2.scala:6: error: non-variable type argument (String, Double) in type Option[(String, Double)] is unchecked since it is eliminated by erasure Some(123).isInstanceOf[Option[(String, Double)]] ^ -unchecked2.scala:7: error: non variable type-argument String => Double in type Option[String => Double] is unchecked since it is eliminated by erasure +unchecked2.scala:7: error: non-variable type argument String => Double in type Option[String => Double] is unchecked since it is eliminated by erasure Some(123).isInstanceOf[Option[String => Double]] ^ 6 errors found diff --git a/test/files/pos/unchecked-a.flags b/test/files/pos/unchecked-a.flags new file mode 100644 index 0000000000..779916d58f --- /dev/null +++ b/test/files/pos/unchecked-a.flags @@ -0,0 +1 @@ +-unchecked -Xfatal-warnings
\ No newline at end of file diff --git a/test/files/pos/unchecked-a.scala b/test/files/pos/unchecked-a.scala new file mode 100644 index 0000000000..deceb91c36 --- /dev/null +++ b/test/files/pos/unchecked-a.scala @@ -0,0 +1,15 @@ +trait Y +trait Z extends Y +class X[+A <: Y] + +object Test { + def f1(x: X[_ <: Y]) = x match { + case _: X[Any] => // looks a little funny; `Any` is outside the bounds for `A` + } + def f2(x: X[_ <: Y]) = x match { + case _: X[Y] => // looks better, let's allow this (too) + } + + // NonLocalReturnControl[_] warnings + def foo: Int = List(0).foldLeft(0){case _ => return 0} +} |