Merge pull request #3559 from adriaanm/t1503

SI-1503 don't assume unsound type for ident/literal patterns

10 files changed, 111 insertions, 4 deletions

diff --git a/src/compiler/scala/reflect/reify/codegen/GenTypes.scala b/src/compiler/scala/reflect/reify/codegen/GenTypes.scala
index a6b69e239f..d007df75e3 100644
--- a/src/compiler/scala/reflect/reify/codegen/GenTypes.scala
+++ b/src/compiler/scala/reflect/reify/codegen/GenTypes.scala
@@ -34,9 +34,9 @@ trait GenTypes {
     if (tsym.isClass && tpe == tsym.typeConstructor && tsym.isStatic)
       Select(Select(reify(tsym), nme.asType), nme.toTypeConstructor)
     else tpe match {
-      case tpe @ NoType =>
+      case tpe : NoType.type =>
         reifyMirrorObject(tpe)
-      case tpe @ NoPrefix =>
+      case tpe : NoPrefix.type =>
         reifyMirrorObject(tpe)
       case tpe @ ThisType(root) if root.isRoot =>
         mirrorBuildCall(nme.thisPrefix, mirrorMirrorSelect(nme.RootClass))
diff --git a/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala b/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala
index dc4969be43..2893cbdf45 100644
--- a/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala
+++ b/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala
@@ -16,6 +16,56 @@ trait TreeAndTypeAnalysis extends Debugging {
   import definitions._
   import analyzer.Typer
 
+  /** Compute the type T implied for a value `v` matched by a pattern `pat` (with expected type `pt`).
+   *
+   * Usually, this is the pattern's type because pattern matching implies instance-of checks.
+   *
+   * However, Stable Identifier and Literal patterns are matched using `==`,
+   * which does not imply a type for the binder that binds the matched value.
+   *
+   * See SI-1503, SI-5024: don't cast binders to types we're not sure they have
+   *
+   * TODO: update spec as follows (deviation between `**`):
+   *
+   *   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 **IMPLIED BY** the pattern p.
+   *   This pattern matches any value v matched by the pattern p
+   *     **Deleted: , provided the run-time type of v is also an instance of T, **
+   *   and it binds the variable name to that value.
+   *
+   *   Addition:
+   *     A pattern `p` _implies_ a type `T` if the pattern matches only values of the type `T`.
+   */
+  def binderTypeImpliedByPattern(pat: Tree, pt: Type, binder: Symbol): Type =
+    pat match {
+      // because `==` decides whether these patterns match, stable identifier patterns (ident or selection)
+      // do not contribute any type information (beyond the pattern's expected type)
+      // e.g., in case x@Nil => x --> all we know about `x` is that it satisfies Nil == x, which could be anything
+      case Ident(_) | Select(_, _) =>
+        if (settings.future) pt
+        else {
+          // TODO: don't warn unless this unsound assumption is actually used in a cast
+          // I tried annotating the type returned here with an internal annotation (`pat.tpe withAnnotation UnsoundAssumptionAnnotation`),
+          // and catching it in the patmat backend when used in a cast (because that would signal the unsound assumption was used),
+          // but the annotation didn't bubble up...
+          // This is a pretty poor approximation.
+          def unsoundAssumptionUsed = binder.name != nme.WILDCARD && !(pt <:< pat.tpe)
+          if (settings.lint && unsoundAssumptionUsed)
+            global.currentUnit.warning(pat.pos,
+              sm"""The value matched by $pat is bound to ${binder.name}, which may be used under the
+                  |unsound assumption that it has type ${pat.tpe}, whereas we can only safely
+                  |count on it having type $pt, as the pattern is matched using `==` (see SI-1503).""")
+
+          pat.tpe
+        }
+
+
+      // the other patterns imply type tests, so we can safely assume the binder has the pattern's type when the pattern matches
+      // concretely, a literal, type pattern, a case class (the constructor's result type) or extractor (the unapply's argument type) all imply type tests
+      // (and, inductively, an alternative)
+      case _ => pat.tpe
+    }
+
   // we use subtyping as a model for implication between instanceof tests
   // i.e., when S <:< T we assume x.isInstanceOf[S] implies x.isInstanceOf[T]
   // unfortunately this is not true in general:
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index fa96d98bcc..8a3ceb3aca 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -4080,9 +4080,10 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
             }
 
             val body1 = typed(body, mode, pt)
+            val impliedType = patmat.binderTypeImpliedByPattern(body1, pt, sym) // SI-1503, SI-5204
             val symTp =
-              if (treeInfo.isSequenceValued(body)) seqType(body1.tpe)
-              else body1.tpe
+              if (treeInfo.isSequenceValued(body)) seqType(impliedType)
+              else impliedType
             sym setInfo symTp
 
             // have to imperatively set the symbol for this bind to keep it in sync with the symbols used in the body of a case
diff --git a/test/files/neg/t1503.check b/test/files/neg/t1503.check
new file mode 100644
index 0000000000..7adeea20f3
--- /dev/null
+++ b/test/files/neg/t1503.check
@@ -0,0 +1,8 @@
+t1503.scala:7: warning: The value matched by Whatever is bound to n, which may be used under the
+unsound assumption that it has type Whatever.type, whereas we can only safely
+count on it having type Any, as the pattern is matched using `==` (see SI-1503).
+  def matchWhateverCCE(x: Any) = x match { case n @ Whatever => n }
+                                                    ^
+error: No warnings can be incurred under -Xfatal-warnings.
+one warning found
+one error found
diff --git a/test/files/neg/t1503.flags b/test/files/neg/t1503.flags
new file mode 100644
index 0000000000..e93641e931
--- /dev/null
+++ b/test/files/neg/t1503.flags
@@ -0,0 +1 @@
+-Xlint -Xfatal-warnings
\ No newline at end of file
diff --git a/test/files/neg/t1503.scala b/test/files/neg/t1503.scala
new file mode 100644
index 0000000000..9877f99d0a
--- /dev/null
+++ b/test/files/neg/t1503.scala
@@ -0,0 +1,8 @@
+object Whatever {
+  override def equals(x: Any) = true
+}
+
+class Test {
+  // when left to its own devices, and not under -Xfuture, the return type is Whatever.type
+  def matchWhateverCCE(x: Any) = x match { case n @ Whatever => n }
+}
\ No newline at end of file
diff --git a/test/files/run/t1503.check b/test/files/run/t1503.check
new file mode 100644
index 0000000000..43eceb0229
--- /dev/null
+++ b/test/files/run/t1503.check
@@ -0,0 +1 @@
+whoops
diff --git a/test/files/run/t1503.scala b/test/files/run/t1503.scala
new file mode 100644
index 0000000000..1be0e74ac2
--- /dev/null
+++ b/test/files/run/t1503.scala
@@ -0,0 +1,20 @@
+object Whatever {
+  override def equals(x: Any) = true
+}
+
+object Test extends App {
+  // this should make it abundantly clear Any is the best return type we can guarantee
+  def matchWhatever(x: Any): Any = x match { case n @ Whatever => n }
+  // when left to its own devices, and not under -Xfuture, the return type is Whatever.type
+  def matchWhateverCCE(x: Any) = x match { case n @ Whatever => n }
+
+  // just to exercise it a bit
+  assert(matchWhatever(1) == 1)
+  assert(matchWhatever("1") == "1")
+
+  try {
+    matchWhateverCCE("1"): Whatever.type
+  } catch {
+    case _: ClassCastException => println("whoops")
+  }
+}
\ No newline at end of file
diff --git a/test/files/run/t1503_future.flags b/test/files/run/t1503_future.flags
new file mode 100644
index 0000000000..112fc720a0
--- /dev/null
+++ b/test/files/run/t1503_future.flags
@@ -0,0 +1 @@
+-Xfuture
\ No newline at end of file
diff --git a/test/files/run/t1503_future.scala b/test/files/run/t1503_future.scala
new file mode 100644
index 0000000000..1e3daad761
--- /dev/null
+++ b/test/files/run/t1503_future.scala
@@ -0,0 +1,17 @@
+object Whatever {
+  override def equals(x: Any) = true
+}
+
+object Test extends App {
+  // this should make it abundantly clear Any is the best return type we can guarantee
+  def matchWhatever(x: Any): Any = x match { case n @ Whatever => n }
+  // when left to its own devices, and not under -Xfuture, the return type is Whatever.type
+  def matchWhateverCCE(x: Any) = x match { case n @ Whatever => n }
+
+  // just to exercise it a bit
+  assert(matchWhatever(1) == 1)
+  assert(matchWhatever("1") == "1")
+
+  assert(matchWhateverCCE(1) == 1)
+  assert(matchWhateverCCE("1") == "1")
+}
\ No newline at end of file