-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix #11008: Support generic tuples as a valid unapply result #14434
Conversation
apply(x :: accum, x2) | ||
case x => foldOver(accum, x) | ||
end tupleFold | ||
tupleFold(Nil, tp).reverse |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
8789fd0
to
51362d8
Compare
Note: there is no documentation that product pattern for generic tuples > 22 elements does not need |
On top #14296.
Done during the Issue Spree of 8th February together with @dwijnand and @prolativ