From e9b154639478e9dbd8447e1585ecb1a6873fb08c Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Fri, 29 Jan 2021 20:58:17 +0000 Subject: [PATCH 1/2] Test size=23 unrelated/redefined unapplies Just recording the current behaviour, to confirm the next commit doesn't regress them. --- .../neg/case-class-23-unrelated-unapply.check | 7 +++ .../neg/case-class-23-unrelated-unapply.scala | 47 +++++++++++++++++++ .../run/case-class-23-redefined-unapply.check | 1 + .../run/case-class-23-redefined-unapply.scala | 35 ++++++++++++++ 4 files changed, 90 insertions(+) create mode 100644 test/files/neg/case-class-23-unrelated-unapply.check create mode 100644 test/files/neg/case-class-23-unrelated-unapply.scala create mode 100644 test/files/run/case-class-23-redefined-unapply.check create mode 100644 test/files/run/case-class-23-redefined-unapply.scala diff --git a/test/files/neg/case-class-23-unrelated-unapply.check b/test/files/neg/case-class-23-unrelated-unapply.check new file mode 100644 index 000000000000..508dd6af77fb --- /dev/null +++ b/test/files/neg/case-class-23-unrelated-unapply.check @@ -0,0 +1,7 @@ +case-class-23-unrelated-unapply.scala:42: error: too many arguments for unapply pattern, maximum = 22 + val TwentyThree(a, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, b) = x + ^ +case-class-23-unrelated-unapply.scala:45: error: too many arguments for unapply pattern, maximum = 22 + val TwentyThree(a2, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, b2) = arr + ^ +2 errors diff --git a/test/files/neg/case-class-23-unrelated-unapply.scala b/test/files/neg/case-class-23-unrelated-unapply.scala new file mode 100644 index 000000000000..93759b3b0065 --- /dev/null +++ b/test/files/neg/case-class-23-unrelated-unapply.scala @@ -0,0 +1,47 @@ +case class TwentyThree( + _1: Int, + _2: Int, + _3: Int, + _4: Int, + _5: Int, + _6: Int, + _7: Int, + _8: Int, + _9: Int, + _10: Int, + _11: Int, + _12: Int, + _13: Int, + _14: Int, + _15: Int, + _16: Int, + _17: Int, + _18: Int, + _19: Int, + _20: Int, + _21: Int, + _22: Int, + _23: Int +) + +object TwentyThree { + def unapply(a: Array[Int]): Option[TwentyThree] = { + Option.when(a.length == 23) { + TwentyThree( + a(0), a(1), a(2), a(3), a(4), a(5), a(6), a(7), a(8), a(9), + a(10), a(11), a(12), a(13), a(14), a(15), a(16), a(17), a(18), a(19), + a(20), a(21), a(22), + ) + } + } +} + +// This is borked.. but I'm fairly certain it's borked since before I started meddling with it.. +object Test extends App { + val x = new TwentyThree(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23) + val TwentyThree(a, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, b) = x + println((a, b)) + val arr = Array(0 , 1 , 2 , 3 , 4 , 5 , 6 , 7 , 8 , 9 , 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22) + val TwentyThree(a2, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, b2) = arr + println((a2, b2)) +} diff --git a/test/files/run/case-class-23-redefined-unapply.check b/test/files/run/case-class-23-redefined-unapply.check new file mode 100644 index 000000000000..99a45a1c9126 --- /dev/null +++ b/test/files/run/case-class-23-redefined-unapply.check @@ -0,0 +1 @@ +(1,3) diff --git a/test/files/run/case-class-23-redefined-unapply.scala b/test/files/run/case-class-23-redefined-unapply.scala new file mode 100644 index 000000000000..205c9abd3b93 --- /dev/null +++ b/test/files/run/case-class-23-redefined-unapply.scala @@ -0,0 +1,35 @@ +case class TwentyThree( + _1: Int, + _2: Int, + _3: Int, + _4: Int, + _5: Int, + _6: Int, + _7: Int, + _8: Int, + _9: Int, + _10: Int, + _11: Int, + _12: Int, + _13: Int, + _14: Int, + _15: Int, + _16: Int, + _17: Int, + _18: Int, + _19: Int, + _20: Int, + _21: Int, + _22: Int, + _23: Int +) + +object TwentyThree { + def unapply(x: TwentyThree): Some[(Int, Int, Int)] = Some(x._1, x._2, x._3) +} + +object Test extends App { + val x = new TwentyThree(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23) + val TwentyThree(a, 2, b) = x + println((a, b)) +} From 0b8dd2a30edccab55ebd810eaf44cd1c3626d22b Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Thu, 28 Jan 2021 22:58:04 +0000 Subject: [PATCH 2/2] Teach typedConstructorPattern to ignore non-redefining unapplies In pos/t12250.scala Foo defines an unapply which doesn't redefine the synthetic one. Thus unapply is overloaded. In the pattern being typed, the synthetic one is indeed the one seeked, so despite being overloaded, we should convertToCaseConstructor. --- .../tools/nsc/typechecker/PatternTypers.scala | 22 +++++++++++++----- test/files/pos/t12250.scala | 11 +++++++++ test/files/pos/t12250b.scala | 23 +++++++++++++++++++ 3 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 test/files/pos/t12250.scala create mode 100644 test/files/pos/t12250b.scala diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala b/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala index 74bfd0491a14..465dc03332e4 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala @@ -83,6 +83,21 @@ trait PatternTypers { val member = unapplyMember(fun.tpe) def resultType = (fun.tpe memberType member).finalResultType def isEmptyType = resultOfIsEmpty(resultType) + + def useConstructor = ( + // Dueling test cases: pos/overloaded-unapply.scala, run/case-class-23.scala, pos/t5022.scala + // Use the case class constructor if (after canElide + isCase) the unapply method: + // (1) doesn't exist, e.g. case classes with 23+ params. run/case-class-23.scala + // (2) is the synthetic case class one, i.e. not user redefined. pos/t11252.scala + // (3a) is overloaded and the synthetic case class one is still present (i.e. not suppressed) pos/t12250.scala + // (3b) the scrutinee type is the case class (not a subtype). pos/overloaded-unapply.scala vs pos/t12250b.scala + canElide && caseClass.isCase && ( + member == NoSymbol // (1) + || member.isSynthetic // (2) + || (member.alternatives.exists(_.isSynthetic) && caseClass.tpe =:= pt) // (3a)(3b) + ) + ) + def isOkay = ( resultType.isErroneous || (resultType <:< BooleanTpe) @@ -94,12 +109,7 @@ trait PatternTypers { // if we're already failing, no need to emit another error here if (fun.tpe.isErroneous) fun - // Dueling test cases: pos/overloaded-unapply.scala, run/case-class-23.scala, pos/t5022.scala - // A case class with 23+ params has no unapply method. - // A case class constructor may be overloaded with unapply methods in the companion. - // A case class maybe have its own custom unapply (so non-synthetic) scala/bug#11252 - // Unapply methods aren't `isCaseApplyOrUnapply` in Scala 3 tasty/run/src-2/tastytest/TestColour.scala - else if (canElide && caseClass.isCase && !member.isOverloaded && (member == NoSymbol || member.isSynthetic)) + else if (useConstructor) logResult(s"convertToCaseConstructor($fun, $caseClass, pt=$pt)")(convertToCaseConstructor(fun, caseClass, pt)) else if (!reallyExists(member)) CaseClassConstructorError(fun, s"${fun.symbol} is not a case class, nor does it have a valid unapply/unapplySeq member") diff --git a/test/files/pos/t12250.scala b/test/files/pos/t12250.scala new file mode 100644 index 000000000000..ec5be8411860 --- /dev/null +++ b/test/files/pos/t12250.scala @@ -0,0 +1,11 @@ +// scalac: -Werror +final case class Foo(value: String) + +object Foo { + def unapply(str: String): Option[Foo] = Some(Foo(str)) + + def extract(id: Foo): String = + id match { + case Foo(a) => a + } +} diff --git a/test/files/pos/t12250b.scala b/test/files/pos/t12250b.scala new file mode 100644 index 000000000000..b47977512fd7 --- /dev/null +++ b/test/files/pos/t12250b.scala @@ -0,0 +1,23 @@ +// scalac: -Werror + +sealed case class Sub1(str: String) +final case class Sup1(str: String) extends Sup0 + +final class Sub2 extends Sub1("") +sealed trait Sup0 { def str: String } + +// both of these unapplies are overloads of the synthetic unapply +// i.e. it isn't suppressed +object Sub1 { def unapply(x: Sub2): Some[String] = Some(x.str) } +object Sup1 { def unapply(x: Sup0): Some[String] = Some(x.str) } + +object Test { + // these seek the original unapplies and should be converted to use their constructors + def testSub1(x: Sub1) = x match { case Sub1(str) => str } + def testSup1(x: Sup1) = x match { case Sup1(str) => str } + + // these seek the user-defined alternative unapplies + // thus they shouldn't accidentally be converted to use their constructors + def testSub2(x: Sub2) = x match { case Sub1(str) => str } + def testSup0(x: Sup0) = x match { case Sup1(str) => str } +}