-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix 2.13.7 regression affecting wildcards and F-bounded types #9806
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
Conversation
- All existentially bound skolems are replaced with wildcards - Annotation types are checked deeply - Nesting of `@uncheckedBounds` is handled properly
try tpe.mapOver(this).filterAnnotations(_.symbol != UncheckedBoundsClass) | ||
finally skipBounds = savedSkipBounds | ||
case tpe: TypeRef => | ||
checkTypeRef(ExistentialToWildcard(tpe)) |
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.
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?
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 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?
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.
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.
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 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]
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.
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.
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 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.
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.
Really nice cleanup, thanks a lot!
community build likes it, on both 2.12 and 2.13 |
RefCheck types uniformly - handle existentials and annotations
@uncheckedBounds
is handled properlyfixes scala/bug#12481