SI-9567 Fix pattern match on 23+ param, method local case class#4862
Merged
lrytz merged 2 commits intoscala:2.11.xfrom Dec 18, 2015
Merged
SI-9567 Fix pattern match on 23+ param, method local case class#4862lrytz merged 2 commits intoscala:2.11.xfrom
lrytz merged 2 commits intoscala:2.11.xfrom
Conversation
Member
Member
Author
|
Looks like a latent bug. If we make the case class in that test case top-level, it also fails under 2.11.7 case class CaseSequenceTopLevel(as: Int*)
object Test {
def main(args: Array[String]): Unit = {
val buffer1 = collection.mutable.Buffer(0, 0)
CaseSequenceTopLevel(buffer1: _*) match {
case CaseSequenceTopLevel(_, i) =>
buffer1(1) = 1
assert(i == 0, i) // fails in 2.11.7 -optimize
}
}Whereas with this patch the failures are uniform for top-level and method-owned case classes. I'll investigate. |
Member
Author
|
I can crash patmat with a variation: case class CaseSequenceTopLevel(var x: Any, as: Int*)
object Test {
def main(args: Array[String]): Unit = {
val buffer1 = collection.mutable.Buffer(0, 0)
CaseSequenceTopLevel("", buffer1: _*) match {
case CaseSequenceTopLevel(_, _, i) =>
buffer1(1) = 1
assert(i == 0, i) // fails in 2.11.7 -optimize
}
// case class CaseSequence(as: Int*)
// val buffer2 = collection.mutable.Buffer(0, 0)
// CaseSequence(buffer2: _*) match {
// case CaseSequence(_, i) =>
// buffer2(1) = 1
// assert(i == 0, i)
// }
}
}This code doesn't account for repeated patterns. |
ceeeb20 to
28ad311
Compare
Under -optimize, the pattern matcher tries to avoid local variables in favour of directly accessing to non-var case class accessors. However, the code that analysed the patterns failed to account properly for repeated parameters, which could either lead to a compiler crash (when assuming that the n-th subpattern must have a corresponding param accessor), or could lead to a correctness problem (when failing to eagerly the bound elements from the sequence.) The test case that tried to cover seems only to have been working because of a separate bug (the primary subject of SI-9567) related to method-local case classes: they were treated during typechecking as extractors, rather than native case classes. The subsequent commit will fix that problem, but first we must pave the way with this commit that emits local vals for bound elements of case class repeated params.
Typechecking constructor patterns of method local case classes was only working because of the existence of the unapply method in the companion, which is used if navigation to the case class companion object fails. We now support defintion of, and pattern matching on, case classes with more than 22 parameters. These have no `unapply` method in the companion, as we don't have a large enough tuple type to return. So for such case classes, the fallback that we inadvertently relied on would no longer save us, and we'd end up with a compile error advising that the identifier in the constructor pattern was neither a case class nor an extractor. This is due to the propensity of `Symbol#companionXxx` to return `NoSymbol` when in the midst of typechecking. That method should only be relied upon after typechecking. During typechecking, `Namers#companionSymbolOf` should be used instead, which looks in the scopes of enclosing contexts for symbol companionship. That's what I've done in this commit.
28ad311 to
e07d62c
Compare
Member
|
LGTM |
lrytz
added a commit
that referenced
this pull request
Dec 18, 2015
SI-9567 Fix pattern match on 23+ param, method local case class
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Typechecking constructor patterns of method local case classes
was only working because of the existence of the unapply method
in the companion, which is used if navigation to the case class
companion object fails.
We now support defintion of, and pattern matching on, case classes
with more than 22 parameters. These have no
unapplymethodin the companion, as we don't have a large enough tuple type to
return. So for such case classes, the fallback that we inadvertently
relied on would no longer save us, and we'd end up with a compile
error advising that the identifier in the constructor pattern was
neither a case class nor an extractor.
This is due to the propensity of
Symbol#companionXxxto returnNoSymbolwhen in the midst of typechecking. That method shouldonly be relied upon after typechecking. During typechecking,
Namers#companionSymbolOfshould be used instead, which looks in thescopes of enclosing contexts for symbol companionship. That's
what I've done in this commit.