From 4e4709a3b0dd869b98c1a8d854f4a54145ade2ff Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 25 Nov 2015 14:53:50 +1000 Subject: SI-9567 Fix latent bugs in patmat's reasoning about mutability 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. --- .../nsc/transform/patmat/MatchTranslation.scala | 18 +++++++++++--- test/files/run/t9567c.scala | 29 ++++++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 test/files/run/t9567c.scala diff --git a/src/compiler/scala/tools/nsc/transform/patmat/MatchTranslation.scala b/src/compiler/scala/tools/nsc/transform/patmat/MatchTranslation.scala index 451b72d498..a2afb76b0e 100644 --- a/src/compiler/scala/tools/nsc/transform/patmat/MatchTranslation.scala +++ b/src/compiler/scala/tools/nsc/transform/patmat/MatchTranslation.scala @@ -504,14 +504,26 @@ trait MatchTranslation { */ def treeMaker(binder: Symbol, binderKnownNonNull: Boolean, pos: Position): TreeMaker = { val paramAccessors = binder.constrParamAccessors + val numParams = paramAccessors.length + def paramAccessorAt(subPatIndex: Int) = paramAccessors(math.min(subPatIndex, numParams - 1)) // binders corresponding to mutable fields should be stored (SI-5158, SI-6070) // make an exception for classes under the scala package as they should be well-behaved, // to optimize matching on List + val hasRepeated = paramAccessors.lastOption match { + case Some(x) => definitions.isRepeated(x) + case _ => false + } val mutableBinders = ( if (!binder.info.typeSymbol.hasTransOwner(ScalaPackageClass) && - (paramAccessors exists (_.isMutable))) - subPatBinders.zipWithIndex.collect{ case (binder, idx) if paramAccessors(idx).isMutable => binder } - else Nil + (paramAccessors exists (x => x.isMutable || definitions.isRepeated(x)))) { + + subPatBinders.zipWithIndex.flatMap { + case (binder, idx) => + val param = paramAccessorAt(idx) + if (param.isMutable || (definitions.isRepeated(param) && !aligner.isStar)) binder :: Nil + else Nil + } + } else Nil ) // checks binder ne null before chaining to the next extractor diff --git a/test/files/run/t9567c.scala b/test/files/run/t9567c.scala new file mode 100644 index 0000000000..560bea8821 --- /dev/null +++ b/test/files/run/t9567c.scala @@ -0,0 +1,29 @@ +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 + } + + 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) + } + + case class CaseSequenceWithVar(var x: Any, as: Int*) + val buffer3 = collection.mutable.Buffer(0, 0) + CaseSequenceWithVar("", buffer3: _*) match { + case CaseSequenceWithVar(_, _, i) => // crashes in 2.11.7 + buffer2(1) = 1 + assert(i == 0, i) + } + } +} -- cgit v1.2.3 From e07d62cdefd5b01734692fc549521e562984ccb0 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 25 Nov 2015 14:53:50 +1000 Subject: SI-9567 Fix pattern match on 23+ param, method local case class 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. --- .../scala/tools/nsc/typechecker/PatternTypers.scala | 2 +- test/files/run/t9567.scala | 18 ++++++++++++++++++ test/files/run/t9567b.scala | 19 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 test/files/run/t9567.scala create mode 100644 test/files/run/t9567b.scala diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala b/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala index a702b3cdf5..f90e61ff92 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala @@ -79,7 +79,7 @@ trait PatternTypers { // do not update the symbol if the tree's symbol's type does not define an unapply member // (e.g. since it's some method that returns an object with an unapply member) val fun = inPlaceAdHocOverloadingResolution(fun0)(hasUnapplyMember) - val caseClass = fun.tpe.typeSymbol.linkedClassOfClass + val caseClass = companionSymbolOf(fun.tpe.typeSymbol.sourceModule, context) val member = unapplyMember(fun.tpe) def resultType = (fun.tpe memberType member).finalResultType def isEmptyType = resultOfMatchingMethod(resultType, nme.isEmpty)() diff --git a/test/files/run/t9567.scala b/test/files/run/t9567.scala new file mode 100644 index 0000000000..69896b8650 --- /dev/null +++ b/test/files/run/t9567.scala @@ -0,0 +1,18 @@ +object Test { + def testMethodLocalCaseClass { + case class MethodLocalWide( + f01: Int, f02: Int, f03: Int, f04: Int, f05: Int, f06: Int, f07: Int, f08: Int, f09: Int, f10: Int, + f11: Int, f12: Int, f13: Int, f14: Int, f15: Int, f16: Int, f17: Int, f18: Int, f19: Int, f20: Int, + f21: Int, f22: Int, f23: Int) + + val instance = MethodLocalWide(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) + val result = instance match { + case MethodLocalWide(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) => true + case _ => false + } + assert(result) + } + def main(args: Array[String]) { + testMethodLocalCaseClass + } +} diff --git a/test/files/run/t9567b.scala b/test/files/run/t9567b.scala new file mode 100644 index 0000000000..88cef0a60e --- /dev/null +++ b/test/files/run/t9567b.scala @@ -0,0 +1,19 @@ +object Test { + def testMethodLocalCaseClass { + object MethodLocalWide + case class MethodLocalWide( + f01: Int, f02: Int, f03: Int, f04: Int, f05: Int, f06: Int, f07: Int, f08: Int, f09: Int, f10: Int, + f11: Int, f12: Int, f13: Int, f14: Int, f15: Int, f16: Int, f17: Int, f18: Int, f19: Int, f20: Int, + f21: Int, f22: Int, f23: Int) + + val instance = MethodLocalWide(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) + val result = instance match { + case MethodLocalWide(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) => true + case _ => false + } + assert(result) + } + def main(args: Array[String]) { + testMethodLocalCaseClass + } +} -- cgit v1.2.3