Skip to content

Fix GADT casting when typing if expressions #15646

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

Merged
merged 5 commits into from
Jul 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 54 additions & 15 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,17 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
case elsep: untpd.If => isIncomplete(elsep)
case _ => false

// Insert a GADT cast if the type of the branch does not conform
// to the type assigned to the whole if tree.
// This happens when the computation of the type of the if tree
// uses GADT constraints. See #15646.
def gadtAdaptBranch(tree: Tree, branchPt: Type): Tree =
TypeComparer.testSubType(tree.tpe.widenExpr, branchPt) match {
case CompareResult.OKwithGADTUsed =>
insertGadtCast(tree, tree.tpe.widen, branchPt)
case _ => tree
}

val branchPt = if isIncomplete(tree) then defn.UnitType else pt.dropIfProto

val result =
Expand All @@ -1148,7 +1159,16 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
val elsep0 = typed(tree.elsep, branchPt)(using cond1.nullableContextIf(false))
thenp0 :: elsep0 :: Nil
}: @unchecked
assignType(cpy.If(tree)(cond1, thenp1, elsep1), thenp1, elsep1)

val resType = thenp1.tpe | elsep1.tpe
val thenp2 :: elsep2 :: Nil =
(thenp1 :: elsep1 :: Nil) map { t =>
// Adapt each branch to ensure that their types conforms to the
// type assigned to the if tree by inserting GADT casts.
gadtAdaptBranch(t, resType)
}: @unchecked

cpy.If(tree)(cond1, thenp2, elsep2).withType(resType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering whether we should push this logic into assignType(If... But we're already rolling our own here, by assigning the type after calling the tree copier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it seems that the original code is the only callsite of assignType(If... (not fully verified since metals often lies to me ;)). It would be good to push the logic into assignType.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it seems that pushing this logic into TypeAssigner will require us to also refactor the methods related to GADT casts (gadtAdaptBranch and Typer.insertGadtCast) into TypeAssigner. I am not sure whether it is good to put them there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it seems that the original code is the only callsite of assignType(If... (not fully verified since metals often lies to me ;)).

There's also the calls from tpd. The top-level defs If and InlineIf, and the If in the tree copier.

In terms of pushing logic, I was quite surprised to find some not-plain-basic code in TypeAssigner in assignType(Apply..) which is called by innocent-looking tpd.Apply(..) (code to do with substituting type parameters). So now I'm not sure if it's better to continue to hide away your logic here for whenever an If is constructed or copied, or whether it's best to break the convention and not move it... 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abgruszecki do you hold an opinion on this?

Copy link
Contributor Author

@Linyxus Linyxus Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! I will quickly mention it during the lab meeting.
Edit: just realized that I mistaken the time of the lab meeting. I thought Alex was saying that I could raise the topic during the capture calculus meeting (but it is actually on Friday). I guess Alex is actually talking to @dwijnand? :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep yep, I am talking about the Thursday meeting @ 15:00.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should come to the meeting on Thursday and raise it! 😄 (Or I can, np)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be glad to join this lab meeting but Thursday 15:00 this week does not work well for me. :( I will appreciate knowing the results of your discussion on this topic! :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Martin raised a good point: seeing as this involves GADT information, that doesn't survive post typing, there's little sense to put it in a more general place like TypeAssigners: leaving it in Typer makes sense.


def thenPathInfo = cond1.notNullInfoIf(true).seq(result.thenp.notNullInfo)
def elsePathInfo = cond1.notNullInfoIf(false).seq(result.elsep.notNullInfo)
Expand Down Expand Up @@ -3763,20 +3783,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
gadts.println(i"unnecessary GADTused for $tree: ${tree.tpe.widenExpr} vs $pt in ${ctx.source}")
res
} =>
// Insert an explicit cast, so that -Ycheck in later phases succeeds.
// The check "safeToInstantiate" in `maximizeType` works to prevent unsound GADT casts.
val target =
if tree.tpe.isSingleton then
val conj = AndType(tree.tpe, pt)
if tree.tpe.isStable && !conj.isStable then
// this is needed for -Ycheck. Without the annotation Ycheck will
// skolemize the result type which will lead to different types before
// and after checking. See i11955.scala.
AnnotatedType(conj, Annotation(defn.UncheckedStableAnnot))
else conj
else pt
gadts.println(i"insert GADT cast from $tree to $target")
tree.cast(target)
insertGadtCast(tree, wtp, pt)
case _ =>
//typr.println(i"OK ${tree.tpe}\n${TypeComparer.explained(_.isSubType(tree.tpe, pt))}") // uncomment for unexpected successes
tree
Expand Down Expand Up @@ -4207,4 +4214,36 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
EmptyTree
else typedExpr(call, defn.AnyType)

/** Insert GADT cast to target type `pt` on the `tree`
* so that -Ycheck in later phases succeeds.
* The check "safeToInstantiate" in `maximizeType` works to prevent unsound GADT casts.
*/
private def insertGadtCast(tree: Tree, wtp: Type, pt: Type)(using Context): Tree =
val target =
if tree.tpe.isSingleton then
// In the target type, when the singleton type is intersected, we also intersect
// the GADT-approximated type of the singleton to avoid the loss of
// information. See #15646.
val gadtApprox = Inferencing.approximateGADT(wtp)
gadts.println(i"gadt approx $wtp ~~~ $gadtApprox")
val conj =
TypeComparer.testSubType(gadtApprox, pt) match {
case CompareResult.OK =>
// GADT approximation of the tree type is a subtype of expected type under empty GADT
// constraints, so it is enough to only have the GADT approximation.
AndType(tree.tpe, gadtApprox)
case _ =>
// In other cases, we intersect both the approximated type and the expected type.
AndType(AndType(tree.tpe, gadtApprox), pt)
}
if tree.tpe.isStable && !conj.isStable then
// this is needed for -Ycheck. Without the annotation Ycheck will
// skolemize the result type which will lead to different types before
// and after checking. See i11955.scala.
AnnotatedType(conj, Annotation(defn.UncheckedStableAnnot))
else conj
else pt
gadts.println(i"insert GADT cast from $tree to $target")
tree.cast(target)
end insertGadtCast
}
12 changes: 12 additions & 0 deletions tests/pos/gadt-cast-if.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
trait Expr[T]
case class IntExpr() extends Expr[Int]

def flag: Boolean = ???

def foo[T](ev: Expr[T]): Int | T = ev match
case IntExpr() =>
if flag then
val i: T = ???
i
else
(??? : Int)
13 changes: 13 additions & 0 deletions tests/pos/gadt-cast-singleton.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
enum SUB[-A, +B]:
case Refl[S]() extends SUB[S, S]

trait R {
type Data
}
trait L extends R

def f(x: L): x.Data = ???

def g[T <: R](x: T, ev: T SUB L): x.Data = ev match
case SUB.Refl() =>
f(x)
15 changes: 15 additions & 0 deletions tests/pos/i14776-patmat.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
trait T1
trait T2 extends T1

trait Expr[T] { val data: T = ??? }
case class Tag2() extends Expr[T2]

def flag: Boolean = ???

def foo[T](e: Expr[T]): T1 = e match {
case Tag2() =>
flag match
case true => new T2 {}
case false => e.data
}

16 changes: 16 additions & 0 deletions tests/pos/i14776.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
trait T1
trait T2 extends T1

trait Expr[T] { val data: T = ??? }
case class Tag2() extends Expr[T2]

def flag: Boolean = ???

def foo[T](e: Expr[T]): T1 = e match {
case Tag2() =>
if flag then
new T2 {}
else
e.data
}