Union types: deduplication + Nothing type members absorption #16312
Conversation
mucciolo
force-pushed
the
deduplicate-union-types-#10693
branch
2 times, most recently
from
7e5e28f
to
304618f
Compare
There was a problem hiding this comment.
Summary points:
- One will need to take into account soft vs hard unions. A hard union should never be massaged like ths. If the user wrote
val x: A | Athe system should not silently rewrite toval x: A. - We should not use equality on Types. Equality does not have a real meaning. We have
=:=which is semantic equality, andeqwhich is identity. Identity would be preferable here since it is faster. - My concerns about performance
| // 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) | ||
| else if r.isNothingType || (l eq r) then l |
There was a problem hiding this comment.
Those computations need to be enabled only if tp.isSoft.
There was a problem hiding this comment.
The .isNothingType cases should be redundant; they were covered by the isBottomType tests above.
There was a problem hiding this comment.
Those computations need to be enabled only if
tp.isSoft.
Thing is: the type of all values in tests/printing/i10693.scala arrive here as a hard union even though not explicitly declared. Is this expected behavior?
The .isNothingType cases should be redundant; they were covered by the isBottomType tests above.
The case where val t0 = Tuple1(1).toList arrives here with context in type mode (ctx.mode.is(Mode.Type)) and it is attributed the type of List[Int | Nothing] without these tests. I feel it is best to understand why test cases get here as hard unions before touching this again.
| /** Returns an equivalent union tree without repeated members and with Nothing types absorbed | ||
| * by other types, if present. Weaker than LUB. | ||
| */ | ||
| def deduplicatedAbsorbingNothingTypes(using Context): Type = { |
There was a problem hiding this comment.
There are two problems with this approach
- It looks inefficient. You need a fast mutable set based on identity not structural equality. Everything else would be needlessly slow.
- Determinism. You need a set that has a reproducible order of elements between compiler runs. A LinkedHashSet would do, except that it's based on equality. A java.util.IdentityHashMap or a dotc.util.EqHashMap would be based on identity, but it's not deterministic. So maybe you have to write that set yourself. A combination of a ListBuffer for the elements and a EqHashMap could work, maybe.
mucciolo
force-pushed
the
deduplicate-union-types-#10693
branch
from
a26084e
to
633eb23
Compare
|
Hey @mucciolo this has been sitting here for quite some time. I just wanted to check if you were still working on this, waiting for a review, etc? |
Hey, @ckipp01! Thank you for reaching me out. I'm not actively working on this issue at the moment but I'm willing to give it a second try. Last time I was researching why inferred union types are "hard" and whether it is possible to infer them soft instead. By the way, would you be able to tell me which is the most appropriate place to ask these kind of questions? |
Added the following simplification rules to OrType: - deduplication without subtype comparisons by the means of object equality/hash code - absorption of Nothing types (T | Nothing == T)
Updated test check files and Markdown syntax used in Scaladoc
- Replaced recursion by a while loop - Replaced immutable.Set by EqLinkedHashSet
Sure, there is a couple different places you can ask. Feel free to just ask those questions here if they are related to your PR. You can also jump on the Scala Discord and check out the #scala-contributors channel. You can ask questions there as well! Also it looks like you're including a ton of unrelated old commits. You may want to make sure you correctly rebase only your commits on top of HEAD. |
mucciolo
force-pushed
the
deduplicate-union-types-#10693
branch
from
bb608b8
to
588ee18
Compare
Greetings, everybody. I'm keen to get started as a Scala contributor and this is my first attempt. Any guidance is welcome.
Closes #10693
This PR contains an implementation of the union tree deduplication algorithm described by @abgruszecki in #10693 (comment) modified to absorb
Nothingtypes. This modification should allow theTuple.toListexamples given in the comments section to have more intuitive inferred types as well.Some tests needed to be updated in accordance to the new union simplification rules.