-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #6709: Correlate match types and match expression #8024
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
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 implementation looks good, but we should not lose details in the docs.
val result = typedMatchFinish(tree, sel1, selType, tree.cases, pt) | ||
|
||
/** Extractor for match types hidden behind an AppliedType/MatchAlias */ | ||
object MatchTypeInDisguise { |
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.
This is interesting. Are there opportunities to use this extractor elsewhere?
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 haven't found any, is it ok to leave it nested in here until we found another use of it?
// To check that pattern types correspond we need to type | ||
// check `pat` here and throw away the result. | ||
val gadtCtx: Context = ctx.fresh.setFreshGADTBounds | ||
val pat1 = typedPattern(pat, selType)(gadtCtx) |
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.
Is there a way to avoid the repeated typing of the patterns? It would be nice if there was but maybe that's too complicated.
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 that would be too much spaghetti, we should need pass these GADT contexts and the typed patterns around inside typedMatchFinish
, typedCases
, and zip them with the cases before finally passing them to typedCase
where they are needed...
db77ff4
to
4cb631d
Compare
I agree the commit updating the docs was unreviewable, I've just splitted it into 6 smaller commits with some explanations in the commit message to justify the changes. I will wait for your input on which part of the delete text you would like to preserve before thinking about creating a more details docs page. |
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 believe we need to talk about variable instantiation and termination - these are difficult but central topics. Otherwise the changes look good to me.
@@ -118,7 +118,6 @@ if | |||
Match(S, C1, ..., Cn) can-reduce i, T | |||
``` | |||
and, for `j` in `1..i-1`: `Cj` is disjoint from `Ci`, or else `S` cannot possibly match `Cj`. | |||
See the section on [overlapping patterns](#overlapping-patterns) for an elaboration of "disjoint" and "cannot possibly match". |
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.
Agree we should remove this. But we should nevertheless investigate whether we would gain something interesting if we also analyzed patterns for overlap. If none of the patterns overlap then we can presumably relax or drop the scrutinee / pattern overlap test. Would that gives us more expressivity in practice?
``` | ||
if `Ci = [Xs] => P => T` and there are minimal instantiations `Is` of the type variables `Xs` such that | ||
``` | ||
S <: [Xs := Is] P |
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 problem is that the rewritten version only talks about subtyping, not about type variable instantiation. But instantiation is crucial for understanding what goes on in more complicated examples. Maybe the existing text is not clear enough. Then it needs to be improved, but it should not be dropped outright.
@@ -143,55 +143,6 @@ The third rule states that a match type conforms to its upper bound | |||
|
|||
Within a match type `Match(S, Cs) <: B`, all occurrences of type variables count as covariant. By the nature of the cases `Ci` this means that occurrences in pattern position are contravarant (since patterns are represented as function type arguments). | |||
|
|||
## Handling Termination | |||
|
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.
Agree that the tone of the description is more like an issue and not like a spec. But a spec still has to talk about termination. So the info needs to be given, just formulated differently.
872d052
to
613ca2d
Compare
|
||
## Variance Laws for Match Types | ||
|
||
Within a match type `Match(S, Cs) <: B`, all occurrences of type variables count as covariant. By the nature of the cases `Ci` this means that occurrences in pattern position are contravarant (since patterns are represented as function type arguments). |
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.
@OlivierBlanvillain this formulation does not make any sense to me.
I think you wanted to say something along the lines of:
The type
S
and each typeCi
occur positively in match typeMatch(S, Cs) <: B
— i.e., the match type is covariant inS
andCi
.
Furthermore, I imagine that the match type is contravariant in B
, 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.
This paragraph was not modified by PR, not sure why it shows up in the overall diff... But it appears to be outdated, so thanks for pointing it out!
Furthermore, I imagine that the match type is contravariant in B , right?
The bound isn't used when doing subtyping between two match types, so Match(S, Cs) <: B1
is =:=
to Match(S, Cs) <: B2
forall B1
, B2
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.
Sure!
The bound isn't used when doing subtyping between two match types
It still needs to be considered during variance checks, no? Otherwise I think it could lead to unsoundness.
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 don't see how, do you have an example?
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 could come up with examples, but there does not seem to be an easy way to get a type with mismatched variance into the bound of a match type, at least from user code:
scala> class C[+T] { type A[X] <: T = X match { case Int => Nothing } }
1 |class C[+T] { type A[X] <: T = X match { case Int => Nothing } }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| covariant type T occurs in invariant position in type [X] =
| X match {
| case Int => Nothing
| } <: T of type A
Is there any other way for match type bounds to get picked up, other than by using and upper bound on an abstract type? If not, then I guess there is no soundness problem. But I'd be wary of corner cases.
PS: weirdly, this one works:
class C[+T] { type A[X] <: T = X match { case _ => Nothing } }
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.
Is there any other way for match type bounds to get picked up, other than by using and upper bound on an abstract type?
No, that's the only syntax we have (which is kind of limiting, for example there is no way to specify bounds of nested match types).
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.
Well, of you ever add a first-class syntax for match type bounds, you should remember to check their variance too :^P
The first sentence was wrong, stating that MT reduce to "a number of RHS", whereas the implementation only reduces to a single RHS. This commit also remove the covariant annotation in the Concat example as it's outdated (trying to compile that snippet on master results in "covariant type parameter Xs occurs in invariant position").
That section was a preliminary descrition of what's implemented in this PR, and is essentially subsumed by the section on Dependent typing.
This information is all outdated, the current implementation doesn't check for overlap between patterns. Instead it's looking at how the overlap between the scrutinee type and each pattern, sequentially.
I didn't understand that section on MT reduction, the rewritten version seams to correspond more closely to what's implemented in the compiler. I didn't get to review that text when it was first added to the docs, but here are the parts that are giving me trouble: - Which part of the code implements the "minimal instantiation" as described in the text? - What is `i` in the two "can-reduce" code blocks? Is it related to T/T'? - The "cannot possibly match" relation, which is central to the algorithm, isn't defined anywhere.
"we should investigate whether..." -> doesn't belong to a spec page. Maybe that text should go into an issue?
See https://github.com/lampepfl/dotty/blob/9982f0dced80d96cf5ca01f89ad68d969b9516ba/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L814-L816 Where we check for equality of scrutinees And https://github.com/lampepfl/dotty/blob/9982f0dced80d96cf5ca01f89ad68d969b9516ba/library/src/scala/internal/MatchCase.scala#L5 Where the pattern of a match case is invariant, meaning we also use equality for those.
613ca2d
to
60cfa2d
Compare
This PR adds a simple form of dependent typing by allowing match expressions to be typed as match types when their scrutinee type, pattern types, and body types align. Concretely, this means that we can define a value level counterpart to the
LeafElem
type from the match type documentation, and type check that expression with the match type:This special mode of typing for match expressions is only used when the following conditions are met: (that logic is implemented by the
isMatchTypeShaped
function in Typer.scala)=:=
to their corresponding type patterns in the match typeFollow up work could potentially relax some of these conditions, for instance to support unapply patterns that are equivalent to typed patterns.