Skip to content

Missed match on Array(1L) in Scala 3.1.1 #14693

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

Closed
devlaam opened this issue Mar 15, 2022 · 9 comments · Fixed by #14766
Closed

Missed match on Array(1L) in Scala 3.1.1 #14693

devlaam opened this issue Mar 15, 2022 · 9 comments · Fixed by #14766

Comments

@devlaam
Copy link

devlaam commented Mar 15, 2022

Observation

Consider the following code:

object MatchTest { 
  val a: Array[Long] = Array(1L)

  def test(x: Any) = x match { 
    case Array(i: Long) => println("Success!")
    case _              => println("Failure!") }

  def main(args: Array[String]): Unit = test(a) }

When run under Scala 2.13.8 this produces Success whereas under Scala 3.1.1 it produces Failure. For the latter it does not matter if the compiler option -source:3.0-migration is used.

Expectation

Same behaviour under both versions or (at least) a warning that the match will fail. Current situation significantly changes runtime behaviour after migrating projects from Scala 2 to Scala 3.

@devlaam devlaam added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 15, 2022
@SethTisue
Copy link
Member

SethTisue commented Mar 15, 2022

Dale (@dwijnand) has been working in this area recently. is the problem reproducible in 3.1.2-RC2 and/or in the latest nightly (3.2.0-RC1-bin-20220308-29073f1-NIGHTLY-git-29073f1)?

@som-snytt
Copy link
Contributor

Welcome to Scala 3.1.3-RC1-bin-SNAPSHOT-git-5fcfeec (17.0.2, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> (Array(42L): Any) match { case Array(x: Long) => x }
scala.MatchError: [J@75add13c (of class [J)
  ... 33 elided

scala> Array(42L) match { case Array(x: Long) => x }
val res1: Long = 42

@devlaam

This comment was marked as off-topic.

@smarter

This comment was marked as off-topic.

@devlaam

This comment was marked as off-topic.

@smarter

This comment was marked as off-topic.

@dwijnand dwijnand added area:pattern-matching and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 18, 2022
@smarter
Copy link
Member

smarter commented Mar 18, 2022

I think the problem is with the definition of Array.unapplySeq, if I tweak it like this:

diff --git src/library/scala/Array.scala src/library/scala/Array.scala
index 14cdabd385..a64ae3cb8c 100644
--- src/library/scala/Array.scala
+++ src/library/scala/Array.scala
@@ -574,9 +574,9 @@ object Array {
    *  @param x the selector value
    *  @return  sequence wrapped in a [[scala.Some]], if `x` is an Array, otherwise `None`
    */
-  def unapplySeq[T](x: Array[T]): UnapplySeqWrapper[T] = new UnapplySeqWrapper(x)
+  def unapplySeq[T](x: Array[? <: T]): UnapplySeqWrapper[T] = new UnapplySeqWrapper(x)

-  final class UnapplySeqWrapper[T](private val a: Array[T]) extends AnyVal {
+  final class UnapplySeqWrapper[T](private val a: Array[? <: T]) extends AnyVal {
     def isEmpty: false = false
     def get: UnapplySeqWrapper[T] = this
     def lengthCompare(len: Int): Int = a.lengthCompare(len)

Then the pattern match succeeds. Otherwise, depending on how unapplySeq is instantiated, we'll end up matching either all arrays or only arrays of objects, because Array[Any] erases to Object[]. Note that the behavior of Scala 2 here seems to be a straight-up bug: if I look at the generated tree it tries to do an isInstanceOf check on Array[T] but there is no T in scope (as pointed out by -Ycheck:pat), but luckily for Scala 2, it happens to erase this malformed type to Object so the check succeeds.

I believe this would be a binary-compatible change, so there's a chance we could just apply this diff in the Scala 2 standard library.

@smarter
Copy link
Member

smarter commented Mar 18, 2022

scala/scala#9977

smarter added a commit to smarter/scala that referenced this issue Mar 18, 2022
@smarter
Copy link
Member

smarter commented Mar 23, 2022

I've just now realized that the way we type Array.unapplySeq in patterns is specific to the fact that it's define in a Scala 2 class, if I define my own extractor, then the match succeeds:

object UnapSeq {
  def unapplySeq[T](x: Array[T]): Array.UnapplySeqWrapper[T] = new Array.UnapplySeqWrapper[T](x)
}

object Test {
  val a: Array[Long] = Array(1L)

  def test(x: Any) = x match { 
    case UnapSeq(i: Long) => println("Success!")
    case _                => println("Failure!") }

  def main(args: Array[String]): Unit = test(a)
}

The culprit is the use of fromScala2x in https://github.com/lampepfl/dotty/blob/fb7f900667ea57e78a098e4831be36e0a7da6cba/compiler/src/dotty/tools/dotc/typer/Applications.scala#L1346
If I replace it by false, then the Array.unapplySeq argument is inferred to be a fresh pattern bound type variable T$1 instead of Any and Array[T$1] erases to Object as expected. So ironically, a flag that is supposed to improve Scala 2 compatibility ends up changing semantics to not match what either of Scala 2 and 3 do by default.
There's no documentation for the behavior of fromScala2x currently, but historically it seems like it was needed to deal with :: in the Scala <= 2.12 standard library:9ea9ef4#diff-8893c00063ce469a750bdbdbca40265550e0901a39a8a45e2f83aa6d51230aa0L925-R939

@odersky do you think we could get rid of fromScala2x in typedUnapply or at least tweak it to fix this?

odersky added a commit to dotty-staging/dotty that referenced this issue Mar 24, 2022
It seems our GADT handling is now able to do without.

Fixes scala#14693
@Kordyjan Kordyjan added this to the 3.1.3 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment