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.
lrytz
force-pushed
the
t12702-pending
branch
from
8535fed
to
84bf5ca
Compare
|
@SethTisue could you run this one through the community build? |
There was a problem hiding this comment.
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
jardiffof 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 andTto the variablex.This seems to trace back to the first version of the "virtual pattern matcher". I could not find a similar use of
glbin 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
Nullbecause 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