Skip to content

Fix discriminant checking in contextual types #23794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 22, 2018
Merged

Fix discriminant checking in contextual types #23794

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15036,6 +15036,26 @@ namespace ts {
}
}

// Return true if the given expression is possibly a discriminant value. We limit the kinds of
// expressions we check to those that don't depend on their contextual type in order not to cause
// recursive (and possibly infinite) invocations of getContextualType.
function isPossiblyDiscriminantValue(node: Expression): boolean {
switch (node.kind) {
case SyntaxKind.StringLiteral:
case SyntaxKind.NumericLiteral:
case SyntaxKind.NoSubstitutionTemplateLiteral:
case SyntaxKind.TrueKeyword:
case SyntaxKind.FalseKeyword:
case SyntaxKind.NullKeyword:
case SyntaxKind.Identifier:
Copy link
Member

@weswigham weswigham Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@weswigham weswigham May 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
    }
};

return true;
case SyntaxKind.PropertyAccessExpression:
case SyntaxKind.ParenthesizedExpression:
return isPossiblyDiscriminantValue((<PropertyAccessExpression | ParenthesizedExpression>node).expression);
}
return false;
}

// Return the contextual type for a given expression node. During overload resolution, a contextual type may temporarily
// be "pushed" onto a node using the contextualType property.
function getApparentTypeOfContextualType(node: Expression): Type {
Expand All @@ -15049,8 +15069,8 @@ namespace ts {
propLoop: for (const prop of node.properties) {
if (!prop.symbol) continue;
if (prop.kind !== SyntaxKind.PropertyAssignment) continue;
if (isDiscriminantProperty(contextualType, prop.symbol.escapedName)) {
const discriminatingType = getTypeOfNode(prop.initializer);
if (isPossiblyDiscriminantValue(prop.initializer) && isDiscriminantProperty(contextualType, prop.symbol.escapedName)) {
const discriminatingType = checkExpression(prop.initializer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (const type of (contextualType as UnionType).types) {
const targetType = getTypeOfPropertyOfType(type, prop.symbol.escapedName);
if (targetType && checkTypeAssignableTo(discriminatingType, targetType, /*errorNode*/ undefined)) {
Expand Down