New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Casts inserted when pattern matching for equality on singletons are unsound #1503
Comments
Imported From: https://issues.scala-lang.org/browse/SI-1503?orig=1 |
@DRMacIver said: |
@DRMacIver said: The suggested fix is that stable identifier patterns defined behaviour will be modified so that something matches it if and only if it is equal to that stable identifier and is an instance of the static type of that identifier. Thus in this case L() will not match the pattern N because it is not an instance of N.type. |
@paulp said: |
@odersky said: \subsection{Pattern Binders} (The differences are, obviously, that the runtime test could apply to more than just equality checks (needs to me analyzed carefully, so that we generate the right amount of runtime checks), and that runtime checks are only needed when an `@' is present. |
@paulp said: I also thought of this approach, but I did not think you'd want to have different semantics for matching solely based on the presence/absence of a binding - that is, if I'm understanding correctly we're saying given this: case N => true
case x @ N => true the first will match and the second won't, given the same scrutinee of L() from the test case. I am assuming that is indeed what you mean - I am all for it and will implement it in the near future. |
@odersky said: For instance: val foo: List[String] foo match { In the first case, the best type for n is List[String], the expected type. I think this would be the most logical approach. But I am not sure how many old programs would break, because some variables would get assigned weaker types than before. Anyway, it looks more and more like there is a paper in this. Things are far from trivial. |
@paulp said: val buf: Array[Byte]
def processVerbatim(i: Int, end: String): Int = {
val END = end.charAt(0)
def processCode(pre: String, i: Int): Int = {...
buf(i) match {
case END if startsWith(i, end) =>
// return
}
Interestingly I can't reproduce this in a simple example, but I did reproduce the behavior in verbfilterScala.scala, and I did verify that reverting the patch for this ticket restored the original matching behavior. |
@paulp said: |
@paulp said: case object Foo {
def bippy = 1
override def equals(other: Any) = true
}
object Test extends App {
("": Any) match { case p @ Foo => p.bippy }
}
|
This comment has been minimized.
This comment has been minimized.
@retronym said: scala> (IndexedSeq(): Any) match { case n @ Nil => n.mapConserve(x => x) }
java.lang.ClassCastException: scala.collection.immutable.Vector cannot be cast to scala.collection.immutable.List |
@paulp said: scala> (Vector(): Any) match { case n @ Nil => n }
java.lang.ClassCastException: scala.collection.immutable.Vector cannot be cast to scala.collection.immutable.Nil$
... 32 elided |
@adriaanm said: |
@adriaanm said: I've implemented Martin's alternative proposal, which calls for determining the type implied by a pattern matching a certain value. Usually, this is the pattern's type because pattern matching implies instance-of checks. However, Stable Identifier and Literal patterns are matched using
|
@adriaanm said: |
@paulp said: object Test {
def f(xs: Any) = xs match {
case xs @ Nil => xs
case _ => List[Int]()
}
} |
@adriaanm said: |
@paulp said: |
@adriaanm said: |
@paulp said: |
@adriaanm said: |
@paulp said (edited on Jan 29, 2014 3:59:00 AM UTC): I think the responsible thing to do is issue a warning when the inferred type as-calculated-until-now differs from the inferred type as-calculated-after-now. It is the silent changes in behavior which can easily accompany any change in type inference which I think unreasonable in this case, because people generally find ClassCastExceptions all by themselves if the code path is ever exercised. That means that in the short run the only likely effect of changing this will be to make code which used to compile stop compiling and to make code which used to work stop working. |
@adriaanm said: |
@retronym said: Just as a reminder, we also need to fix the regression noted in #5900/#7886 before 2.11.0-RC1. That might involve rolling back the changes of #7886 (unsound pattern inference). That patch actually only superfically improved the state of play. These sort of unresolved questions suggests to me that the schedule for RC1 isn't realistic and that we need another month or so. |
@adriaanm said: PS: I'm against further delays. Let's fix what we can (only) fix in 2.11.0 now, reverting where necessary. Everything else goes to 2.11.1 or 2.12. |
@adriaanm said: |
@paulp said: |
@adriaanm said: |
@adriaanm said (edited on Feb 25, 2014 8:46:29 PM UTC): |
Brian Kent (bkent314) said: For example, in doing Currently it just says something is dangerous buy offers no alternatives. |
@milessabin said: > scala
[info] Running scala.tools.nsc.MainGenericRunner -usejavacp
Welcome to Scala 2.12.0-20160722-172732-9ee8124 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_102).
Type in expressions for evaluation. Or try :help.
scala> :paste
// Entering paste mode (ctrl-D to finish)
object Test extends App {
case class L();
object N extends L();
def empty(xs : L) : Unit = xs match {
case x@N => println(x); println(x);
}
empty(L())
}
// Exiting paste mode, now interpreting.
<console>:15: warning: match may not be exhaustive.
It would fail on the following input: L()
def empty(xs : L) : Unit = xs match {
^
defined object Test |
Fixes scala/bug#1503 scala#3559 stopped assuming unsound type for ident/literal patterns under -Xfuture in Scala 2.11. Since enough time has elapsed, I am ripping the bandaid off on SI-1503 (scala/bug#1503).
new PR scala/scala#6502 |
It warns about scala/bug#1503
I've added a test to pending/run/castsingleton.scala:
The problem is that the compiler inserts a cast of xs to N.type, which is unsound: The pattern match will succeed for any L, because N == L().
The text was updated successfully, but these errors were encountered: