From c001b888b896989a2c0afa0c24d038502970151c Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Tue, 28 Jan 2014 16:43:40 -0800 Subject: 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 --- .../scala/reflect/reify/codegen/GenTypes.scala | 4 +- .../tools/nsc/transform/patmat/MatchAnalysis.scala | 50 ++++++++++++++++++++++ .../scala/tools/nsc/typechecker/Typers.scala | 5 ++- test/files/neg/t1503.check | 8 ++++ test/files/neg/t1503.flags | 1 + test/files/neg/t1503.scala | 8 ++++ test/files/run/t1503.check | 1 + test/files/run/t1503.scala | 20 +++++++++ test/files/run/t1503_future.flags | 1 + test/files/run/t1503_future.scala | 17 ++++++++ 10 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 test/files/neg/t1503.check create mode 100644 test/files/neg/t1503.flags create mode 100644 test/files/neg/t1503.scala create mode 100644 test/files/run/t1503.check create mode 100644 test/files/run/t1503.scala create mode 100644 test/files/run/t1503_future.flags create mode 100644 test/files/run/t1503_future.scala diff --git a/src/compiler/scala/reflect/reify/codegen/GenTypes.scala b/src/compiler/scala/reflect/reify/codegen/GenTypes.scala index a90a3a338b..cfd3239015 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 2a3c631a66..cb15be79c0 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 f4d2a2cea0..e0f8d87523 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 -- cgit v1.2.3