Skip to content

Fix #10108: Distinguish between soft and hard union types #10112

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

Merged
merged 9 commits into from
Oct 31, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 28, 2020

The new system widens only soft union types using a join. If a hard union type appears in a soft one, it is dissolved together with the soft union type. Unions with Null continue to be treated specially.

@odersky odersky changed the title Fix #10801: Keep an isSoft flag in OrTypes Fix #10108: Distinguish between soft and hard union types Oct 28, 2020
@odersky odersky force-pushed the change-union-types branch 3 times, most recently from 5405907 to f2aef39 Compare October 29, 2020 16:00
@TheElectronWill TheElectronWill linked an issue Oct 29, 2020 that may be closed by this pull request
@odersky odersky force-pushed the change-union-types branch 2 times, most recently from 9d879fb to da869ed Compare October 30, 2020 16:37
It turned out that it's generally clear whether an OrType should be soft or hard, so
this is a good first step to solve the problem.
@odersky odersky force-pushed the change-union-types branch from da869ed to 24f2a6c Compare October 30, 2020 17:24
@odersky odersky marked this pull request as ready for review October 30, 2020 17:24
@odersky
Copy link
Contributor Author

odersky commented Oct 30, 2020

@liufengyun We get quite a few different check files in pattern matching. One problem is that an extra empty line got added in several of them. There are also two completely different outputs for tests involving hard union types. In one case, the check file is now 8MB long! I updated the check files in 248f9eb. Can you take a look at the failures, and possibly also revert 248f9eb?

@liufengyun
Copy link
Contributor

OK, I'll have a look and fix the exhaustivity check problems.

odersky and others added 4 commits October 31, 2020 00:00
- show at most 6 unmatched cases
@@ -18,7 +18,7 @@ import org.junit.Test
class PatmatExhaustivityTest {
val testsDir = "tests/patmat"
// stop-after: patmatexhaust-huge.scala crash compiler
val options = List("-color:never", "-Ystop-after:crossCast", "-Ycheck-all-patmat", "-classpath", TestConfiguration.basicClasspath)
val options = List("-color:never", "-Ystop-after:explicitSelf", "-Ycheck-all-patmat", "-classpath", TestConfiguration.basicClasspath)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That explains the mystery of the added lines, I suppose? Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, most failures come from this change. The test case i8922c.scala and i4030.scala are due to type inference change.


ss.foldLeft(LazyList(Nil : List[Space])) { (acc, flat) =>
for { sps <- acc; s <- flat }
yield sps :+ s
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@odersky
Copy link
Contributor Author

odersky commented Oct 31, 2020

Thanks for the fixes Fengyun! Your changes LGTM. Can you do the review for the rest?

@odersky odersky requested a review from liufengyun October 31, 2020 10:16
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -18,7 +18,7 @@ import org.junit.Test
class PatmatExhaustivityTest {
val testsDir = "tests/patmat"
// stop-after: patmatexhaust-huge.scala crash compiler
val options = List("-color:never", "-Ystop-after:crossCast", "-Ycheck-all-patmat", "-classpath", TestConfiguration.basicClasspath)
val options = List("-color:never", "-Ystop-after:explicitSelf", "-Ycheck-all-patmat", "-classpath", TestConfiguration.basicClasspath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, most failures come from this change. The test case i8922c.scala and i4030.scala are due to type inference change.

@@ -9,5 +9,6 @@ object TestGADT {
def f[A <: Seq[_], B, Foo >: A => B](v: Root[Foo], u: Root[Foo]) = (v, u) match {
case (C3(), C3()) =>
}
f(C3[Seq[_], Long](), C4[Seq[_], Long]())
// The following line no longer type checks
// f(C3[Seq[_], Long](), C4[Seq[_], Long]())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case shows that this PR improves over the previous handling of union types 👍

@odersky odersky merged commit 14ddc26 into scala:master Oct 31, 2020
@odersky odersky deleted the change-union-types branch October 31, 2020 13:19
@smarter smarter mentioned this pull request Dec 7, 2020
16 tasks
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type inference for union types is surprising
3 participants