-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #6047: Implement variance rules for match types #6050
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
Changes from all commits
da4a9ee
8d6389a
f23275b
75481ef
59054b0
16e21d9
62ffcd5
64d613b
d86b09d
d24b6f2
11a8fad
33d3622
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3756,7 +3756,7 @@ object Types { | |
|
||
def caseType(tp: Type)(implicit ctx: Context): Type = tp match { | ||
case tp: HKTypeLambda => caseType(tp.resType) | ||
case defn.FunctionOf(_, restpe, _, _) => restpe | ||
case defn.MatchCase(_, body) => body | ||
} | ||
|
||
def alternatives(implicit ctx: Context): List[Type] = cases.map(caseType) | ||
|
@@ -4417,10 +4417,12 @@ object Types { | |
|
||
case tp: LambdaType => | ||
def mapOverLambda = { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having to do this kind of distinction in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
val ptypes1 = tp.paramInfos.mapConserve(this).asInstanceOf[List[tp.PInfo]] | ||
variance = -variance | ||
derivedLambdaType(tp)(ptypes1, this(tp.resultType)) | ||
variance = saved | ||
derivedLambdaType(tp)(ptypes1, this(restpe)) | ||
} | ||
mapOverLambda | ||
|
||
|
@@ -4440,7 +4442,9 @@ object Types { | |
derivedOrType(tp, this(tp.tp1), this(tp.tp2)) | ||
|
||
case tp: MatchType => | ||
derivedMatchType(tp, this(tp.bound), this(tp.scrutinee), tp.cases.mapConserve(this)) | ||
val bound1 = this(tp.bound) | ||
val scrut1 = atVariance(0)(this(tp.scrutinee)) | ||
derivedMatchType(tp, bound1, scrut1, tp.cases.mapConserve(this)) | ||
|
||
case tp: SkolemType => | ||
derivedSkolemType(tp, this(tp.info)) | ||
|
@@ -4804,10 +4808,12 @@ object Types { | |
case _: BoundType | _: ThisType => x | ||
|
||
case tp: LambdaType => | ||
variance = -variance | ||
val restpe = tp.resultType | ||
val saved = variance | ||
variance = if (defn.MatchCase.isInstance(restpe)) 0 else -variance | ||
val y = foldOver(x, tp.paramInfos) | ||
variance = -variance | ||
this(y, tp.resultType) | ||
variance = saved | ||
this(y, restpe) | ||
|
||
case tp: TermRef => | ||
if (stopAtStatic && tp.currentSymbol.isStatic || (tp.prefix `eq` NoPrefix)) x | ||
|
@@ -4835,7 +4841,9 @@ object Types { | |
this(this(x, tp.tp1), tp.tp2) | ||
|
||
case tp: MatchType => | ||
foldOver(this(this(x, tp.bound), tp.scrutinee), tp.cases) | ||
val x1 = this(x, tp.bound) | ||
val x2 = atVariance(0)(this(x1, tp.scrutinee)) | ||
foldOver(x2, tp.cases) | ||
|
||
case AnnotatedType(underlying, annot) => | ||
this(applyToAnnot(x, annot), underlying) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,7 +34,7 @@ object VarianceChecker { | |
val paramVarianceStr = if (v == 0) "contra" else "co" | ||
val occursStr = variance match { | ||
case -1 => "contra" | ||
case 0 => "non" | ||
case 0 => "in" | ||
case 1 => "co" | ||
} | ||
val pos = tree.tparams | ||
|
@@ -123,18 +123,19 @@ class VarianceChecker()(implicit ctx: Context) { | |
def apply(status: Option[VarianceError], tp: Type): Option[VarianceError] = trace(s"variance checking $tp of $base at $variance", variances) { | ||
try | ||
if (status.isDefined) status | ||
else tp match { | ||
else tp.normalized match { | ||
case tp: TypeRef => | ||
val sym = tp.symbol | ||
if (sym.variance != 0 && base.isContainedIn(sym.owner)) checkVarianceOfSymbol(sym) | ||
else if (sym.isAliasType) this(status, sym.info.bounds.hi) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not very clear to me than the new code for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it's the same thing. The previous code was a bit outdated. |
||
case _ => foldOver(status, tp) | ||
} | ||
case tp: MethodOrPoly => | ||
this(status, tp.resultType) // params will be checked in their TypeDef or ValDef nodes. | ||
case AnnotatedType(_, annot) if annot.symbol == defn.UncheckedVarianceAnnot => | ||
status | ||
case tp: MatchType => | ||
apply(status, tp.bound) | ||
case tp: ClassInfo => | ||
foldOver(status, tp.classParents) | ||
case _ => | ||
|
@@ -179,7 +180,7 @@ class VarianceChecker()(implicit ctx: Context) { | |
sym.is(PrivateLocal) || | ||
sym.name.is(InlineAccessorName) || // TODO: should we exclude all synthetic members? | ||
sym.is(TypeParam) && sym.owner.isClass // already taken care of in primary constructor of class | ||
tree match { | ||
try tree match { | ||
case defn: MemberDef if skip => | ||
ctx.debuglog(s"Skipping variance check of ${sym.showDcl}") | ||
case tree: TypeDef => | ||
|
@@ -196,6 +197,9 @@ class VarianceChecker()(implicit ctx: Context) { | |
vparamss foreach (_ foreach traverse) | ||
case _ => | ||
} | ||
catch { | ||
case ex: TypeError => ctx.error(ex.toMessage, tree.sourcePos.focus) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
scala> val tuple = (1, "2", 3L) | ||
val tuple: (Int, String, Long) = (1,2,3) | ||
scala> 0.0 *: tuple | ||
val res0: Double *: (Int, String, Long)(tuple) = (0.0,1,2,3) | ||
val res0: (Double, Int, String, Long) = (0.0,1,2,3) | ||
scala> tuple ++ tuple | ||
val res1: Int *: String *: Long *: | ||
scala.Tuple.Concat[Unit, (Int, String, Long)(tuple)] = (1,2,3,1,2,3) |
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.
Should the
inFrozenConstraint
be pushed inside? The way this is written, it's possible thatmatchCase(case1)
returnsNone
while still infer new constrains, which will then be visible when callingmatchCase(case2)
tomatchCase(caseN)
.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 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 tomatchCase(case2)
?