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

array pattern must respect array invariance #3968

Open
scabug opened this issue Oct 29, 2010 · 15 comments
Open

array pattern must respect array invariance #3968

scabug opened this issue Oct 29, 2010 · 15 comments
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Oct 29, 2010

Since it is not going down without a fight, ticket time. Adriaan observed that Array matching is being done covariantly. So is isInstanceOf it turns out. The following prints "1 1" but should print "2 2".

object Test {
  def f1(x: Any) = x match { 
    case _: Array[AnyRef] => 1
    case _: Array[String] => 2
    case _                => 3
  }
  def f2(x: Any) =
    if (x.isInstanceOf[Array[AnyRef]]) 1
    else if (x.isInstanceOf[Array[String]]) 2
    else 3
  
  def main(args: Array[String]): Unit = {
    println(f1(args))
    println(f2(args))
  }
}
@scabug
Copy link
Author

@scabug scabug commented Oct 29, 2010

@scabug
Copy link
Author

@scabug scabug commented Nov 1, 2010

@paulp said:
Inspection reveals this is happening because java is burning us once again: scala is generating the instanceof operator, and according to the JVM an Array[String] is indeed an Array[Object].

I became uncertain and verified with the specification that this should not match. It seems pretty clear that it should not.

First, unlike arrays in Java or C#, arrays in Scala are not co-variant; That is, S <: T does not imply Array[S] <: Array[T ] in Scala. However, it is possible to cast an array of S to an array of T if such a cast is permitted in the host environment.
For instance Array[String] does not conform to Array[Object], even though String conforms to Object. However, it is possible to cast an expression of type Array[String] to Array[Object], and this cast will succeed without raising a ClassCastException. Example:

I believe I know how to fix it, and I will now investigate this belief.

@scabug
Copy link
Author

@scabug scabug commented Nov 1, 2010

@paulp said:
In case anyone's on the edge of their seat, I have this all fixed and there is good news and bad news. I lack the time for a full report right now so it'll have to be tomorrow. As a teaser, there are a number of places in the compiler expecting

  case x: Array[AnyRef] => 

to catch everything but the primitive arrays. So assuming that shouldn't match, what would we like to replace it with? The problem is that this:

  case x: Array[_] =>

Does not give you an Array[AnyRef] on the right hand side, so you end up with lots of currently unnecessary casts. It looks like a bug that:

  case x: Array[_ <: AnyRef] =>

doesn't work (it says "not found: type _$$1" or thereabouts.) However this seems to work, although I haven't properly tested. I can't get it to compile without defining an alias.

  type AnyAnyRef = T forSome { type T <: AnyRef }
  case x: Array[AnyAnyRef]   => ...
@scabug
Copy link
Author

@scabug scabug commented Nov 1, 2010

@scabug
Copy link
Author

@scabug scabug commented Nov 5, 2010

@adriaanm said:
(note: this is pending for discussion at a meeting where Martin is present, probably next week)

@scabug
Copy link
Author

@scabug scabug commented Nov 5, 2010

@paulp said:
Replying to [comment:6 moors]:

(note: this is pending for discussion at a meeting where Martin is present, probably next week)

OK, thanks for the info. As a point of interest this appears to have been the true cause of various confusions we experienced regarding the necessity of matchesPattern etc.

@scabug
Copy link
Author

@scabug scabug commented Nov 9, 2010

@adriaanm said:
the compiler should report an unreachable code error for the example above, but the matching behaviour has keep following the semantics of Java's instanceof, such is the oath we took when embarking on this journey

even if we did make a u-turn onto the high road, the worry is we'd be stuck in reflection traffic

@scabug
Copy link
Author

@scabug scabug commented Nov 9, 2010

@paulp said:
Replying to [comment:10 moors]:

the compiler should report an unreachable code error for the example above, but the matching behaviour has keep following the semantics of Java's instanceof, such is the oath we took when embarking on this journey

I think this needs further consideration. Can we use spec citations? I missed the oath apparently.

even if we did make a u-turn onto the high road, the worry is we'd be stuck in reflection traffic

I can't see how it's a u-turn. The example above doesn't now and hasn't ever said unreachable code. But if we intentionally match covariantly it remains trivial to create runtime failures. How is it any different in effect to have Array[String] match as Array[AnyRef] in a pattern match than it is to allow Array[String] as an argument to an Array[AnyRef] parameter type?

I can't see any point in making arrays behave invariantly everywhere else and then leaving open this huge hole for runtime failure.

(Array[String]("a"): Any) match { case x: Array[AnyRef] => x(0) = new AnyRef }
java.lang.ArrayStoreException: java.lang.Object

And if we're really going to make matching work right wrt type parameters when a manifest is available, we have this fairly ridiculous situation where the erased types match correctly and the type which actually reified the relevant type information has that information discarded and leads to a runtime failure.

@scabug
Copy link
Author

@scabug scabug commented Nov 9, 2010

@paulp said:
]And I realize we could take the position that case x: Array[String] is "unchecked" the way it is with say List. But it just seems terribly perverse to act like it's unchecked when it's only unchecked because we refuse to check, not because we can't. (Also, it doesn't now and I do not think ever has give an unchecked warning.)

@scabug
Copy link
Author

@scabug scabug commented Nov 9, 2010

@paulp said:
OK, I see it in the spec now, although it seems awfully quiet.

A parameterized type pattern scala.Array[T1], where T1 is a type pattern. This type pattern matches any non-null instance of type scala.Array[U1], where U1 is a type matched by T1.
@scabug
Copy link
Author

@scabug scabug commented Mar 8, 2011

@paulp said:
Noting for the record that martin has approved fixing this for pattern matching (with isInstanceOf doing the looser "java" match, which is consistent with the other places where the pattern matcher is tighter such as path dependent types) and I just need a little time to finish it. Hopefully whatever follows 2.9.

@scabug
Copy link
Author

@scabug scabug commented Dec 1, 2011

@soc said (edited on Dec 1, 2011 8:04:09 PM UTC):
Running this on

scala -Yvirtpatmat
Welcome to Scala version 2.10.0.r26093-b20111130020250 (OpenJDK 64-Bit Server VM, Java 1.7.0_147-icedtea).

Results in:

scala> Test.main(null)
3
3

This got worse, didn't it?

Update: Weird, same behavior also with the old patter matcher ... is this a regression?

@scabug
Copy link
Author

@scabug scabug commented Jul 20, 2012

@adriaanm said:
null is not an array

@scabug
Copy link
Author

@scabug scabug commented Jul 20, 2012

@adriaanm said:
so this is really a triple-whammy bug:

  1. the spec should be updated, as currently I read it as specifying covariant matching for arrays:
    "A parameterized type pattern scala.Array[T1], where T1 is a type pattern.
    This type pattern matches any non-null instance of type scala.Array[U1], where U1 is a type matched by T1."

"where U1 is a type matched by T1" should really read "where a value of type U1 is matched by the type T1"
(this is consistent with the uses of "a value matched by a type" in the other bullets of 8.2)

  1. implement invariant matching for arrays (including updating the analyses)

  2. implement support for bounded existentials in type patterns

I agree all of these are worthwhile, but I don't think I can do this in time for 2.10.0.
Thus, demoting to major.

@scabug
Copy link
Author

@scabug scabug commented Oct 15, 2013

@gkossakowski said:
Unassigning and rescheduling to M7 as previous deadline was missed.

@scabug scabug added this to the Backlog milestone Apr 7, 2017
@SethTisue SethTisue removed the blocker label Jul 19, 2017
@adriaanm adriaanm removed their assignment Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.