- Notifications
You must be signed in to change notification settings - Fork3.1k
Don't GLB binders of type patterns, use the type directly#10247
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
In type patterns `c match { case x: T }`, the translation would assignthe GLB of `c`'s type and `T` to the varaible `x`.This seems to trace back to the first version of the "virtual patternmatcher". I could not find a similar use of `glb` in that revisionof the codebase. So I'm not sure if it was a new addition, or pickedup from the previous implementation.https://github.com/scala/scala/blob/8a9fd64129926eea35f7dca181242855f14e153f/src/compiler/scala/tools/nsc/typechecker/PatMatVirtualiser.scala#L438-L440In the test case, the GLB collapsed to `Null` because itscomputation failed (combination of f-bounds, existentials, skolems thatI don't follow), see `throw GlbFailure`. This is how it's been for 14years (b894f80). This resulted in a cast to `Null$` failing atruntime.I assume GLB is fixed in Scala 3, as this core of the type system hasa new implementation. But the test case as such doesn't compile inScala 3 due to the non-wildcard existential.lrytz commentedDec 22, 2022
@SethTisue could you run this one through the community build? |
retronym left a comment• edited by lrytz
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by lrytz
Uh oh!
There was an error while loading.Please reload this page.
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 fixingglb 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.
lrytz commentedDec 22, 2022
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 |
SethTisue commentedJan 6, 2023
I'll get to it... a bit backed up, post-holidays |
SethTisue commentedJan 11, 2023
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) |
SethTisue commentedJan 11, 2023
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. |
Uh oh!
There was an error while loading.Please reload this page.
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 in8a9fd64
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.
Fixesscala/bug#12702