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.

Sign up for GitHub

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

Fix #11008: Support generic tuples as a valid unapply result #14434

Merged
merged 3 commits into from
Feb 14, 2022
Merged

Fix #11008: Support generic tuples as a valid unapply result #14434

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac43086
Fix #11008: Support generic tuples as a valid unapply result
anatoliykmetyuk Feb 8, 2022
51362d8
Support generic tuples as a valid unapplySeq return type
anatoliykmetyuk Feb 10, 2022
69b884d
Add test for the generic tuples as result types of unapplySeq
anatoliykmetyuk Feb 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import collection.mutable
import Symbols._, Contexts._, Types._, StdNames._, NameOps._
import ast.Trees._
import util.Spans._
import typer.Applications.{isProductMatch, isGetMatch, isProductSeqMatch, productSelectors, productArity, unapplySeqTypeElemTp}
import typer.Applications.*
import SymUtils._
import Flags._, Constants._
import Decorators._
Expand Down Expand Up @@ -325,15 +325,16 @@ object PatternMatcher {
def isSyntheticScala2Unapply(sym: Symbol) =
sym.isAllOf(SyntheticCase) && sym.owner.is(Scala2x)

def tupleApp(i: Int, receiver: Tree) = // manually inlining the call to NonEmptyTuple#apply, because it's an inline method
dwijnand marked this conversation as resolved.
Show resolved Hide resolved
ref(defn.RuntimeTuplesModule)
.select(defn.RuntimeTuples_apply)
.appliedTo(receiver, Literal(Constant(i)))
.cast(args(i).tpe.widen)

if (isSyntheticScala2Unapply(unapp.symbol) && caseAccessors.length == args.length)
def tupleSel(sym: Symbol) = ref(scrutinee).select(sym)
def tupleApp(i: Int) = // manually inlining the call to NonEmptyTuple#apply, because it's an inline method
ref(defn.RuntimeTuplesModule)
.select(defn.RuntimeTuples_apply)
.appliedTo(ref(scrutinee), Literal(Constant(i)))
.cast(args(i).tpe.widen)
val isGenericTuple = defn.isTupleClass(caseClass) && !defn.isTupleNType(tree.tpe)
val components = if isGenericTuple then caseAccessors.indices.toList.map(tupleApp) else caseAccessors.map(tupleSel)
val components = if isGenericTuple then caseAccessors.indices.toList.map(tupleApp(_, ref(scrutinee))) else caseAccessors.map(tupleSel)
matchArgsPlan(components, args, onSuccess)
else if (unapp.tpe <:< (defn.BooleanType))
TestPlan(GuardTest, unapp, unapp.span, onSuccess)
Expand All @@ -345,6 +346,9 @@ object PatternMatcher {
.map(ref(unappResult).select(_))
matchArgsPlan(selectors, args, onSuccess)
}
else if unappResult.info <:< defn.NonEmptyTupleTypeRef then
val components = (0 until foldApplyTupleType(unappResult.denot.info).length).toList.map(tupleApp(_, ref(unappResult)))
matchArgsPlan(components, args, onSuccess)
else if (isUnapplySeq && isProductSeqMatch(unapp.tpe.widen, args.length, unapp.srcPos)) {
val arity = productArity(unapp.tpe.widen, unapp.srcPos)
unapplyProductSeqPlan(unappResult, args, arity)
Expand Down
12 changes: 12 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,22 @@ object Applications {
productSelectorTypes(unapplyResult, pos)
// this will cause a "wrong number of arguments in pattern" error later on,
// which is better than the message in `fail`.
else if unapplyResult.derivesFrom(defn.NonEmptyTupleClass) then
foldApplyTupleType(unapplyResult)
else fail
}
}

def foldApplyTupleType(tp: Type)(using Context): List[Type] =
object tupleFold extends TypeAccumulator[List[Type]]:
override def apply(accum: List[Type], t: Type): List[Type] =
t match
case AppliedType(tycon, x :: x2 :: Nil) if tycon.typeSymbol == defn.PairClass =>
apply(x :: accum, x2)
case x => foldOver(accum, x)
end tupleFold
tupleFold(Nil, tp).reverse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I overshot by thinking we should do a fold here. I think that's the cause of the failure. We should just traverse the type structure and do the various unwrapping calls ourselves, e.g dealiasing, unwrapping annotations, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the fold does the right thing here, the problem is that we didn't special-case for unapplySeq. I added a commit to do so, let's see if it breaks anything else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually it might not be the fold. But it's nothing to do with unapplySeq. We just didn't carry over the && !isUnapplySeq that was also present for the product branch.

Moving it has the same affect (though it keeps it separate from the product match). But that unapplySeq change isn't right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? Shouldn't we also support generic tuples as a return type of unapplySeq, and shouldn't this be special-cased?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of unapplySeq is to define an extractor that can do runtime checks on the length of the sequence and operate on it (apply/drop/toSeq). But generic tuples, like all tuples and case class (i.e. all products), have a known length and we consume them as a products or product-like, not as a sequence, by directly accessing their components.

What use case were you thinking of?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g.:

scala> object Foo:
     |   def unapplySeq(x: String): (Int, Seq[String]) = (x.length, x.toList.map(_.toString))

scala> "foo" match
     |   case Foo(1, c) => "One character " + c
     |   case Foo(x, xs*) => s"Many $x characters $xs"

We might want to use a generic tuple there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. But if you want to support it you should add tests that it actually works.


def wrapDefs(defs: mutable.ListBuffer[Tree], tree: Tree)(using Context): Tree =
if (defs != null && defs.nonEmpty) tpd.Block(defs.toList, tree) else tree

Expand Down
2 changes: 2 additions & 0 deletions tests/run/i11008.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
hello
hello and 10
13 changes: 13 additions & 0 deletions tests/run/i11008.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
object A:
def unapply(s: String): String *: EmptyTuple = Tuple1(s)

object B:
def unapply(s:String): String *: Int *: EmptyTuple = Tuple2(s, 10)

@main def Test =
"hello" match
case A(x) =>
println(x)
"hello" match
case B(x, y) =>
println(s"$x and $y")