Skip to content
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

Refine GADT casts on singletons #11987

Closed
wants to merge 2 commits into from
Closed
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
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/core/CheckRealizable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class CheckRealizable(using Context) {
case tp =>
def isConcrete(tp: Type): Boolean = tp.dealias match {
case tp: TypeRef => tp.symbol.isClass
case tp: TypeParamRef => false
case tp: TypeProxy => isConcrete(tp.underlying)
case tp: AndType => isConcrete(tp.tp1) && isConcrete(tp.tp2)
case tp: OrType => isConcrete(tp.tp1) && isConcrete(tp.tp2)
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,7 @@ class Definitions {
@tu lazy val ParamMetaAnnot: ClassSymbol = requiredClass("scala.annotation.meta.param")
@tu lazy val SetterMetaAnnot: ClassSymbol = requiredClass("scala.annotation.meta.setter")
@tu lazy val ShowAsInfixAnnot: ClassSymbol = requiredClass("scala.annotation.showAsInfix")
@tu lazy val StableAnnot: ClassSymbol = requiredClass("scala.annotation.internal.Stable")
@tu lazy val FunctionalInterfaceAnnot: ClassSymbol = requiredClass("java.lang.FunctionalInterface")
@tu lazy val TargetNameAnnot: ClassSymbol = requiredClass("scala.annotation.targetName")
@tu lazy val VarargsAnnot: ClassSymbol = requiredClass("scala.annotation.varargs")
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ object Types {
case _: SingletonType | NoPrefix => true
case tp: RefinedOrRecType => tp.parent.isStable
case tp: ExprType => tp.resultType.isStable
case tp: AnnotatedType => tp.parent.isStable
case tp: AnnotatedType => tp.annot.symbol == defn.StableAnnot || tp.parent.isStable
case tp: AndType =>
// TODO: fix And type check when tp contains type parames for explicit-nulls flow-typing
// see: tests/explicit-nulls/pos/flow-stable.scala.disabled
Expand Down
22 changes: 20 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3530,12 +3530,30 @@ class Typer extends Namer
typr.println(i"adapt to subtype ${tree.tpe} !<:< $pt")
//typr.println(TypeComparer.explained(tree.tpe <:< pt))
adaptToSubType(wtp)
case CompareResult.OKwithGADTUsed if pt.isValueType =>
case CompareResult.OKwithGADTUsed
if pt.isValueType
&& !inContext(ctx.fresh.setGadt(EmptyGadtConstraint)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abgruszecki Not sure this is the right criterion. We need a way to make sure that the tree type conforms to the expected type without taking GADT constraints into account.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the intention, then this check makes perfect sense.

(tree.tpe.widenExpr frozen_<:< pt)
// we overshot; a cast is not needed, after all. (this happens a lot. We should
// find out whether we can make GADTused more precise)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems strange that we'd overshoot often. Could it be that this line is a culprit? It should be debuggable reasonably easily, by printing type comparison traces for the subtype check with and without GADT constraints, right at this code line. I'll take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"this" gives me a 404 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

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, that's a possible source since we do GADT reasoning before falling back to fourthTry. I tried to swap the order of the two clauses but that gives inference errors.

.showing(i"false alarm for $tree: ${tree.tpe.widenExpr} vs $pt in ${ctx.source}", gadts)
} =>
// Insert an explicit cast, so that -Ycheck in later phases succeeds.
// I suspect, but am not 100% sure that this might affect inferred types,
// if the expected type is a supertype of the GADT bound. It would be good to come
// up with a test case for this.
tree.cast(pt)
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.StableAnnot))
else conj
else pt
gadts.println(i"insert GADT cast from $tree to $target")
tree.cast(target)
case _ =>
tree
}
Expand Down
4 changes: 4 additions & 0 deletions compiler/test/dotc/pos-test-pickling.blacklist
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,7 @@ i8182.scala

# local lifted value in annotation argument has different position after pickling
i2797a

# GADT cast applied to singleton type difference
i4176-gadt.scala

6 changes: 6 additions & 0 deletions library/src/scala/annotation/internal/Stable.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package scala.annotation.internal

import scala.annotation.Annotation

/** An annotation asserting that the annotated type is stable */
final class Stable() extends Annotation
8 changes: 8 additions & 0 deletions tests/pos/i11220.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import scala.annotation.tailrec
class Context {
type Tree
}

final def loop3[C <: Context](): Unit =
@tailrec
def loop4[A <: C](c: A): c.Tree = loop4(c)
25 changes: 25 additions & 0 deletions tests/pos/i11955.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
object Hello {

sealed abstract class X[+A] {
type This[+A] <: X[A]
def asThis: This[A]
}

class Y[+A] extends X[A] {
override type This[+AA] = Y[AA]
override def asThis: This[A] = this
}

def hackBackToSelf[F[+u] <: X[u], A](f: F[Any])(f2: f.This[A]): F[A] =
f2.asInstanceOf[F[A]]

case class G[F[+u] <: X[u], A](wrapped: F[A]) {

def mapF[F2[+u] <: X[u]](f: F[A] => F2[A]): G[F2, A] =
G[F2, A](f(wrapped))

def test_ko_1: G[F, A] = mapF(ct => hackBackToSelf(ct)(ct.asThis)) // error
def test_ko_2: G[F, A] = mapF[F](ct => hackBackToSelf(ct)(ct.asThis)) // error
def test_ok : G[F, A] = mapF(ct => hackBackToSelf[F, A](ct)(ct.asThis)) // ok
}
}