Fix an issue with types possibly being related to discriminated type despite of incompatible variance #51208
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| if (getObjectFlags(source) & ObjectFlags.Reference && getObjectFlags(type) & ObjectFlags.Reference && (source as TypeReference).target === (type as TypeReference).target && | ||
| !isTupleType(source) && !(isMarkerType(source) || isMarkerType(type))) { | ||
| // When strictNullChecks is disabled, the element type of the empty array literal is undefinedWideningType, | ||
| // and an empty array literal wouldn't be assignable to a `never[]` without this check. | ||
| if (isEmptyArrayLiteralType(source)) { | ||
| return Ternary.True; | ||
| } | ||
| // We have type references to the same generic type, and the type references are not marker | ||
| // type references (which are intended by be compared structurally). Obtain the variance | ||
| // information for the type parameters and relate the type arguments accordingly. | ||
| const variances = getVariances((source as TypeReference).target); | ||
| // We return Ternary.Maybe for a recursive invocation of getVariances (signalled by emptyArray). This | ||
| // effectively means we measure variance only from type parameter occurrences that aren't nested in | ||
| // recursive instantiations of the generic type. | ||
| if (variances === emptyArray) { | ||
| return Ternary.Unknown; | ||
| } | ||
| if (!typeArgumentsRelatedTo(getTypeArguments(source as TypeReference), getTypeArguments(type as TypeReference), variances, /* reportErrors */ false, IntersectionState.None)) { | ||
| return Ternary.False; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is effectively a copy-paste (with very small adjustments) of the block here:
https://github.dev/microsoft/TypeScript/blob/4c9afe8812fcdb4658472ccbced4a5cd4bae70ea/src/compiler/checker.ts#L20150-L20171
This definitely should be deduplicated if the solution itself is correct. I will wait for the review before making any changes to this.
There was a problem hiding this comment.
Using variance results more is usually good for perf, but a variance-based result and a structural result being different is fundamentally the problem. Usually means some comparison needs to add an Unmeasurrable or Unreliable marker to the result calculated for the variance relationship.
There was a problem hiding this comment.
Do you think there is a problem with the added code? Or that there is a problem elsewhere and that elsewhere Unmeasurable/Unreliable is not handled correctly?
There was a problem hiding this comment.
More likely that a non-strictly-co--or-contra-variant comparison somewhere needs to throw out a Unmeasurable or Unreliable flag during the measurement comparison but isn't yet doing so.
…despite of incompatible variance
|
It's kinda wild that the CI fails here with a type error. I can't repro this locally (using the same script) and I don't know how I would get a type error at that location as the changes in this PR are totally unrelated to it. |
|
The failure here is saying that this change causes TS to no longer be able to build itself. Confirming, I see this locally checking out your PR branch, but not in main. You can run to reproduce |
|
Aside, it's quite rare to break TS without affecting any other tests! We'd want a testcase that isolates the problem this exposes, assuming there is one. |
|
Thanks for the tip on how to reproduce this locally! I was trying I've checked out the failing call site and as far as I can tell - it's a legit one :P it shouldn't fail. I've created a TS playground demonstrating the problem: TS playground. Gonna wrap this up later in a test case that fails with this PR but passes with the main branch and gonna open a PR with that directly to main. In the meantime, maybe you'd have suggestions on how this PR could be improved to fix the original issue without degrading this case? |
|
OTOH - IIRC variance check can return a different result than a full structural check and that's intended... 🤔 |
Definitely not(*). Any time the variance measurement for a type differs from the structural check, it's a bug, usually in the variance calculation logic. (We have many structural type relationships that aren't strictly co- or contra- variant and so have * Unless |
|
This has fallen off my radar and pushed down the priority list but I will be back here to investigate this further (unless somebody beats me to it). For now, I will convert this to a draft to indicate that I don't exactly work on this right now. |
# Conflicts: # src/compiler/checker.ts
|
@weswigham going to close this PR since you have fixed the inconsistency in #52106 but part of the issue still looks suspicious and perhaps could be looked into or explained (see here) |
fixes #51180
This union was correctly failing a check here:
https://github.dev/microsoft/TypeScript/blob/4c9afe8812fcdb4658472ccbced4a5cd4bae70ea/src/compiler/checker.ts#L19750
but its result isn't directly used if the check failed. This check effectively went through
isRelatedToto this block and failed there:https://github.dev/microsoft/TypeScript/blob/4c9afe8812fcdb4658472ccbced4a5cd4bae70ea/src/compiler/checker.ts#L20150-L20171
So since this correct result of
Ternary.Falsewas not returned there (for reasons I don't understand but they seem to be important), the execution has continued. It continued up to this check here which succeeded:https://github.dev/microsoft/TypeScript/blob/4c9afe8812fcdb4658472ccbced4a5cd4bae70ea/src/compiler/checker.ts#L20219
In there, only the structures are compared here:
https://github.dev/microsoft/TypeScript/blob/4c9afe8812fcdb4658472ccbced4a5cd4bae70ea/src/compiler/checker.ts#L20370-L20383
I've tried to call
isRelatedthere instead but that experiment failed (maybe somebody else could make it work though). So I've reused the code related to variances that was responsible for creating the "initial" correct result ofTernary.False.