Fix an issue with types possibly being related to discriminated type despite of incompatible variance

[Andarist]

fixes https://github.com/microsoft/TypeScript/issues/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 isRelatedTo to 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.False was 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 isRelated there 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 of Ternary.False.

[Andarist]

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.

[RyanCavanaugh]

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

gulp local --built

to reproduce

[RyanCavanaugh]

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.

[Andarist]

Thanks for the tip on how to reproduce this locally! I was trying gulp local before (guided by the error logs) but I didn't use the --built flag (nor --lkg=false that is used by the CI).

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?

[Andarist]

OTOH - IIRC variance check can return a different result than a full structural check and that's intended... 🤔

[weswigham]

IRC 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 Unreliable and Unmeasurable bail-outs in variance measurement to flag those cases so we rely partially or solely on structural results for them, respectively).

* Unless in and out annotations are used, then it's anyone's guess. We toss a lot of variance measurement correctness guarantees out the window when those get used, since they can often be more restrictive than the measured variance, but only get used for variance-based comparisons, and not structural ones. The resulting mismatch can cause a headache of weird behavior which I usually just warn people off with "only use in and out for performance, and only if you know what you're doing".

[Andarist]

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.

[Andarist]

@weswigham going to close this PR since you have fixed the inconsistency in https://github.com/microsoft/TypeScript/pull/52106 but part of the issue still looks suspicious and perhaps could be looked into or explained (see here)