Fix discriminant checking in contextual types

[ahejlsberg]

Fixes #23777. Since the repro scenario is somewhat involved there is no regression test in this PR. However, I have manually verified that the issue is fixed for #23777.

[weswigham]

I don't particularly like the idea of merging a change like this without a test - it's super easy to accidentally regress 😢

[weswigham]

An identifier may end up being any type and cause a circularity still (likely indirected through control flow analysis, like the issue in #23661 is) - while this change may happen to break the exact loop in the issue, there's probably still a problem if you add an indirection.

[ahejlsberg]

I'm not sure what you mean. The key here is that checkIdentifier will never ask for the contextual type of the identifier and therefore it can't cause a circularity issue.

[weswigham]

AFAIK if the Identifier in question is a parameter name it can still be contextually typed - though I suppose that case should be handled outside of checkExpression, and inside getContextualType.

[weswigham]

Also, literally any expression can be a discriminant here (because discrimination is type-based and not syntactic), see this, which works today and this PR would break:

type A = {
    kind: "a",
    value: 42,
    doer(): void;
}

type B = {
    kind: "b",
    value: string,
    doer(): void;
}

function getMode(): "a" {
    return "a";
}

function needsNumber(x: 42): void { }

const x: A | B = {
    kind: getMode(),
    value: 42,
    doer() {
        needsNumber(this.value);
    }
};

[weswigham]

I think it'd work much more uniformly to just check the initializer with the thus-far-known context, eg: checkExpressionWithContextualType(prop.initializer, getTypeOfPropertyOfContextualType(contextualType, prop.symbol.escapedName), cloneTypeMapper(identityMapper)), rather than a patchy syntactic kludge. Should also reduce how much work needs to be done.

[ahejlsberg]

I don't think so. There's nothing kludgy about this fix and it definitely reduces the amount of work more than what you suggest. When obtaining a contextual type we should never reach back down to the same type again unless we're absolutely sure that it won't again ask for a contextual type.

[weswigham]

When obtaining a contextual type we should never reach back down to the same type again unless we're absolutely sure that it won't again ask for a contextual type.

We use resolveCall within getContextualType, which we know will call back into getContextualType - we just rely on the resolvedSignature circularity guards to hopefully prevent bad behavior. IMO, checking the expression in the context that we already have puts a hard ceiling on the contextual type (and is much more inline with that resolveCall philosophy of using existing resolution and circularity detection mechanics) and doesn't have the potential to change our behavior as much. I also think this change isn't going to affect our tests much one way or the other (especially if we're not adding a regression test) because we only have about two tests covering this branch of getApparentTypeOfContextualType, which were drastically simplified to only have literals as, at the time, that was all that was needed for the original repro. This branch of code is just for the case that a contextual type is discriminable based on what's already been defined, after all.