Fix #1828: add warning when patternmatching on generics #1834

felixmulder · 2016-12-19T14:23:18Z

smarter · 2016-12-19T14:25:18Z

compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala

-          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)


This should be a Message class

@unchecked should turn the warning off

smarter · 2016-12-19T14:27:31Z

compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala

-          if (valueClassesOrAny) tree
-          else if (knownStatically)
+          if (valueClassesOrAny) {
+            if (selector eq defn.ObjectType)


Why is this limited to the selector being Object?

Check the definition of valueClassesOrAny. If the selector is also Object here, we are dealing with a generic type param that's been erased.

We should also check to see that it was not originally Object either.

Does that also work with class A; def unsafeCast[S <: A](a: Any) = a match { case s: S => s; case _ => ??? } ? In that case S will be erased to A, not Object.

smarter · 2016-12-19T14:40:32Z

We also need warnings when pattern matching on something that will be erased like List[Int]

felixmulder · 2016-12-19T17:31:18Z

@smarter: fixed your suggestions

odersky · 2016-12-21T07:49:18Z

compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala

+          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


There's one exception to this, and that's arrays.

case xs: Array[Int]

is testable. We should (a) check that it is indeed tested and (b) adapt the error condition.

Good catch! I'll fix this ASAP :)

retronym · 2016-12-21T08:55:53Z

A nuance of the Scalac implementation of this is that it allows you to match on List[Int] without warning if the expected type (ie, the scrutinee type) is Seq[Int].

There are a number of bugs that I've got half fixed on a branch, which will at least have some good test cases for you. I'll dig that up when I'm not on my phone.

felixmulder · 2016-12-21T14:18:38Z

@retronym - thanks, that will surely come in handy!

With the nuanced implementation isn't it possible for the user to define a subtype of Seq[Int] upcast it and then match on List[Int]?

i.e:

class Foo extends Seq[Int] { ... }

(new Foo: Seq[Int]) match {
  case xs: List[Int] => ... // no warning
}

retronym · 2016-12-21T22:26:55Z

@felixmulder The rationale is that either if the erased instanceof check succeeds, List[Int] is the correct type. Basically, it is a shorthand for saying that the folllowing cast can be statically proven to be sound.

def (x: Seq[Int]) = x match {
  case xs: List[_] is xs.length > 0 => ... x.asInstanceOf[List[Int]].head + 1
}

My WIP to better test and implement our unchecked warnings in in scala/scala#4366

retronym · 2016-12-21T22:28:08Z

One more thing: we can also silence the warning with _: List[Int @unchecked]

felixmulder · 2016-12-23T12:03:40Z

@odersky - I think this is ready to go in once there's been a final review :)

odersky · 2017-01-29T07:14:05Z

The current implementation gives a false warning for

val ss: Seq[Int] = ???
ss match {
  case ss: List[Int] => ???
}

We get:

/Users/odersky/workspace/dotty/tests/neg> dotc match.scala -unchecked -explain
-- [E035] Erased Type Unchecked Warning: match.scala ---------------------------
5 |    case ss: List[Int] => ???
  |                       ^
  |      abstract type pattern is unchecked since it is eliminated by erasure

Explanation
===========
Since type parameters are erased, you should not match on them in match expressions.

This will not be easy to fix with the chosen approach. But it is essential that it is fixed. Otherwise, signalling false negatives will simply mean that people don't play attention to the real problems.

odersky · 2017-01-29T07:18:28Z

One possible fix might be to refine the translation of the patterm matcher. Here, we currently see:

val ss: scala.collection.Seq[Int] = ???
{
  case val selector11: scala.collection.Seq[Int] = Test.ss
  {
    def case11(): Nothing = 
      {
        def matchFail11(): Nothing = throw new MatchError(selector11)
        if selector11.isInstanceOf[scala.collection.immutable.List[Int]]
           then
         
          {
            case val ss: scala.collection.immutable.List[Int] = 
              selector11.asInstanceOf[
                scala.collection.immutable.List[Int](ss)
              ]
            {
              {
                ???
              }
            }
          }
         else matchFail11()
      }

If the pattern matcher could figure out that the argument is always an Int, it could use

        if selector11.isInstanceOf[scala.collection.immutable.List[_]]

instead of currently:

        if selector11.isInstanceOf[scala.collection.immutable.List[Int]]

DarkDimius · 2017-01-29T09:28:04Z

@odersky,
I think this false negative can be fixed inside IsInstanceOfEvaluator, without modifying the pattern matcher itself, by observing that List derrives from Seq and that they share the same type argument.
I would prefer pattern matcher to keep the most precise type possible.

odersky · 2017-01-30T05:32:57Z

Rebased to master

odersky · 2017-01-30T06:27:41Z

compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala

+              // param is: Any | AnyRef | java.lang.Object
+              val topType = defn.ObjectType <:< arg
+              // has @unchecked annotation to suppress warnings
+              val hasUncheckedAnnot = arg.hasAnnotation(defn.UncheckedAnnot)


Two of these conditions seem ad-hoc to me. Why exclude Object and arrays of AnyVal element types? I would suspect these are just symptoms and the real exclusion criteria are wider.

The anyValArray check is to make sure we allow: xs.isInstanceOf[Array[X]] where X is a primitive. However, before it would have allowed any value class - I constrained it to only allow checking on primitives (199fd9d).

The topType check is to allow explicit checking on top types, eg:

xs.isInstanceOf[Array[Any]]

xs.isInstanceOf[Array[AnyRef]]

xs.isInstanceOf[Array[Object]]

odersky · 2017-01-30T06:28:45Z

@felixmulder Can you figure out why tests fail now? It seems we have some REPL test failures after rebasing.

felixmulder · 2017-01-30T10:13:30Z

The tests were failing because after the rebase, the error number of ErasedType changed. Fixed :)

odersky · 2017-04-04T17:11:30Z

Needs to be rebased to master. Also need to verify that

val ss: Seq[Int] = ???  
ss match {
  case ss: List[Int] => ???
}

does not give a spurious error. If that's the case LGTM.

odersky · 2017-04-06T07:55:52Z

compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala

@@ -147,7 +185,7 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer =>

          val inMatch = s.qualifier.symbol is Case


This is not a correct discrimination for isInstanceOf's used in pattern matching! It would also cover case objects and enum cases. We need a more robust check. Maybe have a special, second isInstanceOf symbol?

@odersky: This logic was proposed by @DarkDimius when I implemented the original IsInstanceOfEvaluator, he could perhaps know of some reason why this isn't a problem. But the basic idea was that:

val bindings created inside a pattern match desugaring would get the case flag. This was so that we can know that they were specifically created by the PatternMatcher.

I'd have to verify that this does indeed conflict with case objects.

If you're sure (or if I get to verifying before you respond), then if I've understood your proposal correctly:

PatternMatcher should be changed to emit a special caseIsInstanceOf symbol (or similar)

IsInstanceOfEvaluator should intercept these and replace them with either regular isInstanceOf checks or the evaluated expressions.

should it exclusively do this for the special symbol, or do we still want the optimization to be performed elsewhere?

Last - do you want this fix included in this PR or can it be separate? The PR doesn't otherwise touch the original logic. I'd open a second PR with the implementation during the day so that we don't forget.

The caseIsInstanceOf idea looks right (exclusive or not: no idea, propose something :-) It could be a separate PR.

felixmulder · 2017-04-07T09:46:47Z

@odersky - added the missing Seq => List nuance suggested by Jason. If all looks good, please approve and I'll move to fixing the isMatch logic.

felixmulder · 2017-04-07T09:56:41Z

@odersky - actually one more caveat. (x: Any) match { case List[_] => ??? } still warns. I'll fix this, then we should be good to go.

Update: fixed in ed5bb8b

felixmulder · 2017-04-07T12:00:09Z

Rebased

odersky

The general case of false negatives still needs to be solved.

odersky · 2017-04-10T09:00:39Z

compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala

+                case tr: TypeRef =>
+                  tr.symbol.is(BindDefinedType)
+                case TypeBounds(lo, hi) =>
+                  (lo eq defn.NothingType) && (hi eq defn.AnyType)


eq usually works, but is not in general a reliable method to compare types, since hash consing is not guaranteed. I recommend to use instead:

lo.isRef(defn.NothingClass) && hi.isRef(defn.AnyClass)

odersky · 2017-04-10T09:05:21Z

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
+


This should not be needed (see below).

felixmulder · 2017-04-11T12:21:27Z

Needs subtypeWithArgs before progress can be made.

felixmulder · 2017-07-31T12:49:05Z

Since IsInstanceOfEvaluator.scala no longer exists, this PR is obsolete. Issue #1828 still remains to be solved in a different manner.

felixmulder added the area:reporting Error reporting including formatting, implicit suggestions, etc label Dec 19, 2016

felixmulder requested a review from smarter December 19, 2016 14:23

felixmulder added stat:in progress and removed stat:in progress labels Dec 19, 2016

smarter reviewed Dec 19, 2016

View reviewed changes

felixmulder force-pushed the fix-#1828 branch 2 times, most recently from e27232a to 8b7b0ef Compare December 19, 2016 15:55

odersky reviewed Dec 21, 2016

View reviewed changes

felixmulder force-pushed the fix-#1828 branch 3 times, most recently from 423e6fb to 2909603 Compare December 22, 2016 11:04

felixmulder requested a review from smarter January 11, 2017 18:21

odersky force-pushed the fix-#1828 branch from 6974d2a to 02577b4 Compare January 30, 2017 05:32

odersky requested changes Jan 30, 2017

View reviewed changes

felixmulder force-pushed the fix-#1828 branch 2 times, most recently from 6974d2a to 3dd2eb7 Compare January 30, 2017 09:46

odersky reviewed Apr 6, 2017

View reviewed changes

felixmulder force-pushed the fix-#1828 branch from 199fd9d to 4b31518 Compare April 6, 2017 13:50

felixmulder added 11 commits April 7, 2017 13:56

Fix scala#1828: add warning when patternmatching on generics

4ce3d6a

Make abstract pattern error into an error message

df6f478

Make sure REPL tests run with -unchecked flag

be9f737

Cover cases where other generics are erased

539ea0c

Don't warn for erasure on arrays with value types

ec21ea7

Add comments to Phase explaining warnings

90ceb34

Make sure IsInstanceOfEvaluator respects @unchecked

980638f

Fix matching on top types

b0d3aa2

Constrain anyValArray check to primitives

1f1b1ba

Relax warnings when matching from Seq to List

1a8af58

Relax unchecked for underscore and type bindings

237cd34

felixmulder force-pushed the fix-#1828 branch from ed5bb8b to 237cd34 Compare April 7, 2017 11:56

odersky requested changes Apr 10, 2017

View reviewed changes

felixmulder added stat:on hold and removed stat:needs review labels Apr 11, 2017

felixmulder closed this Jul 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1828: add warning when patternmatching on generics #1834

Fix #1828: add warning when patternmatching on generics #1834

felixmulder commented Dec 19, 2016

smarter Dec 19, 2016

smarter Dec 19, 2016

smarter Dec 19, 2016

felixmulder Dec 19, 2016

smarter Dec 19, 2016 •

edited

Loading

smarter commented Dec 19, 2016

felixmulder commented Dec 19, 2016

odersky Dec 21, 2016

felixmulder Dec 21, 2016

retronym commented Dec 21, 2016

felixmulder commented Dec 21, 2016

retronym commented Dec 21, 2016

retronym commented Dec 21, 2016

felixmulder commented Dec 23, 2016

odersky commented Jan 29, 2017 •

edited

Loading

odersky commented Jan 29, 2017

DarkDimius commented Jan 29, 2017 •

edited

Loading

odersky commented Jan 30, 2017

odersky Jan 30, 2017

felixmulder Jan 30, 2017 •

edited

Loading

odersky commented Jan 30, 2017

felixmulder commented Jan 30, 2017 •

edited

Loading

odersky commented Apr 4, 2017

odersky Apr 6, 2017

felixmulder Apr 6, 2017

odersky Apr 6, 2017

felixmulder commented Apr 7, 2017

felixmulder commented Apr 7, 2017 •

edited

Loading

felixmulder commented Apr 7, 2017

odersky left a comment

odersky Apr 10, 2017

odersky Apr 10, 2017

felixmulder commented Apr 11, 2017

felixmulder commented Jul 31, 2017

		@@ -147,7 +185,7 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer =>

		val inMatch = s.qualifier.symbol is Case

Fix #1828: add warning when patternmatching on generics #1834

Fix #1828: add warning when patternmatching on generics #1834

Conversation

felixmulder commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter Dec 19, 2016 • edited Loading

Choose a reason for hiding this comment

smarter commented Dec 19, 2016

felixmulder commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retronym commented Dec 21, 2016

felixmulder commented Dec 21, 2016

retronym commented Dec 21, 2016

retronym commented Dec 21, 2016

felixmulder commented Dec 23, 2016

odersky commented Jan 29, 2017 • edited Loading

odersky commented Jan 29, 2017

DarkDimius commented Jan 29, 2017 • edited Loading

odersky commented Jan 30, 2017

Choose a reason for hiding this comment

felixmulder Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

odersky commented Jan 30, 2017

felixmulder commented Jan 30, 2017 • edited Loading

odersky commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixmulder commented Apr 7, 2017

felixmulder commented Apr 7, 2017 • edited Loading

felixmulder commented Apr 7, 2017

odersky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixmulder commented Apr 11, 2017

felixmulder commented Jul 31, 2017

smarter Dec 19, 2016 •

edited

Loading

odersky commented Jan 29, 2017 •

edited

Loading

DarkDimius commented Jan 29, 2017 •

edited

Loading

felixmulder Jan 30, 2017 •

edited

Loading

felixmulder commented Jan 30, 2017 •

edited

Loading

felixmulder commented Apr 7, 2017 •

edited

Loading