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

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

Merged
merged 1 commit into from Nov 11, 2021

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Nov 8, 2021

RefCheck types uniformly - handle existentials and annotations

  • All existentially bound skolems are replaced with wildcards
  • Annotation types are checked deeply
  • Nesting of @uncheckedBounds is handled properly

fixes scala/bug#12481

@scala-jenkins scala-jenkins added this to the 2.13.8 milestone Nov 8, 2021
 - All existentially bound skolems are replaced with wildcards
 - Annotation types are checked deeply
 - Nesting of `@uncheckedBounds` is handled properly
@joroKr21 joroKr21 marked this pull request as ready for review November 9, 2021 02:29
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

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.

Copy link
Member

@lrytz lrytz left a 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!

@lrytz lrytz merged commit 5697b87 into scala:2.13.x Nov 11, 2021
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Nov 11, 2021
@joroKr21 joroKr21 deleted the ref-checks branch November 11, 2021 18:20
@SethTisue
Copy link
Member

community build likes it, on both 2.12 and 2.13

@SethTisue SethTisue changed the title RefCheck types uniformly - handle existentials and annotations Fix 2.13.7 regression affecting wildcards and F-bounded types Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
4 participants