Fix infinite recursion in union type reduction #3157
Conversation
Conflicts: tests/baselines/reference/unionTypeWithRecursiveSubtypeReduction2.errors.txt tests/baselines/reference/unionTypeWithRecursiveSubtypeReduction2.js tests/cases/compiler/unionTypeWithRecursiveSubtypeReduction2.ts
| } | ||
| } | ||
| else if (type.reducedType === resolvingType) { | ||
| type.reducedType = type; |
There was a problem hiding this comment.
- No error when we hit the circularity? Or is that handled elsewhere? If handled elsewhere, can you comment in the code where.
- It seems like you're setting the reduced type to the type itself. That seems... odd. What's the intuition for why that's the appropriate type.
There was a problem hiding this comment.
Subtype reduction is basically an optimization we do to avoid excessively large union types, which take longer to process and look strange in quick info and error messages. Semantically there is no difference between the reduced type and the type itself. So, when we detect a circularity we simply say that the reduced type is the type itself.
There was a problem hiding this comment.
That makes sense. We should definitely state this in the code itself with a comment of some sort. It's not at all self-evident, and it's definitely important for the code to make this clear that this is expected and desirable behavior.
|
👍 with the change to document the behavior/invariants more clearly in the code. |
| if (!type.reducedType) { | ||
| type.reducedType = getUnionType(type.types, /*noSubtypeReduction*/ false); | ||
| type.reducedType = resolvingType; |
There was a problem hiding this comment.
Sorry, I know we talked about this, but why do you not have to use the type resolution stack for this? What if it participates in a cycle that other functions have to know about, in order to report errors?
There was a problem hiding this comment.
The kind of cycles detected by the type resolution stack has nothing to do with reduced union types and would never involve union type reduction. This is a distinct and unrelated kind of recursion. It so happens to use a marker type called resolvingType for cycle detection, but that's just a name. Maybe it should be renamed markerType or circularType since it isn't actually used by type resolution anymore (we have the resolution stack for that).
There was a problem hiding this comment.
I think circularType is good, or circularTypeMarker.
There was a problem hiding this comment.
I don't get it, we always use resolvingType to detect other circularities, why would we be different here?
There was a problem hiding this comment.
The resolvingType marker was removed in #2991. This introduces a similar marker, but used for detecting a different kind of circularity.
Fix infinite recursion in union type reduction
Fixes #2997 and #3152. Subsumes #3071.