Skip to content
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

Pattern match with unchecked type parameter may cause later runtime failure when Array[T] (for primitive T) is involved #11801

Open
OndrejSpanel opened this issue Nov 14, 2019 · 11 comments

Comments

@OndrejSpanel
Copy link

@OndrejSpanel OndrejSpanel commented Nov 14, 2019

Following code throws a ClassCastException with a description of [D cannot be cast to [Ljava.lang.Object; when executing val a = jsa.data.array. Tested with Scala versions: "2.10.7", "2.11.12", "2.12.10", "2.13.1", all show the issue.

I am running Windows 10 x64, jdk1.8.0_181.

import scala.collection.IndexedSeq

class JSArrayBuffer[A](val array: Array[A])

class JSArray[T](val data: JSArrayBuffer[T]) extends IndexedSeq[T] {
  def length: Int = data.array.length
  def apply(i: Int) = data.array.apply(i)
}

object Main extends App {

  def arraySize[T](a: Array[T]) = a.length

  def print[T: Numeric](a: IndexedSeq[T]): Unit = {
    a match {
      case jsa: JSArray[T@unchecked] =>
        println("arraySize " + arraySize(jsa.data.array)) // prints fine
        println("size " + jsa.data.array.size) // prints fine
        val a = jsa.data.array // throws exception D cannot be cast to [Ljava.lang.Object
      case _ =>
    }
  }

  def run() {
    val a = new JSArray[Double](new JSArrayBuffer(Array(1.0, 2.0)))

    print(a)
  }

  run()
}

@SethTisue

This comment has been minimized.

Copy link
Member

@SethTisue SethTisue commented Nov 14, 2019

can this be minimized any further?

@OndrejSpanel

This comment has been minimized.

Copy link
Author

@OndrejSpanel OndrejSpanel commented Nov 14, 2019

Inheriting from a simple trait instead of IndexedSeq still shows the issue. Numeric is not necessary. One level of encapsulation is enough (JSArray can be removed), but needs to be generic, the print method needs to be generic as well. Following code is reduced a bit and still throws:

trait Marker

class Buffer[A](val a: Array[A]) extends Marker

object Main extends App {

  def print[T](m: Marker): Unit = {
    m match {
      case b: Buffer[T@unchecked] =>
        val a = b.a // throws exception D cannot be cast to [Ljava.lang.Object
    }
  }

  val a = new Buffer(Array[Double](1.0, 2.0))

  print[Double](a)
}

@Jasper-M

This comment has been minimized.

Copy link
Member

@Jasper-M Jasper-M commented Nov 14, 2019

I haven't looked into this much, but it stops throwing when you replace T@unchecked with _. So perhaps this is just a consequence of ignoring a compiler warning?

Can't say for sure whether this is a bug or not though.

@OndrejSpanel

This comment has been minimized.

Copy link
Author

@OndrejSpanel OndrejSpanel commented Nov 14, 2019

It is hard for me to be sure, being no expert, but @unchecked seem safe to me in this code.

In the original post code the value entering is guaranteed to be JSArray[T] (if it is JSArray at all), as it is entering as IndexedSeq[T]. In the reduced repro the value entered is always Buffer[Double], therefore matching it against Buffer[T] with T = Double should still be a valid code. What I find most strange is one can call a function like arraySize(b.a) without any issues (see the code which is not reduced), the exception is thrown only when you assign the b.a into a variable or try to execute b.a.length directly.

@SethTisue

This comment has been minimized.

Copy link
Member

@SethTisue SethTisue commented Nov 14, 2019

still a ways from minimal — the Marker trait plays no role here. (and I suspect, though I don't know, that isn't the only thing you can get rid of?)

@som-snytt

This comment has been minimized.

Copy link

@som-snytt som-snytt commented Nov 14, 2019

Scala 3 correctly omits the cast.

Compare the other ticket from yesterday where Scala 2 mishandles the type of the variable introduced by a typed pattern, also fixed in dotty.

def f[T](m: Marker): T = m match { case b: Buffer[T@unchecked] => b.a(0) }

scala 2

  public <T extends java.lang.Object> T f(Marker);
    descriptor: (LMarker;)Ljava/lang/Object;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=5, args_size=2
         0: aload_1
         1: astore_3
         2: aload_3
         3: instanceof    #93                 // class Buffer
         6: ifeq          29
         9: aload_3
        10: checkcast     #93                 // class Buffer
        13: astore        4
        15: aload         4
        17: invokevirtual #96                 // Method Buffer.a:()Ljava/lang/Object;
        20: checkcast     #98                 // class "[Ljava/lang/Object;"
        23: iconst_0
        24: aaload
        25: astore_2
        26: goto          41
        29: goto          32
        32: new           #106                // class scala/MatchError
        35: dup
        36: aload_3
        37: invokespecial #109                // Method scala/MatchError."<init>":(Ljava/lang/Object;)V
        40: athrow
        41: aload_2
        42: areturn

scala 3

  public <T extends java.lang.Object> T f(Marker);
    descriptor: (LMarker;)Ljava/lang/Object;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=4, args_size=2
         0: aload_1
         1: astore_2
         2: aload_2
         3: instanceof    #35                 // class Buffer
         6: ifeq          28
         9: aload_2
        10: checkcast     #35                 // class Buffer
        13: astore_3
        14: getstatic     #135                // Field scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$;
        17: aload_3
        18: invokevirtual #120                // Method Buffer.a:()Ljava/lang/Object;
        21: iconst_0
        22: invokevirtual #139                // Method scala/runtime/ScalaRunTime$.array_apply:(Ljava/lang/Object;I)Ljava/lang/Object;
        25: goto          37
        28: new           #122                // class scala/MatchError
        31: dup
        32: aload_2
        33: invokespecial #123                // Method scala/MatchError."<init>":(Ljava/lang/Object;)V
        36: athrow
        37: areturn
@OndrejSpanel

This comment has been minimized.

Copy link
Author

@OndrejSpanel OndrejSpanel commented Nov 14, 2019

Removing Marker is indeed possible, but other than that, I am really no longer able to remove anything.

Reading "ways from minimal" was quite discouraging. I have started with a really large project and I was glad to be able to reduce it to 20 lines. Now it is self-contained, no external references and looks quite short to me. If anyone else can reduce it even more, please, do so.

class Buffer[A](val a: Array[A])

object Main extends App {
  def print[T](m: AnyRef): Unit = {
    m match {
      case b: Buffer[T@unchecked] =>
        val a = b.a // throws exception D cannot be cast to [Ljava.lang.Object
    }
  }

  val a = new Buffer(Array[Double](1.0, 2.0))

  print[Double](a)
}
@SethTisue

This comment has been minimized.

Copy link
Member

@SethTisue SethTisue commented Nov 14, 2019

Minimizing bugs is hard work, I'm afraid, and a skill in itself. I didn't mean to scold.

And you're right that Marker is the last significant thing can go, afaict. Leaving us with:

class Buffer[A](val a: Array[A])
def foo[T](x: Any) = x match { case b: Buffer[T@unchecked] => b.a }
foo[Double](new Buffer(Array[Double]()))

Changes that make the bug vanish (noting these is also part of characterizing a bug):

  • change Array to List
  • change Double to a reference type such as String
  • omit @unchecked

The last of these seems rather peculiar; https://www.scala-lang.org/api/current/scala/unchecked.html says "should not be considered for additional compiler checks" but doesn't clearly indicate that anything might be affected except warnings.

I will change the name of the ticket, now that its nature is more clear.

@SethTisue SethTisue changed the title Pattern matched class derived from IndexedSeq throws ClassCastException when a data member is accessed Pattern match with unchecked type parameter may cause later runtime failure when Array[T] (for primitive T) is involved Nov 14, 2019
@SethTisue SethTisue added the patmat label Nov 14, 2019
@SethTisue SethTisue added this to the Backlog milestone Nov 14, 2019
@SethTisue

This comment has been minimized.

Copy link
Member

@SethTisue SethTisue commented Nov 14, 2019

Compare the other ticket from yesterday where Scala 2 mishandles the type of the variable introduced by a typed pattern, also fixed in dotty.

(#11800)

Edit: actually the other one #10315 where the type of the typed pattern is not respected.

@som-snytt

This comment has been minimized.

Copy link

@som-snytt som-snytt commented Nov 14, 2019

We need a scolded label so people don't waste time re-scolding.

@som-snytt

This comment has been minimized.

Copy link

@som-snytt som-snytt commented Nov 14, 2019

anything might be affected

A bug is an unconstrained effect by definition. In my example, TIL when it doesn't know the array type, it uses array_apply to runtime test it. For some reason, unchecked makes it think it's an array of references, the erased T, which is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.