Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Merged
merged 1 commit into from

2 participants

@adriaanm
Owner

The fix only kicks in under -Xfuture. We also warn under -Xlint.

What type should a variable bound to the value matched by a pattern have?
To avoid CCEs, it should be a type that's implied by the matching
semantics of the pattern.

Usually, the type implied by a pattern matching a certain value
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.

The change in type checking due to this fix is that programs that used to crash with a CCE
(because we blindly cast to the type of the pattern, which a == check does not imply)
now get a weaker type instead (and no cast). They may still type check, or they may not.

To compensate for this fix, change case x@Foo => x to case x: Foo.type => x,
if it's important that x have type Foo.type.

See also:

  • SI-4577: matching of singleton type patterns uses eq, not == (so that the types are not a lie).
  • SI-5024: patmat strips unused bindings, but affects semantics
@adriaanm
Owner

Review by @gkossakowski

@adriaanm adriaanm SI-1503 don't assume unsound type for ident/literal patterns
The fix only kicks in under -Xfuture. We also warn under -Xlint.

What type should a variable bound to the value matched by a pattern have?
To avoid CCEs, it should be a type that's implied by the matching
semantics of the pattern.

Usually, the type implied by a pattern matching a certain value
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.

The change in type checking due to this fix is that programs that used to crash with a CCE
(because we blindly cast to the type of the pattern, which a `==` check does not imply)
now get a weaker type instead (and no cast). They may still type check, or they may not.

To compensate for this fix, change `case x@Foo => x` to `case x: Foo.type => x`,
if it's important that `x` have type `Foo.type`.

See also:
- SI-4577: matching of singleton type patterns uses `eq`,
  not `==` (so that the types are not a lie).
- SI-5024: patmat strips unused bindings, but affects semantics
c001b88
@adriaanm adriaanm added tested and removed needs-attention labels
@gkossakowski

LGTM

@gkossakowski gkossakowski merged commit 5c84255 into scala:master

1 check passed

Details default pr-scala Took 72 min.
@adriaanm adriaanm deleted the adriaanm:t1503 branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 19, 2014
  1. @adriaanm

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

    adriaanm authored
    The fix only kicks in under -Xfuture. We also warn under -Xlint.
    
    What type should a variable bound to the value matched by a pattern have?
    To avoid CCEs, it should be a type that's implied by the matching
    semantics of the pattern.
    
    Usually, the type implied by a pattern matching a certain value
    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.
    
    The change in type checking due to this fix is that programs that used to crash with a CCE
    (because we blindly cast to the type of the pattern, which a `==` check does not imply)
    now get a weaker type instead (and no cast). They may still type check, or they may not.
    
    To compensate for this fix, change `case x@Foo => x` to `case x: Foo.type => x`,
    if it's important that `x` have type `Foo.type`.
    
    See also:
    - SI-4577: matching of singleton type patterns uses `eq`,
      not `==` (so that the types are not a lie).
    - SI-5024: patmat strips unused bindings, but affects semantics
This page is out of date. Refresh to see the latest.
View
4 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))
View
50 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:
View
5 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
View
8 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
View
1  test/files/neg/t1503.flags
@@ -0,0 +1 @@
+-Xlint -Xfatal-warnings
View
8 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 }
+}
View
1  test/files/run/t1503.check
@@ -0,0 +1 @@
+whoops
View
20 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")
+ }
+}
View
1  test/files/run/t1503_future.flags
@@ -0,0 +1 @@
+-Xfuture
View
17 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")
+}
Something went wrong with that request. Please try again.