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

Re-lub also hard union types in simplify #20027

Merged
merged 3 commits into from Apr 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 2 additions & 9 deletions compiler/src/dotty/tools/dotc/core/TypeOps.scala
Expand Up @@ -157,15 +157,8 @@ object TypeOps:
tp.derivedAlias(simplify(tp.alias, theMap))
case AndType(l, r) if !ctx.mode.is(Mode.Type) =>
simplify(l, theMap) & simplify(r, theMap)
case tp @ OrType(l, r)
if !ctx.mode.is(Mode.Type)
&& (tp.isSoft || l.isBottomType || r.isBottomType) =>
// Normalize A | Null and Null | A to A even if the union is hard (i.e.
// explicitly declared), but not if -Yexplicit-nulls is set. The reason is
// that in this case the normal asSeenFrom machinery is not prepared to deal
// with Nulls (which have no base classes). Under -Yexplicit-nulls, we take
// corrective steps, so no widening is wanted.
simplify(l, theMap) | simplify(r, theMap)
case tp @ OrType(l, r) if !ctx.mode.is(Mode.Type) =>
TypeComparer.lub(simplify(l, theMap), simplify(r, theMap), isSoft = tp.isSoft)
case tp @ CapturingType(parent, refs) =>
if !ctx.mode.is(Mode.Type)
&& refs.subCaptures(parent.captureSet, frozen = true).isOK
Expand Down
11 changes: 5 additions & 6 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Expand Up @@ -3598,12 +3598,11 @@ object Types extends TypeUtils {

override def widenUnionWithoutNull(using Context): Type =
if myUnionPeriod != ctx.period then
myUnion =
if isSoft then
TypeComparer.lub(tp1.widenUnionWithoutNull, tp2.widenUnionWithoutNull, canConstrain = true, isSoft = isSoft) match
case union: OrType => union.join
case res => res
else derivedOrType(tp1.widenUnionWithoutNull, tp2.widenUnionWithoutNull, soft = isSoft)
val union = TypeComparer.lub(
tp1.widenUnionWithoutNull, tp2.widenUnionWithoutNull, canConstrain = isSoft, isSoft = isSoft)
noti0na1 marked this conversation as resolved.
Show resolved Hide resolved
myUnion = union match
case union: OrType if isSoft => union.join
case _ => union
if !isProvisional then myUnionPeriod = ctx.period
myUnion

Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Namer.scala
Expand Up @@ -1961,7 +1961,8 @@ class Namer { typer: Typer =>
else
// don't strip @uncheckedVariance annot for default getters
TypeOps.simplify(tp.widenTermRefExpr,
if defaultTp.exists then TypeOps.SimplifyKeepUnchecked() else null) match
if defaultTp.exists then TypeOps.SimplifyKeepUnchecked() else null)
match
case ctp: ConstantType if sym.isInlineVal => ctp
case tp => TypeComparer.widenInferred(tp, pt, widenUnions = true)

Expand Down
16 changes: 16 additions & 0 deletions compiler/test-resources/repl/10693
@@ -0,0 +1,16 @@
scala> def test[A, B](a: A, b: B): A | B = a
def test[A, B](a: A, b: B): A | B
scala> def d0 = test("string", 1)
def d0: String | Int
scala> def d1 = test(1, "string")
def d1: Int | String
scala> def d2 = test(d0, d1)
def d2: String | Int
scala> def d3 = test(d1, d0)
def d3: Int | String
scala> def d4 = test(d2, d3)
def d4: String | Int
scala> def d5 = test(d3, d2)
def d5: Int | String
scala> def d6 = test(d4, d5)
def d6: String | Int
8 changes: 8 additions & 0 deletions tests/pos/i10693.scala
@@ -0,0 +1,8 @@
def test[A, B](a: A, b: B): A | B = a
val v0 = test("string", 1)
val v1 = test(1, "string")
val v2 = test(v0, v1)
val v3 = test(v1, v0)
val v4 = test(v2, v3)
val v5 = test(v3, v2)
val v6 = test(v4, v5)

Choose a reason for hiding this comment

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

I'm glad the issue is being resolved, but I don't understand this test.

How can we see that for instance the type of v2 isn't String | Int | Int | String? Don't we want to show that it is inferred as Int | String or String | Int?

Copy link
Contributor Author

@odersky odersky Mar 26, 2024

Choose a reason for hiding this comment

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

Right now: manual inspection after -Xprint:typer. It would be good if someone could add a better test. We don't have good infrastructure for inferred types complexity yet. Maybe the presentation compiler tests have some of the necessary setup?

Choose a reason for hiding this comment

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

Now this bit I had figured out!
Its a bit of a kludge, but In #20014 I had a test that was actually validating this via the repl tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! That's a good idea to use repl tests for that. I added your test to the PR.