-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Check for mismatched argument type length in PatternMatcher due to Named Tuple :* syntax #23602
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
Conversation
…med-Tuple :* syntax
This makes the behaviour of auto-untupling consistent between the usual and generic tuples. Related to scala#23602
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.
Catching the issue in the PatternMatcher is a bit unideal, though much better than crashing of course.
I spent a few hours trying to figure out where things go wrong in Applications
, i.e. why the Typer outputs a tree without Tuple2.unapply
and without error.
I didn't really reach anything, but commenting the following for reference.
As far as un-named generic tuples are concerned (tests/neg/i23155b.scala), I believe we can fix auto-untupling by modifying Applications.typedPatterns
to also allow argType.isGenericTuple
at:
&& defn.isTupleNType(argType) => |
However, I'm not sure I really understood the logic for auto-untupling named tuples.
It seems like it would become ambiguous pretty quickly, e.g.:
case class C(a: (a: Int, b: Int))
C(???) match
case C((a = x, b = y)) => ??? // ok, no auto-untupling
case C(x, y) => ??? // ok, auto-tupling due to length constraints
case C(a = x) => ??? // presumably `a` is the outer-one and x has type (a: Int, b: Int)
case C(b = y) => ??? // inner one?
case C(a = x, b = y) => ??? // inner ones due to length constraints?
From an implementation perspective, the point from where named and unnamed tuples take a different code path appears to be in productUnapplySelectors
as @aherlihy reported.
Specifically, we may be missing some named-tuple equivalent of the if isProductMatch(tp, args1.length, pos)
guard from the unnamed case.
case Some(args1) if isProductMatch(tp, args1.length, pos) => |
Fixes #23155
After discussing at the compiler meeting, it was decided that the Named Tuple behavior should be the same as the Tuple behavior with respect to pattern matching with the
:*
syntax. To error in the exact same way, it should be caught here:scala3/compiler/src/dotty/tools/dotc/typer/Applications.scala
Line 291 in 00d19df
For regular tuples,
argTypes
is anAppliedType
of:*
and is not yet reduced, so has length 1, so the arity check will fail. However, at this point Named Tuple types are fully reduced bytupleElementTypes
here:scala3/compiler/src/dotty/tools/dotc/typer/Applications.scala
Line 249 in 00d19df
So for Named Tuples,
argTypes
at the time of the check will be(Int, Int)
so the arity check will pass, causing a later crash in PatternMatcher here:scala3/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala
Line 270 in 6146b90
We cannot un-reduce or fail to reduce for Named Tuple types because it will cause a host of other tests to fail that rely on the fully reduced Named Tuple-value types. For now I have added a check in PatternMatcher because it’s better to fix the compiler crash ASAP, but I think we could revisit if we want something like
Int :* Int :* EmptyTuple
to really behave differently from(Int, Int)
here, since pattern matching currently works for both Tuples and Named Tuples using(Int, Int)
orTuple2[Int, Int]
, just not:*
.