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.

Sign up for GitHub

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

Jump to bottom

Fix 2.13.7 regression affecting wildcards and F-bounded types #9806

Merged
merged 1 commit into from Nov 11, 2021
Merged

Fix 2.13.7 regression affecting wildcards and F-bounded types #9806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
93 changes: 46 additions & 47 deletions src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
View file
Open in desktop
Expand Up @@ -1407,32 +1407,53 @@ abstract class RefChecks extends Transform {
false
}

private def checkTypeRef(tp: Type, tree: Tree, skipBounds: Boolean): Unit = tp match {
case TypeRef(pre, sym, args) =>
tree match {
case tt: TypeTree if tt.original == null => // scala/bug#7783 don't warn about inferred types
// FIXME: reconcile this check with one in resetAttrs
case _ => checkUndesiredProperties(sym, tree.pos)
private object RefCheckTypeMap extends TypeMap {
object ExistentialToWildcard extends TypeMap {
override def apply(tpe: Type): Type =
if (tpe.typeSymbol.isExistential) WildcardType else tpe.mapOver(this)
}

private[this] var skipBounds = false
private[this] var tree: Tree = EmptyTree

def check(tpe: Type, tree: Tree): Type = {
this.tree = tree
try apply(tpe) finally {
skipBounds = false
this.tree = EmptyTree
}
if (sym.isJavaDefined)
sym.typeParams foreach (_.cookJavaRawInfo())
if (!tp.isHigherKinded && !skipBounds)
checkBounds(tree, pre, sym.owner, sym.typeParams, args)
case _ =>
}
}

private def checkTypeRefBounds(tp: Type, tree: Tree) = {
var skipBounds = false
tp match {
case AnnotatedType(ann :: Nil, underlying) if ann.symbol == UncheckedBoundsClass =>
// check all bounds, except those that are existential type parameters
// or those within typed annotated with @uncheckedBounds
override def apply(tpe: Type): Type = tpe match {
case tpe: AnnotatedType if tpe.hasAnnotation(UncheckedBoundsClass) =>
// scala/bug#7694 Allow code synthesizers to disable checking of bounds for TypeTrees based on inferred LUBs
// which might not conform to the constraints.
val savedSkipBounds = skipBounds
skipBounds = true
underlying
try tpe.mapOver(this).filterAnnotations(_.symbol != UncheckedBoundsClass)
finally skipBounds = savedSkipBounds
case tpe: TypeRef =>
checkTypeRef(ExistentialToWildcard(tpe))
Copy link
Member

Choose a reason for hiding this comment

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

This used to be discarded:

scala> trait T[X <: String]
trait T

scala> class C { def f: T[X forSome { type X <: Int }] = ??? }
                            ^
       warning: the existential type X forSome { type X <: Int }, which cannot be expressed by wildcards, should be enabled
       by making the implicit value scala.language.existentials visible.
                        ^
       error: type arguments [X forSome { type X <: Int }] do not conform to trait T's type parameter bounds [X <: String]

now it passes. OTOH, these two passed before (2.13.6/7):

scala> class C { def f: T[_ <: Int] = ??? }
class C

scala> class C { def f: T[X] forSome { type X <: Int } = ??? }
class C

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I see now, this is basically the oversight in the old impl, which only collected existentials at the top level to replace them with wildcards.

I guess to be more precise, we should only replace existentials with wildcards that were originally actual wildcards in source? But that's not possible to tell?

Copy link
Member Author

@joroKr21 joroKr21 Nov 11, 2021 •
edited

Choose a reason for hiding this comment

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

Ahh I see what you mean - let me think about it some more. We want to convert existential quantifiers that come from the outside to wildcards, but not those that are nested within.

I guess to be more precise, we should only replace existentials with wildcards that were originally actual wildcards in source? But that's not possible to tell?

It's possible to tell, but that actually makes some tests fail. As you noticed existential quantifiers already get a pass when it comes to bounds regardless of whether they are expressed as wildcards or not. Do you think that was intentional? If not, we can make that case fail and it would solve our other problem as well.

Copy link
Member

Choose a reason for hiding this comment

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

I really don't know, I'm just trying to infer stuff from intuition / experience.

To start with the basics, I guess T[_] should always pass for practical reasons? I can't find anything about it in the spec.

wildcard type is of the form _>:𝐿<:𝑈. If >:𝐿 is missing, >:scala.Nothing is assumed, if <:𝑈 is missing, <:scala.Any is assumed. A wildcard type is a shorthand for an existentially quantified type variable

Say the type parameters have lower bounds 𝐿1,…,𝐿𝑛 and upper bounds 𝑈1,…,𝑈𝑛. The parameterized type is well-formed if each actual type parameter conforms to its bounds, i.e. 𝐿𝑖<:𝑇𝑖<:𝑈𝑖

Is checking the bounds required for type safety? Or will mismatching bounds be always flagged in subtype tests?

scala> trait T[X <: String] { def x: X }
trait T

scala> def f(t: T[_ <: Int]) = t.x
def f(t: T[_ <: Int]): Int

scala> def t: T[String] = ???
def t: T[String]

scala> f(t)
         ^
       error: type mismatch;
        found   : T[String]
        required: T[_ <: Int]

Copy link
Member

Choose a reason for hiding this comment

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

Scala 3 also seems to accept wildcards with non-matching bounds

scala> trait T[X <: String]
// defined trait T

scala> class C { def f(x: T[_ <: Int]): T[_ <: Int] = x }
// defined class C

So let's go with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will see if we need a followup for class C { def f: T[X forSome { type X <: Int }] = ??? } because it still looks wrong to me. X forSome { type X <: Int } is not a subtype of String although in T[X] forSome { type X <: Int } we accept X as a subtype of String. Good idea to check Scala 3 - but of course it doesn't have existential types.

tpe.mapOver(this)
case tpe =>
tpe.mapOver(this)
}

private def checkTypeRef(tpe: Type): Unit = tpe match {
case TypeRef(pre, sym, args) =>
if (!tp.isHigherKinded && !skipBounds)
tree match {
// scala/bug#7783 don't warn about inferred types
// FIXME: reconcile this check with one in resetAttrs
case tree: TypeTree if tree.original == null =>
case tree => checkUndesiredProperties(sym, tree.pos)
}
if (sym.isJavaDefined)
sym.typeParams.foreach(_.cookJavaRawInfo())
if (!tpe.isHigherKinded && !skipBounds)
checkBounds(tree, pre, sym.owner, sym.typeParams, args)
tp
case _ =>
tp
}
}

Expand All @@ -1449,8 +1470,7 @@ abstract class RefChecks extends Transform {
def applyChecks(annots: List[AnnotationInfo]): List[AnnotationInfo] = if (annots.isEmpty) Nil else {
annots.foreach { ann =>
checkVarArgs(ann.atp, tree)
checkTypeRef(ann.atp, tree, skipBounds = false)
checkTypeRefBounds(ann.atp, tree)
RefCheckTypeMap.check(ann.atp, tree)
if (ann.original != null && ann.original.hasExistingSymbol)
checkUndesiredProperties(ann.original.symbol, tree.pos)
}
Expand Down Expand Up @@ -1755,29 +1775,8 @@ abstract class RefChecks extends Transform {
}
}

val existentialParams = new ListBuffer[Symbol]
var skipBounds = false
// check all bounds, except those that are existential type parameters
// or those within typed annotated with @uncheckedBounds
if (!inPattern) tree.tpe foreach {
case tp @ ExistentialType(tparams, tpe) =>
existentialParams ++= tparams
case ann: AnnotatedType if ann.hasAnnotation(UncheckedBoundsClass) =>
// scala/bug#7694 Allow code synthesizers to disable checking of bounds for TypeTrees based on inferred LUBs
// which might not conform to the constraints.
skipBounds = true
case tp: TypeRef =>
val tpWithWildcards = deriveTypeWithWildcards(existentialParams.toList)(tp)
checkTypeRef(tpWithWildcards, tree, skipBounds)
case _ =>
}
if (skipBounds) {
tree.setType(tree.tpe.map {
_.filterAnnotations(_.symbol != UncheckedBoundsClass)
})
}

tree
if (inPattern) tree
else tree.setType(RefCheckTypeMap.check(tree.tpe, tree))

case TypeApply(fn, args) =>
checkBounds(tree, NoPrefix, NoSymbol, fn.tpe.typeParams, args map (_.tpe))
Expand Down Expand Up @@ -1812,8 +1811,8 @@ abstract class RefChecks extends Transform {
case x @ Select(_, _) =>
transformSelect(x)

case Literal(Constant(tp: Type)) =>
checkTypeRef(tp, tree, skipBounds = false)
case Literal(Constant(tpe: Type)) =>
RefCheckTypeMap.check(tpe, tree)
tree

case UnApply(fun, args) =>
Expand Down
7 changes: 7 additions & 0 deletions test/files/neg/ref-checks.check
View file
Open in desktop
@@ -0,0 +1,7 @@
ref-checks.scala:8: error: type arguments [Int] do not conform to trait Chars's type parameter bounds [A <: CharSequence]
@ann[Chars[Int]] val x = 42
^
ref-checks.scala:9: error: type arguments [Double] do not conform to trait Chars's type parameter bounds [A <: CharSequence]
val y: Two[Chars[Long] @uncheckedBounds, Chars[Double]] = null
^
2 errors
10 changes: 10 additions & 0 deletions test/files/neg/ref-checks.scala
View file
Open in desktop
@@ -0,0 +1,10 @@
import scala.annotation.StaticAnnotation
import scala.reflect.internal.annotations.uncheckedBounds

object Test {
trait Chars[A <: CharSequence]
trait Two[A, B]
class ann[A] extends StaticAnnotation
@ann[Chars[Int]] val x = 42
val y: Two[Chars[Long] @uncheckedBounds, Chars[Double]] = null
}
2 changes: 2 additions & 0 deletions test/files/run/t12481.check
View file
Open in desktop
@@ -0,0 +1,2 @@
Test$Universe[_ <: Any]
Test$Universe[<?>]
6 changes: 6 additions & 0 deletions test/files/run/t12481.scala
View file
Open in desktop
@@ -0,0 +1,6 @@
object Test extends App {
trait Txn[T <: Txn[T]]
trait Universe[T <: Txn[T]]
println(implicitly[Manifest[Universe[_]]])
println(implicitly[OptManifest[Universe[_]]])
}