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 matcher allows wrong number of subpatterns in extractor #6675

Closed
scabug opened this Issue Nov 16, 2012 · 26 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented Nov 16, 2012

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

This comment has been minimized.

Copy link

scabug commented Nov 16, 2012

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

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 16, 2012

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

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 16, 2012

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

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 17, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 17, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 17, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 17, 2012

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

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 17, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 25, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 25, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 25, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 25, 2012

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

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 25, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 25, 2012

@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

This comment has been minimized.

Copy link

scabug commented Nov 25, 2012

@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.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 13, 2013

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@retronym said:
Marking as fixed, even though we require you to compile with -Xlint to be notified about this.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@paulp said:
Let's not do that; I don't care what the spec says, I think this is a bug, and notification is insufficient.

@scabug

This comment has been minimized.

Copy link

scabug commented Apr 19, 2013

@retronym said:
The warning seems to overstretch, only the first should warn below:

scala> object LeftOrRight {
     |   def unapply[A](value: Either[A, A]): Option[A] = value match {
     |     case scala.Left(x) => Some(x)
     |     case scala.Right(x) => Some(x)
     |   }
     | }
defined module LeftOrRight

scala> (Left((0, 0)): Either[(Int, Int), (Int, Int)]) match { case LeftOrRight(a) => a  }
<console>:9: warning: extractor pattern binds a single value to a Product2 of type (Int, Int)
              (Left((0, 0)): Either[(Int, Int), (Int, Int)]) match { case LeftOrRight(a) => a  }
                                                                          ^
res0: (Int, Int) = (0,0)

scala> (Left((0, 0)): Either[(Int, Int), (Int, Int)]) match { case LeftOrRight((a, b)) => a  }
<console>:9: warning: extractor pattern binds a single value to a Product2 of type (Int, Int)
              (Left((0, 0)): Either[(Int, Int), (Int, Int)]) match { case LeftOrRight((a, b)) => a  }
                                                                          ^
res1: Int = 0
@scabug

This comment has been minimized.

Copy link

scabug commented Dec 16, 2013

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 14, 2014

@retronym said:
Reopening; even under Paul's latest patch this is still only a lint warning, which was insufficient grounds to close this ticket previously.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 14, 2014

@paulp said:
Insufficient grounds according to me, back then. These days, I have no standards. Close away.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 20, 2014

@paulp said:
Since this is still assigned to me, I'm calling it fixed. If anyone wants to reopen please assume ownership of it.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 5, 2014

@adriaanm said:
Since we failed to address this early enough in 2.11, the best I can do is to make this a deprecation warning. Marking as blocker.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 6, 2014

@rkuhn said:
A deprecation warning is sure better than nothing, +1 from me.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 20, 2014

@adriaanm said:
Deprecation warning: scala/scala#3564

@scabug scabug added the has PR label Apr 7, 2017

@scabug scabug added this to the 2.11.0-RC1 milestone Apr 7, 2017

zimmermatt added a commit to zimmermatt/atlas that referenced this issue Sep 14, 2018

Address compilation warning.
 `case extractor(extracted) => extracted`

yields

 ```
 Warning:(68, 16) class ValueExtractor expects 2 patterns to hold
 (String, String) but crushing into 2-tuple to fit single pattern
 (scala/bug#6675)
 ```

See
* scala/bug#6675
* scala/bug#6111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment