From 4ce3d6a2d4cd8aa582ca76d42584900c3639e0f5 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 19 Dec 2016 15:22:32 +0100 Subject: [PATCH 01/11] Fix #1828: add warning when patternmatching on generics --- .../dotc/transform/IsInstanceOfEvaluator.scala | 7 +++++-- tests/repl/errmsgs.check | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index ebd2ae43634a..8463f8797ab3 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -148,8 +148,11 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val inMatch = s.qualifier.symbol is Case - if (valueClassesOrAny) tree - else if (knownStatically) + if (valueClassesOrAny) { + if (selector eq defn.ObjectType) + ctx.warning(i"abstract type pattern is unchecked since it is eliminated by erasure", tree.pos) + tree + } else if (knownStatically) handleStaticallyKnown(s, scrutinee, selector, inMatch, tree.pos) else if (falseIfUnrelated && scrutinee <:< selector) // scrutinee is a subtype of the selector, safe to rewrite diff --git a/tests/repl/errmsgs.check b/tests/repl/errmsgs.check index 4e89a16a5d24..6d4011660ea4 100644 --- a/tests/repl/errmsgs.check +++ b/tests/repl/errmsgs.check @@ -92,4 +92,19 @@ scala> { def f: Int = g; val x: Int = 1; def g: Int = 5; } | `g` is a forward reference extending over the definition of `x` longer explanation available when compiling with `-explain` +scala> def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } +-- Warning: ---------------------------------------------------------- +4 |def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } + | ^ + | abstract type pattern is unchecked since it is eliminated by erasure +def unsafeCast[S](a: Any): [S] => (a: Any)S +scala> unsafeCast[String](1) +java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String + at .(:6) + at .() + at RequestResult$.(:3) + at RequestResult$.() + at RequestResult$result() + at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) + at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav... scala> :quit From df6f4783fd8a5dea214d4a518f1c303a917e904c Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 19 Dec 2016 15:44:16 +0100 Subject: [PATCH 02/11] Make abstract pattern error into an error message --- .../tools/dotc/reporting/diagnostic/ErrorMessageID.java | 1 + .../dotty/tools/dotc/reporting/diagnostic/messages.scala | 8 ++++++++ .../tools/dotc/transform/IsInstanceOfEvaluator.scala | 5 +++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java index 6ea168b99241..82eddb9fd8c6 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java @@ -56,6 +56,7 @@ public enum ErrorMessageID { CyclicReferenceInvolvingID, CyclicReferenceInvolvingImplicitID, SuperQualMustBeParentID, + ErasedTypeID, ; public int errorNumber() { diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala index 87837fd82c0b..cba33923dbe5 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala @@ -1221,4 +1221,12 @@ object messages { |Attempting to define a field in a method signature after a varargs field is an error. |""".stripMargin } + + case class ErasedType()(implicit ctx: Context) + extends Message(ErasedTypeID) { + val kind = "Erased Type" + val msg = + i"abstract type pattern is unchecked since it is eliminated by erasure" + val explanation = "" + } } diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index 8463f8797ab3..9613213df59a 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -6,6 +6,7 @@ import TreeTransforms.{MiniPhaseTransform, TransformerInfo} import core._ import Contexts.Context, Types._, Constants._, Decorators._, Symbols._ import TypeUtils._, TypeErasure._, Flags._ +import reporting.diagnostic.messages._ /** Implements partial evaluation of `sc.isInstanceOf[Sel]` according to: * @@ -149,8 +150,8 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val inMatch = s.qualifier.symbol is Case if (valueClassesOrAny) { - if (selector eq defn.ObjectType) - ctx.warning(i"abstract type pattern is unchecked since it is eliminated by erasure", tree.pos) + if ((selector eq defn.ObjectType)) + ctx.uncheckedWarning(ErasedType(), tree.pos) tree } else if (knownStatically) handleStaticallyKnown(s, scrutinee, selector, inMatch, tree.pos) From be9f73772705c0b317b758c627c5a013b6c5c71b Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 19 Dec 2016 15:44:41 +0100 Subject: [PATCH 03/11] Make sure REPL tests run with `-unchecked` flag --- compiler/test/dotty/tools/dotc/repl/TestREPL.scala | 3 ++- tests/repl/errmsgs.check | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/test/dotty/tools/dotc/repl/TestREPL.scala b/compiler/test/dotty/tools/dotc/repl/TestREPL.scala index c5557b86e985..2a3cfa568a82 100644 --- a/compiler/test/dotty/tools/dotc/repl/TestREPL.scala +++ b/compiler/test/dotty/tools/dotc/repl/TestREPL.scala @@ -10,7 +10,7 @@ import org.junit.Test /** A subclass of REPL used for testing. * It takes a transcript of a REPL session in `script`. The transcript - * starts with the first input prompt `scala> ` and ends with `scala> :quit` and a newline. + * starts with the first input prompt `scala> ` and ends with `scala> :quit`. * Invoking `process()` on the `TestREPL` runs all input lines and * collects then interleaved with REPL output in a string writer `out`. * Invoking `check()` checks that the collected output matches the original @@ -26,6 +26,7 @@ class TestREPL(script: String) extends REPL { override def context(ctx: Context) = { val fresh = ctx.fresh fresh.setSetting(ctx.settings.color, "never") + fresh.setSetting(ctx.settings.unchecked, true) fresh.setSetting(ctx.settings.classpath, Jars.dottyReplDeps.mkString(":")) fresh.initialize()(fresh) fresh diff --git a/tests/repl/errmsgs.check b/tests/repl/errmsgs.check index 6d4011660ea4..d2251d6c9c01 100644 --- a/tests/repl/errmsgs.check +++ b/tests/repl/errmsgs.check @@ -93,7 +93,7 @@ scala> { def f: Int = g; val x: Int = 1; def g: Int = 5; } longer explanation available when compiling with `-explain` scala> def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } --- Warning: ---------------------------------------------------------- +-- [E034] Erased Type Unchecked Warning: ----------------------------- 4 |def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure From 539ea0c784c61b19ab361e694df19886d11d70e5 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 19 Dec 2016 16:52:01 +0100 Subject: [PATCH 04/11] Cover cases where other generics are erased --- .../dotc/reporting/diagnostic/messages.scala | 1 - .../transform/IsInstanceOfEvaluator.scala | 28 +++++++++++++++---- tests/repl/errmsgs.check | 11 ++++++++ 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala index cba33923dbe5..b8a72dd1c6bf 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala @@ -1227,6 +1227,5 @@ object messages { val kind = "Erased Type" val msg = i"abstract type pattern is unchecked since it is eliminated by erasure" - val explanation = "" } } diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index 9613213df59a..39e1a1edf8d8 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -5,7 +5,7 @@ import dotty.tools.dotc.util.Positions._ import TreeTransforms.{MiniPhaseTransform, TransformerInfo} import core._ import Contexts.Context, Types._, Constants._, Decorators._, Symbols._ -import TypeUtils._, TypeErasure._, Flags._ +import TypeUtils._, TypeErasure._, Flags._, TypeApplications._ import reporting.diagnostic.messages._ /** Implements partial evaluation of `sc.isInstanceOf[Sel]` according to: @@ -128,6 +128,25 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val selClassNonFinal = selClass && !(selector.typeSymbol is Final) val selFinalClass = selClass && (selector.typeSymbol is Final) + val selTypeParam = tree.args.head.tpe.widen match { + case AppliedType(tycon, args) => + ctx.uncheckedWarning( + ErasedType(hl"""|Since type parameters are erased, you should not match on them in + |${"match"} expressions."""), + tree.pos + ) + true + case x if tree.args.head.symbol is TypeParam => + ctx.uncheckedWarning( + ErasedType( + hl"""|`${tree.args.head.tpe}` will be erased to `${selector}`. Which means that the specified + |behavior could be different during runtime.""" + ), + tree.pos + ) + true + case _ => false + } // Cases --------------------------------- val valueClassesOrAny = @@ -149,11 +168,8 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val inMatch = s.qualifier.symbol is Case - if (valueClassesOrAny) { - if ((selector eq defn.ObjectType)) - ctx.uncheckedWarning(ErasedType(), tree.pos) - tree - } else if (knownStatically) + if (selTypeParam || valueClassesOrAny) tree + else if (knownStatically) handleStaticallyKnown(s, scrutinee, selector, inMatch, tree.pos) else if (falseIfUnrelated && scrutinee <:< selector) // scrutinee is a subtype of the selector, safe to rewrite diff --git a/tests/repl/errmsgs.check b/tests/repl/errmsgs.check index d2251d6c9c01..737c2a316542 100644 --- a/tests/repl/errmsgs.check +++ b/tests/repl/errmsgs.check @@ -97,6 +97,8 @@ scala> def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } 4 |def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure + +longer explanation available when compiling with `-explain` def unsafeCast[S](a: Any): [S] => (a: Any)S scala> unsafeCast[String](1) java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String @@ -107,4 +109,13 @@ java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.Stri at RequestResult$result() at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav... +scala> class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } +-- [E034] Erased Type Unchecked Warning: ----------------------------- +4 |class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } + | ^ + | abstract type pattern is unchecked since it is eliminated by erasure + +longer explanation available when compiling with `-explain` +defined class A +def unsafeCast2[S <: A](a: Any): [S <: A] => (a: Any)S scala> :quit From ec21ea79985772b534ec5d2375407513c315db1f Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Wed, 21 Dec 2016 14:59:15 +0100 Subject: [PATCH 05/11] Don't warn for erasure on arrays with value types --- .../transform/IsInstanceOfEvaluator.scala | 10 +++- tests/repl/erasure.check | 53 +++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 tests/repl/erasure.check diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index 39e1a1edf8d8..82acbb8e6541 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -129,8 +129,14 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val selClassNonFinal = selClass && !(selector.typeSymbol is Final) val selFinalClass = selClass && (selector.typeSymbol is Final) val selTypeParam = tree.args.head.tpe.widen match { - case AppliedType(tycon, args) => - ctx.uncheckedWarning( + case tp @ AppliedType(tycon, args) => + // If the type is Array[X] where x extends AnyVal, this shouldn't yield a warning: + val illegalComparison = !(tp.isRef(defn.ArrayClass) && { + args.head.derivesFrom(defn.AnyValClass) || + args.head.isRef(defn.AnyClass) + }) + + if (illegalComparison) ctx.uncheckedWarning( ErasedType(hl"""|Since type parameters are erased, you should not match on them in |${"match"} expressions."""), tree.pos diff --git a/tests/repl/erasure.check b/tests/repl/erasure.check new file mode 100644 index 000000000000..b4371236bb0a --- /dev/null +++ b/tests/repl/erasure.check @@ -0,0 +1,53 @@ +scala> def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } +-- [E035] Erased Type Unchecked Warning: ----------------------------- +4 |def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } + | ^ + | abstract type pattern is unchecked since it is eliminated by erasure + +longer explanation available when compiling with `-explain` +def unsafeCast[S](a: Any): [S] => (a: Any)S +scala> unsafeCast[String](1) +java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String + at .(:6) + at .() + at RequestResult$.(:3) + at RequestResult$.() + at RequestResult$result() + at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) + at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav... +scala> class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } +-- [E035] Erased Type Unchecked Warning: ----------------------------- +4 |class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } + | ^ + | abstract type pattern is unchecked since it is eliminated by erasure + +longer explanation available when compiling with `-explain` +defined class A +def unsafeCast2[S <: A](a: Any): [S <: A] => (a: Any)S +scala> def matchArray1[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } +def matchArray1[A](xs: Array[A]): [A] => (xs: Array[A])Array[Int] +scala> def matchArray2[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } +-- [E035] Erased Type Unchecked Warning: ----------------------------- +5 |def matchArray2[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } + | ^ + | abstract type pattern is unchecked since it is eliminated by erasure + +longer explanation available when compiling with `-explain` +def matchArray2[A](xs: Array[Any]): [A] => (xs: Array[Any])Array[Int] +scala> def matchArray3[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[AnyRef] => ???; case xs: Array[Any] => ??? } +-- [E035] Erased Type Unchecked Warning: ----------------------------- +5 |def matchArray3[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[AnyRef] => ???; case xs: Array[Any] => ??? } + | ^ + | abstract type pattern is unchecked since it is eliminated by erasure + +longer explanation available when compiling with `-explain` +def matchArray3[A](xs: Array[A]): [A] => (xs: Array[A])Array[Int] +scala> def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ???; case xs: Array[Any] => ??? } +-- [E035] Erased Type Unchecked Warning: ----------------------------- +5 |def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ???; case xs: Array[Any] => ??? } + | ^ + | abstract type pattern is unchecked since it is eliminated by erasure + +longer explanation available when compiling with `-explain` +def matchArray4(xs: Array[Any]): Array[Int] +scala> :quit From 90ceb3474432a19c24594bd964b50a20c243c899 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Wed, 21 Dec 2016 15:06:47 +0100 Subject: [PATCH 06/11] Add comments to Phase explaining warnings --- .../dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index 82acbb8e6541..ca266ed79fc0 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -19,6 +19,11 @@ import reporting.diagnostic.messages._ * This is a generalized solution to raising an error on unreachable match * cases and warnings on other statically known results of `isInstanceOf`. * + * This phase also warns if the erased type parameter of a parameterized type + * is used in a match where it would be erased to `Object` or if the + * typeparameters are removed. Both of these cases could cause surprising + * behavior for the users. + * * Steps taken: * * 1. `evalTypeApply` will establish the matrix and choose the appropriate @@ -128,6 +133,8 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val selClassNonFinal = selClass && !(selector.typeSymbol is Final) val selFinalClass = selClass && (selector.typeSymbol is Final) + + /** Check if the selector's potential type parameters will be erased, and if so warn */ val selTypeParam = tree.args.head.tpe.widen match { case tp @ AppliedType(tycon, args) => // If the type is Array[X] where x extends AnyVal, this shouldn't yield a warning: From 980638fe3b706c047278cc54e23fb01f297810f4 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Thu, 22 Dec 2016 11:50:25 +0100 Subject: [PATCH 07/11] Make sure `IsInstanceOfEvaluator` respects `@unchecked` --- .../transform/IsInstanceOfEvaluator.scala | 40 ++++++++++--------- tests/repl/erasure.check | 6 +++ 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index ca266ed79fc0..70485a6bbc4d 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -135,30 +135,32 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val selFinalClass = selClass && (selector.typeSymbol is Final) /** Check if the selector's potential type parameters will be erased, and if so warn */ - val selTypeParam = tree.args.head.tpe.widen match { - case tp @ AppliedType(tycon, args) => - // If the type is Array[X] where x extends AnyVal, this shouldn't yield a warning: - val illegalComparison = !(tp.isRef(defn.ArrayClass) && { - args.head.derivesFrom(defn.AnyValClass) || - args.head.isRef(defn.AnyClass) - }) - - if (illegalComparison) ctx.uncheckedWarning( + val selTypeParam = tree.args.head.tpe.widen match { + case tp @ AppliedType(_, arg :: _) => + // If the type is `Array[X]` where `X` extends AnyVal or `X =:= + // Any`, this shouldn't yield a warning: + val isArray = tp.isRef(defn.ArrayClass) + val unerased = arg.derivesFrom(defn.AnyValClass) || arg.isRef(defn.AnyClass) + val hasAnnot = arg.hasAnnotation(defn.UncheckedAnnot) + + if (!hasAnnot && !(isArray && unerased)) ctx.uncheckedWarning( ErasedType(hl"""|Since type parameters are erased, you should not match on them in |${"match"} expressions."""), tree.pos ) true - case x if tree.args.head.symbol is TypeParam => - ctx.uncheckedWarning( - ErasedType( - hl"""|`${tree.args.head.tpe}` will be erased to `${selector}`. Which means that the specified - |behavior could be different during runtime.""" - ), - tree.pos - ) - true - case _ => false + case _ => + if (tree.args.head.symbol.is(TypeParam)) { + ctx.uncheckedWarning( + ErasedType( + hl"""|`${tree.args.head.tpe}` will be erased to `${selector}`. Which means that the specified + |behavior could be different during runtime.""" + ), + tree.pos + ) + true + } + else false } // Cases --------------------------------- diff --git a/tests/repl/erasure.check b/tests/repl/erasure.check index b4371236bb0a..d30a36d06eb6 100644 --- a/tests/repl/erasure.check +++ b/tests/repl/erasure.check @@ -50,4 +50,10 @@ scala> def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; c longer explanation available when compiling with `-explain` def matchArray4(xs: Array[Any]): Array[Int] +scala> def matchArray5[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A @unchecked] => ??? } +def matchArray5[A](xs: Array[Any]): [A] => (xs: Array[Any])Array[Int] +scala> def matchList1(xs: List[Any]) = xs match { case xs: List[Int @unchecked] => ??? } +def matchList1(xs: List[Any]): Nothing +scala> def matchList2(xs: List[Any]) = xs match { case List() => ???; case _ => ??? } +def matchList2(xs: List[Any]): Nothing scala> :quit From b0d3aa204f412a944dee9477fa82da87fec44bde Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Fri, 23 Dec 2016 12:43:15 +0100 Subject: [PATCH 08/11] Fix matching on top types --- .../dotc/reporting/diagnostic/messages.scala | 2 +- .../dotc/transform/IsInstanceOfEvaluator.scala | 15 ++++++++------- tests/repl/erasure.check | 16 ++++++---------- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala index b8a72dd1c6bf..e85d6e692e01 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala @@ -1222,7 +1222,7 @@ object messages { |""".stripMargin } - case class ErasedType()(implicit ctx: Context) + case class ErasedType(val explanation: String = "")(implicit ctx: Context) extends Message(ErasedTypeID) { val kind = "Erased Type" val msg = diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index 70485a6bbc4d..746992832357 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -137,13 +137,14 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => /** Check if the selector's potential type parameters will be erased, and if so warn */ val selTypeParam = tree.args.head.tpe.widen match { case tp @ AppliedType(_, arg :: _) => - // If the type is `Array[X]` where `X` extends AnyVal or `X =:= - // Any`, this shouldn't yield a warning: - val isArray = tp.isRef(defn.ArrayClass) - val unerased = arg.derivesFrom(defn.AnyValClass) || arg.isRef(defn.AnyClass) - val hasAnnot = arg.hasAnnotation(defn.UncheckedAnnot) - - if (!hasAnnot && !(isArray && unerased)) ctx.uncheckedWarning( + // If the type is `Array[X]` where `X` extends AnyVal + val anyValArray = tp.isRef(defn.ArrayClass) && arg.derivesFrom(defn.AnyValClass) + // param is: Any | AnyRef | java.lang.Object + val topType = defn.ObjectType <:< arg + // has @unchecked annotation to suppress warnings + val hasUncheckedAnnot = arg.hasAnnotation(defn.UncheckedAnnot) + + if (!topType && !hasUncheckedAnnot && !anyValArray) ctx.uncheckedWarning( ErasedType(hl"""|Since type parameters are erased, you should not match on them in |${"match"} expressions."""), tree.pos diff --git a/tests/repl/erasure.check b/tests/repl/erasure.check index d30a36d06eb6..37f2de608a29 100644 --- a/tests/repl/erasure.check +++ b/tests/repl/erasure.check @@ -1,5 +1,5 @@ scala> def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } --- [E035] Erased Type Unchecked Warning: ----------------------------- +-- [E037] Erased Type Unchecked Warning: ----------------------------- 4 |def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure @@ -16,7 +16,7 @@ java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.Stri at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav... scala> class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } --- [E035] Erased Type Unchecked Warning: ----------------------------- +-- [E037] Erased Type Unchecked Warning: ----------------------------- 4 |class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure @@ -27,7 +27,7 @@ def unsafeCast2[S <: A](a: Any): [S <: A] => (a: Any)S scala> def matchArray1[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } def matchArray1[A](xs: Array[A]): [A] => (xs: Array[A])Array[Int] scala> def matchArray2[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } --- [E035] Erased Type Unchecked Warning: ----------------------------- +-- [E037] Erased Type Unchecked Warning: ----------------------------- 5 |def matchArray2[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure @@ -35,15 +35,9 @@ scala> def matchArray2[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs longer explanation available when compiling with `-explain` def matchArray2[A](xs: Array[Any]): [A] => (xs: Array[Any])Array[Int] scala> def matchArray3[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[AnyRef] => ???; case xs: Array[Any] => ??? } --- [E035] Erased Type Unchecked Warning: ----------------------------- -5 |def matchArray3[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[AnyRef] => ???; case xs: Array[Any] => ??? } - | ^ - | abstract type pattern is unchecked since it is eliminated by erasure - -longer explanation available when compiling with `-explain` def matchArray3[A](xs: Array[A]): [A] => (xs: Array[A])Array[Int] scala> def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ???; case xs: Array[Any] => ??? } --- [E035] Erased Type Unchecked Warning: ----------------------------- +-- [E037] Erased Type Unchecked Warning: ----------------------------- 5 |def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ???; case xs: Array[Any] => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure @@ -56,4 +50,6 @@ scala> def matchList1(xs: List[Any]) = xs match { case xs: List[Int @unchecked] def matchList1(xs: List[Any]): Nothing scala> def matchList2(xs: List[Any]) = xs match { case List() => ???; case _ => ??? } def matchList2(xs: List[Any]): Nothing +scala> def matchList3(xs: Seq[_]) = xs match { case List() => ???; case _ => ??? } +def matchList3(xs: Seq[_]): Nothing scala> :quit From 1f1b1bac79cee374c9993e9e3ff5060c73a1ca0d Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 30 Jan 2017 11:06:04 +0100 Subject: [PATCH 09/11] Constrain `anyValArray` check to primitives --- .../transform/IsInstanceOfEvaluator.scala | 8 +++--- tests/repl/erasure.check | 20 +++++++------- tests/repl/errmsgs.check | 26 ------------------- 3 files changed, 15 insertions(+), 39 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index 746992832357..81c4f1be509c 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -134,11 +134,13 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => val selClassNonFinal = selClass && !(selector.typeSymbol is Final) val selFinalClass = selClass && (selector.typeSymbol is Final) - /** Check if the selector's potential type parameters will be erased, and if so warn */ + // Check if the selector's potential type parameters will be erased, and if so warn val selTypeParam = tree.args.head.tpe.widen match { case tp @ AppliedType(_, arg :: _) => - // If the type is `Array[X]` where `X` extends AnyVal - val anyValArray = tp.isRef(defn.ArrayClass) && arg.derivesFrom(defn.AnyValClass) + // If the type is `Array[X]` where `X` is a primitive value + // class. In the future, when we have a solid implementation of + // Arrays of value classes, we might be able to relax this check. + val anyValArray = tp.isRef(defn.ArrayClass) && arg.typeSymbol.isPrimitiveValueClass // param is: Any | AnyRef | java.lang.Object val topType = defn.ObjectType <:< arg // has @unchecked annotation to suppress warnings diff --git a/tests/repl/erasure.check b/tests/repl/erasure.check index 37f2de608a29..c637288e908c 100644 --- a/tests/repl/erasure.check +++ b/tests/repl/erasure.check @@ -1,11 +1,11 @@ scala> def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } --- [E037] Erased Type Unchecked Warning: ----------------------------- +-- [E048] Erased Type Unchecked Warning: ----------------------------- 4 |def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure longer explanation available when compiling with `-explain` -def unsafeCast[S](a: Any): [S] => (a: Any)S +def unsafeCast[S](a: Any): [S](a: Any)S scala> unsafeCast[String](1) java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String at .(:6) @@ -16,28 +16,28 @@ java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.Stri at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav... scala> class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } --- [E037] Erased Type Unchecked Warning: ----------------------------- +-- [E048] Erased Type Unchecked Warning: ----------------------------- 4 |class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure longer explanation available when compiling with `-explain` defined class A -def unsafeCast2[S <: A](a: Any): [S <: A] => (a: Any)S +def unsafeCast2[S <: A](a: Any): [S <: A](a: Any)S scala> def matchArray1[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } -def matchArray1[A](xs: Array[A]): [A] => (xs: Array[A])Array[Int] +def matchArray1[A](xs: Array[A]): [A](xs: Array[A])Array[Int] scala> def matchArray2[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } --- [E037] Erased Type Unchecked Warning: ----------------------------- +-- [E048] Erased Type Unchecked Warning: ----------------------------- 5 |def matchArray2[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure longer explanation available when compiling with `-explain` -def matchArray2[A](xs: Array[Any]): [A] => (xs: Array[Any])Array[Int] +def matchArray2[A](xs: Array[Any]): [A](xs: Array[Any])Array[Int] scala> def matchArray3[A](xs: Array[A]) = xs match { case xs: Array[Int] => xs; case xs: Array[AnyRef] => ???; case xs: Array[Any] => ??? } -def matchArray3[A](xs: Array[A]): [A] => (xs: Array[A])Array[Int] +def matchArray3[A](xs: Array[A]): [A](xs: Array[A])Array[Int] scala> def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ???; case xs: Array[Any] => ??? } --- [E037] Erased Type Unchecked Warning: ----------------------------- +-- [E048] Erased Type Unchecked Warning: ----------------------------- 5 |def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A] => ???; case xs: Array[Any] => ??? } | ^ | abstract type pattern is unchecked since it is eliminated by erasure @@ -45,7 +45,7 @@ scala> def matchArray4(xs: Array[Any]) = xs match { case xs: Array[Int] => xs; c longer explanation available when compiling with `-explain` def matchArray4(xs: Array[Any]): Array[Int] scala> def matchArray5[A](xs: Array[Any]) = xs match { case xs: Array[Int] => xs; case xs: Array[A @unchecked] => ??? } -def matchArray5[A](xs: Array[Any]): [A] => (xs: Array[Any])Array[Int] +def matchArray5[A](xs: Array[Any]): [A](xs: Array[Any])Array[Int] scala> def matchList1(xs: List[Any]) = xs match { case xs: List[Int @unchecked] => ??? } def matchList1(xs: List[Any]): Nothing scala> def matchList2(xs: List[Any]) = xs match { case List() => ???; case _ => ??? } diff --git a/tests/repl/errmsgs.check b/tests/repl/errmsgs.check index 737c2a316542..4e89a16a5d24 100644 --- a/tests/repl/errmsgs.check +++ b/tests/repl/errmsgs.check @@ -92,30 +92,4 @@ scala> { def f: Int = g; val x: Int = 1; def g: Int = 5; } | `g` is a forward reference extending over the definition of `x` longer explanation available when compiling with `-explain` -scala> def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } --- [E034] Erased Type Unchecked Warning: ----------------------------- -4 |def unsafeCast[S](a: Any) = a match { case s: S => s; case _ => ??? } - | ^ - | abstract type pattern is unchecked since it is eliminated by erasure - -longer explanation available when compiling with `-explain` -def unsafeCast[S](a: Any): [S] => (a: Any)S -scala> unsafeCast[String](1) -java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String - at .(:6) - at .() - at RequestResult$.(:3) - at RequestResult$.() - at RequestResult$result() - at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) - at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav... -scala> class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } --- [E034] Erased Type Unchecked Warning: ----------------------------- -4 |class A; def unsafeCast2[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } - | ^ - | abstract type pattern is unchecked since it is eliminated by erasure - -longer explanation available when compiling with `-explain` -defined class A -def unsafeCast2[S <: A](a: Any): [S <: A] => (a: Any)S scala> :quit From 1a8af58e5086bfefbe465b98c7df39c08010abd9 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Fri, 7 Apr 2017 11:45:08 +0200 Subject: [PATCH 10/11] Relax warnings when matching from `Seq` to `List` --- .../dotty/tools/dotc/core/Definitions.scala | 3 +++ .../transform/IsInstanceOfEvaluator.scala | 26 +++++++++++++++---- tests/repl/erasure.check | 4 +++ 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index b70fcb093ff3..467e1f27ffec 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -357,6 +357,9 @@ class Definitions { List(AnyClass.typeRef), EmptyScope) def SingletonType = SingletonClass.typeRef + lazy val ListType: TypeRef = ctx.requiredClassRef("scala.collection.immutable.List") + def ListClass(implicit ctx: Context) = ListType.symbol.asClass + lazy val SeqType: TypeRef = ctx.requiredClassRef("scala.collection.Seq") def SeqClass(implicit ctx: Context) = SeqType.symbol.asClass diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index 81c4f1be509c..e70e39ba22a4 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -146,11 +146,27 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => // has @unchecked annotation to suppress warnings val hasUncheckedAnnot = arg.hasAnnotation(defn.UncheckedAnnot) - if (!topType && !hasUncheckedAnnot && !anyValArray) ctx.uncheckedWarning( - ErasedType(hl"""|Since type parameters are erased, you should not match on them in - |${"match"} expressions."""), - tree.pos - ) + // we don't want to warn when matching on `List` from `Seq` e.g: + // (xs: Seq[Int]) match { case xs: List[Int] => ??? } + val matchingSeqToList = { + val hasSameTypeArgs = s.qualifier.tpe.widen match { + case AppliedType(_, scrutArg :: Nil) => + (scrutArg eq arg) || arg <:< scrutArg + case _ => false + } + + scrutinee.isRef(defn.SeqClass) && + tp.isRef(defn.ListClass) && + hasSameTypeArgs + } + + if (!topType && !hasUncheckedAnnot && !matchingSeqToList && !anyValArray) { + ctx.uncheckedWarning( + ErasedType(hl"""|Since type parameters are erased, you should not match on them in + |${"match"} expressions."""), + tree.pos + ) + } true case _ => if (tree.args.head.symbol.is(TypeParam)) { diff --git a/tests/repl/erasure.check b/tests/repl/erasure.check index c637288e908c..9e78fbca1d35 100644 --- a/tests/repl/erasure.check +++ b/tests/repl/erasure.check @@ -52,4 +52,8 @@ scala> def matchList2(xs: List[Any]) = xs match { case List() => ???; case _ => def matchList2(xs: List[Any]): Nothing scala> def matchList3(xs: Seq[_]) = xs match { case List() => ???; case _ => ??? } def matchList3(xs: Seq[_]): Nothing +scala> val xs: Seq[Int] = Seq(1,2,3) +val xs: scala.collection.Seq[Int] = List(1, 2, 3) +scala> val xsMatch = xs match { case ss: List[Int] => 1 } +val xsMatch: Int = 1 scala> :quit From 237cd346b7f6546e8cc819c0e1b94e132b47e4e7 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Fri, 7 Apr 2017 12:39:14 +0200 Subject: [PATCH 11/11] Relax unchecked for underscore and type bindings --- .../transform/IsInstanceOfEvaluator.scala | 29 ++++++++++++++----- tests/repl/erasure.check | 5 ++++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala index e70e39ba22a4..d8733fd1cba3 100644 --- a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala +++ b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala @@ -136,7 +136,7 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => // Check if the selector's potential type parameters will be erased, and if so warn val selTypeParam = tree.args.head.tpe.widen match { - case tp @ AppliedType(_, arg :: _) => + case tp @ AppliedType(_, args @ (arg :: _)) => // If the type is `Array[X]` where `X` is a primitive value // class. In the future, when we have a solid implementation of // Arrays of value classes, we might be able to relax this check. @@ -146,6 +146,16 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => // has @unchecked annotation to suppress warnings val hasUncheckedAnnot = arg.hasAnnotation(defn.UncheckedAnnot) + // Shouldn't warn when matching on a subclass with underscore + // params or type binding + val matchingUnderscoresOrTypeBindings = args.forall(_ match { + case tr: TypeRef => + tr.symbol.is(BindDefinedType) + case TypeBounds(lo, hi) => + (lo eq defn.NothingType) && (hi eq defn.AnyType) + case _ => false + }) && selector <:< scrutinee + // we don't want to warn when matching on `List` from `Seq` e.g: // (xs: Seq[Int]) match { case xs: List[Int] => ??? } val matchingSeqToList = { @@ -160,13 +170,16 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer => hasSameTypeArgs } - if (!topType && !hasUncheckedAnnot && !matchingSeqToList && !anyValArray) { - ctx.uncheckedWarning( - ErasedType(hl"""|Since type parameters are erased, you should not match on them in - |${"match"} expressions."""), - tree.pos - ) - } + val shouldWarn = + !topType && !hasUncheckedAnnot && + !matchingUnderscoresOrTypeBindings && !matchingSeqToList && + !anyValArray + + if (shouldWarn) ctx.uncheckedWarning( + ErasedType(hl"""|Since type parameters are erased, you should not match on them in + |${"match"} expressions."""), + tree.pos + ) true case _ => if (tree.args.head.symbol.is(TypeParam)) { diff --git a/tests/repl/erasure.check b/tests/repl/erasure.check index 9e78fbca1d35..828aec884e25 100644 --- a/tests/repl/erasure.check +++ b/tests/repl/erasure.check @@ -56,4 +56,9 @@ scala> val xs: Seq[Int] = Seq(1,2,3) val xs: scala.collection.Seq[Int] = List(1, 2, 3) scala> val xsMatch = xs match { case ss: List[Int] => 1 } val xsMatch: Int = 1 +scala> trait Foo[X]; trait Bar[X,Y] +defined trait Foo +defined trait Bar +scala> val underScoreMatch = (Nil: Any) match { case _: Foo[_] => ???; case _: Bar[_,_] => ???; case _ => 0 } +val underScoreMatch: Int = 0 scala> :quit