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

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

pattern matcher allows wrong number of subpatterns in extractor #6675

scabug opened this issue Nov 16, 2012 · 26 comments

Comments

@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
Copy link
Author

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
Copy link
Author

scabug commented Nov 16, 2012

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

@scabug
Copy link
Author

scabug commented Nov 16, 2012

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

@scabug
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

scabug commented Nov 17, 2012

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

@scabug
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

scabug commented Jan 13, 2013

@retronym said:
scala/scala#1888

@scabug
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

scabug commented Dec 16, 2013

@paulp said:
scala/scala#3275

@scabug
Copy link
Author

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
Copy link
Author

scabug commented Jan 14, 2014

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

@scabug
Copy link
Author

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
Copy link
Author

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
Copy link
Author

scabug commented Feb 6, 2014

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

@scabug
Copy link
Author

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
 `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
larsrh pushed a commit to isabelle-prover/mirror-isabelle that referenced this issue May 24, 2021
ssrg-bamboo pushed a commit to seL4/isabelle that referenced this issue May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants