-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Don't GLB binders of type patterns, use the type directly #10247
Conversation
In type patterns `c match { case x: T }`, the translation would assign the GLB of `c`'s type and `T` to the varaible `x`. This seems to trace back to the first version of the "virtual pattern matcher". I could not find a similar use of `glb` in that revision of the codebase. So I'm not sure if it was a new addition, or picked up from the previous implementation. https://github.com/scala/scala/blob/8a9fd64129926eea35f7dca181242855f14e153f/src/compiler/scala/tools/nsc/typechecker/PatMatVirtualiser.scala#L438-L440 In the test case, the GLB collapsed to `Null` because its computation failed (combination of f-bounds, existentials, skolems that I don't follow), see `throw GlbFailure`. This is how it's been for 14 years (b894f80). This resulted in a cast to `Null$` failing at runtime. I assume GLB is fixed in Scala 3, as this core of the type system has a new implementation. But the test case as such doesn't compile in Scala 3 due to the non-wildcard existential.
8535fed
to
84bf5ca
Compare
@SethTisue could you run this one through the community build? |
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.
Glad my hunch seems to be working! Easier than fixing glb
itself in this instance!
- Remove
def glbWith
, should be dead code now - Analyze a
jardiff
of a bootstrapped vs non-boostrapped compiler to get a sense of what, if anything, changes using the compiler as a test case.
The Jardiff for library+reflect+compiler here: https://gist.github.com/lrytz/35b3e7be0b6dab13ed95e771feb04429 It looks fine. The type of this As it's captured below in |
I'll get to it... a bit backed up, post-holidays |
https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4226/ was green 🎉 (the metals failure is expected, it always fails on PR validation snapshots) |
I have lingering suspicion that there may be codebases out there relying on the old behavior. I think we can go ahead with it regardless, but let's release-note it. |
In type patterns
c match { case x: T }
, the translation would assign the GLB ofc
's type andT
to the variablex
.This seems to trace back to the first version of the "virtual pattern matcher". I could not find a similar use of
glb
in that revision of the codebase. So I'm not sure if it was a new addition, or picked up from the previous implementation.scala/src/compiler/scala/tools/nsc/typechecker/PatMatVirtualiser.scala
Lines 438 to 440 in 8a9fd64
In the test case, the GLB collapsed to
Null
because its computation failed (combination of f-bounds, existentials, skolems that I don't follow), seethrow GlbFailure
. This is how it's been for 14 years (b894f80). This resulted in a cast toNull$
failing at runtime.I assume GLB is fixed in Scala 3, as this core of the type system has a new implementation. But the test case as such doesn't compile in Scala 3 due to the non-wildcard existential.
Fixes scala/bug#12702