Fix #6047: Implement variance rules for match types #6050
Conversation
That way, we can control that the variance of a case is 0.
The precise error message i3430.scala depends on the order in which files are found. This is OK. But unfortunately it means there's no easy way to write a check file. My (local) testCompilation, CI test, and CI bootstrapped tests all disagree pairwise which two implicit instances are reported as ambiguous. We could invest in a more specialized test that just checks that an ambiguity is reported no matter what the participants. But my patience has run out, so it would be good if someone else who picks this up could do it.
odersky
changed the title
I believe these were lost when we changed to the new SourcePos scheme. At least nobody was setting the `outer` part of a SourcePosition anymore. We now do this at the latest possible stage, when generating MessageContainers in Reporter.
The run-test-picklin.blacklist got a new entry where before and after pickling differ in whether a match type is reduced or not. It's currently not clear how to write pickling tests that are independent of this detail.
Since we don't use function types for cases anymore, we have to insert the `=>` explicitly.
They might be recursive, so we can't blindly expand them for variance checking. Instead use normalize to follow possible reductions.
| result | ||
|
|
||
| def recur(cases: List[Type]): Type = cases match { | ||
| case cas :: cases1 => matchCase(cas).getOrElse(recur(cases1)) |
There was a problem hiding this comment.
Should the inFrozenConstraint be pushed inside? The way this is written, it's possible that matchCase(case1) returns None while still infer new constrains, which will then be visible when calling matchCase(case2) to matchCase(caseN).
There was a problem hiding this comment.
I don't see how that could happen. Once the constraint is frozen is stays so (in the same typeComparer). Which constraints would be computed by matchCase(case1) and propagated to matchCase(case2)?
| variance = -variance | ||
| val restpe = tp.resultType | ||
| val saved = variance | ||
| variance = if (defn.MatchCase.isInstance(restpe)) 0 else -variance |
There was a problem hiding this comment.
Having to do this kind of distinction in TypeMap makes me wonder if reusing LambdaType-s to encode cases of MatchType is the correct approach. Maybe MatchCase should simply be its own independent type...
There was a problem hiding this comment.
There's a lot of machinery associated with LambdaType which would have to be duplicated. So I think what we have is the lesser evil.
| @@ -28,6 +28,7 @@ object Formatting { | |||
| case arg: Showable => | |||
| try arg.show | |||
| catch { | |||
| case ex: CyclicReference => "..." | |||
There was a problem hiding this comment.
Is this going to be visible to the user? Either way we should probably have something more grepable than "..."
There was a problem hiding this comment.
This would kick in if we hit a Cyclic Reference during printing. The idea is that printing should try to do the best it can rather than quitting with an exception. But I'll make clearer that we elided a cyclic reference.
| else foldOver(status, tp) | ||
| else sym.info match { | ||
| case MatchAlias(_) => foldOver(status, tp) | ||
| case TypeAlias(alias) => this(status, alias) |
There was a problem hiding this comment.
It's not very clear to me than the new code for TypeAlias-es is equivalent to the previous one. Is .bounds.hi and .alias the same thing here?
There was a problem hiding this comment.
yes, it's the same thing. The previous code was a bit outdated.
|
|
||
| /** A type constructor for a case in a match type. | ||
| */ | ||
| final abstract class MatchCase[Pat, +Body] |
There was a problem hiding this comment.
Why do we really need a runtime class for that?
There was a problem hiding this comment.
We don't need a runtime class. But it's easiest to get the definition that way. The class will never be referenced at runtime anyway.
| else { | ||
| def fallbackApply(): Expr[Elem[Tup, N]] = nValue match { | ||
| case Some(n) => quoted.QuoteError("index out of bounds: " + n) | ||
| case None => '{dynamicApply($tup, $n)} | ||
| case None => '{dynamicApply($tup, $n)}.asInstanceOf // TODO: check that variance is OK here |
There was a problem hiding this comment.
I think you might be able to avoid the casts by changing the signature of dynamicApply to the following:
def dynamicApply[This <: NonEmptyTuple, N <: Int] (self: This, n: N): Elem[This, N] = {
type Result = Elem[This, N]
...
}
No description provided.