pattern matcher allows wrong number of subpatterns in extractor

[scabug]

In addition to matching the contents of a tuple returned by def unapply, scala 2.10 also allows a single pattern matching the tuple itself. This is a regression from 2.9 in terms of pattern verification.

Welcome to Scala version 2.9.2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_37).
Type in expressions to have them evaluated.
Type :help for more information.

scala> object X{def unapply(s: String):Option[(Int,Int,Int)]=Some((1,2,3))}
defined module X

scala> "" match { case X(b) => b }
<console>:9: error: wrong number of arguments for object X
              "" match { case X(b) => b }
                               ^
<console>:9: error: not found: value b
              "" match { case X(b) => b }
                                      ^

Welcome to Scala version 2.10.0-RC2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_37).
Type in expressions to have them evaluated.
Type :help for more information.

scala>  object X{def unapply(s: String):Option[(Int,Int,Int)]=Some((1,2,3))}
defined module X

scala> "" match { case X(b) => b }
res0: (Int, Int, Int) = (1,2,3)

This is problematic because there is no warning (except in certain lucky cases) if argument patterns are forgotten, resulting in a case statement which will never match, e.g. case X(x: SomeTrait).

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-6675?orig=1
Reporter: @rkuhn
Affected Versions: 2.10.0-RC2

[scabug]

@paulp said:
Regressed in scala/scala@ee5721e864 .

[scabug]

@paulp said:
It's not clear to me everyone will agree this is a bug, but I do.

[scabug]

@adriaanm said:
Thanks Paul for digging up the ticket in which I flip-flop and argue (almost) both cases: #6111

I think this is currently as specified, but reasonable people could disagree on this.

[scabug]

@paulp said:
I know why one would want it the way it was; it would be useful to hear why one would want it the way it now is.

[scabug]

@paulp said:
Basically, we kill ourselves by using the same type for wrapping up multiple values as we do for encoding information about an extractor. If you want to say "extractor is arity 1, here is an ((Int, Int))" you are screwed. Possibly enhancements include

def unapplyArity: Int

for those cases where you want them to differ. But the worst scenario is the present one, where the pattern matcher is "flexible" in regards to the arity - maybe it's 1, maybe it's 3, let's see what works. We should be looking for more type safety in the matcher, not less.

[scabug]

@rkuhn said:
Thanks, Paul, that was exactly what I meant to say.

[scabug]

@paulp said:
I find this particularly egregious:

scala> "" match { case X((b)) => b }
res3: (Int, Int, Int) = (1,2,3)

That's a pretty emphatic way to say you expect Tuple1, but the parentheses are discarded and you are happily handed a 3tuple. (Typing out Tuple1(b) correctly fails.)

[scabug]

@Blaisorblade said:
This bug is scheduled for 2.10.1. But that makes little sense.

How's this fix supposed to make its way into 2.10.1, since it will break source code which 2.10 had started accepting? Sure people will start using this, and I'd argue you can't change the pattern matcher behavior in a point release, even more so now that we even have binary compatibility. IOW, source compatibility is to me stricter than binary compatibility (and yes, we need to figure this out once and for all).

Unlike for libraries, it's a bit hard to argue then break their code arguing that this is "unspecified behavior": in general, not reading Scaladocs is not excusable, while ScalaReference is often (and in this case) more complex to read. The specification didn't change between 2.9 and 2.10, but the behavior did; and by looking at the text, it's not a prime example of pedagogical clarity in this matter, especially if you're used to the behavior pre-2.10.

[scabug]

@paulp said:
Can we document it as a known issue in 2.10.0 which will be fixed in 2.10.1? If the policy doesn't already enable that approach, it should be crafted to do so, especially if "timely releases" is something to which we aspire.

[scabug]

@rkuhn said:
Complete agreement from my side: we do not want another RC for this, so it should be documented very prominently that we currently allow some behavior which will be prohibited again in the next point release. “Known issue” describes this pretty well, I think.

[scabug]

@retronym said:
Even if we can't re-introduce a compile error, we could introduce a stern warning in 2.10.1.

[scabug]

@Blaisorblade said:
I would have argued to revert the change and thinking more about it, since there are conflicting language design issues here. To me, "we changed the implemented language but we didn't figure out something quite annoying about it" would be enough for revert, at least before RC time.
I do see your point on timely releases, and documenting it as a known issue sounds good. The confusing part is that the old behavior also gave problems (in #6111), so we might want to leave the future behavior not entirely specified in the release notes, which is quite annoying.

Finally, legalese-speaking, the current spec indeed supports Adriaan; but pedagogically speaking, this behavior is allowed so implicitly that I suspect who wrote the text did not mean to allow this behavior, or simply did a very poor job at explaining it, so that even Adriaan was confused.

[scabug]

@paulp said:
That's the risk of treating the spec as holy writ. A common understanding of it shared by multiple parties should not be treated as an unnecessary luxury.

[scabug]

@Blaisorblade said:
Paul, I think nobody questioned that (yet) in this case - at least I didn't mean to, since I don't like how this part of the spec is written. I raised the point only because unspecified behavior is easier to change (at least for the library), but this clause unfortunately does not apply here.