Fix #8128: Fix atoms computations #8139
Conversation
| @@ -1181,13 +1181,37 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w | |||
| * for equality would give the wrong result, so we should not use the sets | |||
| * for comparisons. | |||
| */ | |||
| def canCompare(atoms: Set[Type]): Boolean = | |||
| ctx.phase.isTyper || { | |||
| def compareAtoms(tp1: Type, tp2: Type): Option[Boolean] = | |||
There was a problem hiding this comment.
The use of an Option here might noticeably increase our allocation rate, a tri-state enum could be used instead.
There was a problem hiding this comment.
I don't think so, since the overwhelming majority of calls should give None. You get a Some only if both compared sides are consist of atoms.
| @@ -1181,13 +1181,37 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w | |||
| * for equality would give the wrong result, so we should not use the sets | |||
| * for comparisons. | |||
| */ | |||
There was a problem hiding this comment.
The comment above applies to the canCompare method so it should be moved down.
| } | ||
| def verified(result: Boolean): Boolean = | ||
| if Config.checkAtomsComparisons && false then |
There was a problem hiding this comment.
Since `checkAtomsComparisons is a final val this is equivalent to:
| if Config.checkAtomsComparisons && false then | |
| if Config.checkAtomsComparisons then |
| } | ||
| tp.underlying.atoms match | ||
| case as @ Atoms.Range(lo, hi) => | ||
| if hi.size == 1 then as else Atoms.Range(Set.empty, hi) |
There was a problem hiding this comment.
Could you document this special case ?
| tp1.atoms match | ||
| case Atoms.Range(lo1, hi1) => | ||
| if hi1.subsetOf(lo2) then Some(verified(true)) | ||
| else if !lo1.subsetOf(hi2) then falseUnlessBottom |
There was a problem hiding this comment.
In what situation do we have a bottom type with a non-empty lo1 ? Do we have a testcase for this ?
There was a problem hiding this comment.
I think it should be just false
If we have `x: 0 | 1` then `x.type` should have atoms {`0`, `1`} only if widening is allowed.
On the other hand, if `x: 0` then `x` has atoms `{0}`irrespective of widening.
Previously we merged the atoms info of a type with an empty atoms set and a type with no atoms, since we identified the empty set with "does not have atoms". But that cannot type the inequality 0 & 1 <: 0 The new scheme distinguishes the two states by using an Option[Set[Type]] in the computations.
- print hashes for term params under -uniqid - don't print RefinedTypes as dependent function types under -Ydebug-print
Several fixes related to how atoms are computed, which fixes some problems when comparing unions of singleton types. It's a partial fix of #8128. There's still some problem with typing dependent functions over singleton types (see commented out code in i8128.scala). This problem does not look related to atoms, however.
Since I won't have time to work on this right now, it's better to get the fixes in this PR in independently.
UPDATE: I managed to find and fix the remaining problem. The fix meant that all intermediate commits were superseded because the structure to compute atoms had to be changed again. So this is best reviewed without looking at individual commits.