Skip to content
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.

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

Discriminate contextual types #19733

Merged
merged 5 commits into from
Nov 7, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
29 changes: 27 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13663,8 +13663,33 @@ namespace ts {
// 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 {
const type = getContextualType(node);
return type && getApparentType(type);
let contextualType = getContextualType(node);
contextualType = contextualType && getApparentType(contextualType);
if (contextualType && contextualType.flags & TypeFlags.Union && isObjectLiteralExpression(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

invert this I think

let match: Type | undefined;
propLoop: for (const prop of node.properties) {
Copy link
Member

@sandersn sandersn Nov 6, 2017

Choose a reason for hiding this comment

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

this code is almost exactly the same as the also-highly-questionable findMatchingDiscriminantType. The only real difference, I suspect, is that this code doesn't bail if (type === match). Two things:

  1. Can you harmonise the two code paths so there's only one findMatchingDiscriminantType?
    2.It's quite possible that the excess-property usage and union-error usage would both benefit from the (type === match) addition, so it could be that you can just swap out the existing body for this code.
  2. Three things! Using findMatchingDiscriminantType is slow, and only makes the compiler faster because it eliminates unions early on. Can you test performance with this change?

Copy link
Member Author

@weswigham weswigham Nov 6, 2017

Choose a reason for hiding this comment

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

  1. findMatchingDiscriminantType operates on type members (and findMatchingDiscriminantType directly uses isRelatedTo), this operates on an actual node tree (as most contextual type operations do), so sadly I don't think they can be merged (without a bunch of inefficient abstractions over weather you're getting symbols from types or nodes and such)
  2. Sure, I'll try modifying findMatchingDiscriminantType, too; but I don't think it'll show up in much unless we add a test case where multiple fields are capable of acting as a discriminant for an object and discriminate to the same types. 🐱
  3. Sure, will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh hey, waddoyaknow, we already do have a test (excessPropertyCheckWithUnions) improved by changing findMatchingDiscriminantType. Neat.

if (!prop.symbol) continue;
if (prop.kind !== SyntaxKind.PropertyAssignment) continue;
if (isDiscriminantProperty(contextualType, prop.symbol.escapedName)) {
const discriminatingType = getTypeOfNode(prop.initializer);
for (const type of (contextualType as UnionType).types) {
const targetType = getTypeOfPropertyOfType(type, prop.symbol.escapedName);
if (targetType && checkTypeAssignableTo(discriminatingType, targetType, /*errorNode*/ undefined)) {
if (match) {
if (type === match) continue; // Finding multiple fields which discriminate to the same type is fine
match = undefined;
break propLoop;
}
match = type;
}
}
}
}
if (match) {
contextualType = match;
}
}
return contextualType;
}

/**
Expand Down
42 changes: 42 additions & 0 deletions tests/baselines/reference/contextuallyTypedByDiscriminableUnion.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//// [contextuallyTypedByDiscriminableUnion.ts]
type ADT = {
kind: "a",
method(x: string): number;
} | {
kind: "b",
method(x: number): string;
};


function invoke(item: ADT) {
if (item.kind === "a") {
item.method("");
}
else {
item.method(42);
}
}

invoke({
kind: "a",
method(a) {
return +a;
}
});


//// [contextuallyTypedByDiscriminableUnion.js]
function invoke(item) {
if (item.kind === "a") {
item.method("");
}
else {
item.method(42);
}
}
invoke({
kind: "a",
method: function (a) {
return +a;
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
=== tests/cases/compiler/contextuallyTypedByDiscriminableUnion.ts ===
type ADT = {
>ADT : Symbol(ADT, Decl(contextuallyTypedByDiscriminableUnion.ts, 0, 0))

kind: "a",
>kind : Symbol(kind, Decl(contextuallyTypedByDiscriminableUnion.ts, 0, 12))

method(x: string): number;
>method : Symbol(method, Decl(contextuallyTypedByDiscriminableUnion.ts, 1, 14))
>x : Symbol(x, Decl(contextuallyTypedByDiscriminableUnion.ts, 2, 11))

} | {
kind: "b",
>kind : Symbol(kind, Decl(contextuallyTypedByDiscriminableUnion.ts, 3, 5))

method(x: number): string;
>method : Symbol(method, Decl(contextuallyTypedByDiscriminableUnion.ts, 4, 14))
>x : Symbol(x, Decl(contextuallyTypedByDiscriminableUnion.ts, 5, 11))

};


function invoke(item: ADT) {
>invoke : Symbol(invoke, Decl(contextuallyTypedByDiscriminableUnion.ts, 6, 2))
>item : Symbol(item, Decl(contextuallyTypedByDiscriminableUnion.ts, 9, 16))
>ADT : Symbol(ADT, Decl(contextuallyTypedByDiscriminableUnion.ts, 0, 0))

if (item.kind === "a") {
>item.kind : Symbol(kind, Decl(contextuallyTypedByDiscriminableUnion.ts, 0, 12), Decl(contextuallyTypedByDiscriminableUnion.ts, 3, 5))
>item : Symbol(item, Decl(contextuallyTypedByDiscriminableUnion.ts, 9, 16))
>kind : Symbol(kind, Decl(contextuallyTypedByDiscriminableUnion.ts, 0, 12), Decl(contextuallyTypedByDiscriminableUnion.ts, 3, 5))

item.method("");
>item.method : Symbol(method, Decl(contextuallyTypedByDiscriminableUnion.ts, 1, 14))
>item : Symbol(item, Decl(contextuallyTypedByDiscriminableUnion.ts, 9, 16))
>method : Symbol(method, Decl(contextuallyTypedByDiscriminableUnion.ts, 1, 14))
}
else {
item.method(42);
>item.method : Symbol(method, Decl(contextuallyTypedByDiscriminableUnion.ts, 4, 14))
>item : Symbol(item, Decl(contextuallyTypedByDiscriminableUnion.ts, 9, 16))
>method : Symbol(method, Decl(contextuallyTypedByDiscriminableUnion.ts, 4, 14))
}
}

invoke({
>invoke : Symbol(invoke, Decl(contextuallyTypedByDiscriminableUnion.ts, 6, 2))

kind: "a",
>kind : Symbol(kind, Decl(contextuallyTypedByDiscriminableUnion.ts, 18, 8))

method(a) {
>method : Symbol(method, Decl(contextuallyTypedByDiscriminableUnion.ts, 19, 14))
>a : Symbol(a, Decl(contextuallyTypedByDiscriminableUnion.ts, 20, 11))

return +a;
>a : Symbol(a, Decl(contextuallyTypedByDiscriminableUnion.ts, 20, 11))
}
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
=== tests/cases/compiler/contextuallyTypedByDiscriminableUnion.ts ===
type ADT = {
>ADT : ADT

kind: "a",
>kind : "a"

method(x: string): number;
>method : (x: string) => number
>x : string

} | {
kind: "b",
>kind : "b"

method(x: number): string;
>method : (x: number) => string
>x : number

};


function invoke(item: ADT) {
>invoke : (item: ADT) => void
>item : ADT
>ADT : ADT

if (item.kind === "a") {
>item.kind === "a" : boolean
>item.kind : "a" | "b"
>item : ADT
>kind : "a" | "b"
>"a" : "a"

item.method("");
>item.method("") : number
>item.method : (x: string) => number
>item : { kind: "a"; method(x: string): number; }
>method : (x: string) => number
>"" : ""
}
else {
item.method(42);
>item.method(42) : string
>item.method : (x: number) => string
>item : { kind: "b"; method(x: number): string; }
>method : (x: number) => string
>42 : 42
}
}

invoke({
>invoke({ kind: "a", method(a) { return +a; }}) : void
>invoke : (item: ADT) => void
>{ kind: "a", method(a) { return +a; }} : { kind: "a"; method(a: string): number; }

kind: "a",
>kind : string
>"a" : "a"

method(a) {
>method : (a: string) => number
>a : string

return +a;
>+a : number
>a : string
}
});

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
tests/cases/compiler/excessPropertyCheckWithUnions.ts(10,30): error TS2322: Type '{ tag: "T"; a1: string; }' is not assignable to type 'ADT'.
Object literal may only specify known properties, and 'a1' does not exist in type '{ tag: "T"; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(11,21): error TS2322: Type '{ tag: "A"; d20: 12; }' is not assignable to type 'ADT'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(11,21): error TS2322: Type '{ tag: "A"; d20: number; }' is not assignable to type 'ADT'.
Object literal may only specify known properties, and 'd20' does not exist in type '{ tag: "A"; a1: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(12,1): error TS2322: Type '{ tag: "D"; }' is not assignable to type 'ADT'.
Type '{ tag: "D"; }' is not assignable to type '{ tag: "D"; d20: 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20; }'.
Expand Down Expand Up @@ -35,7 +35,7 @@ tests/cases/compiler/excessPropertyCheckWithUnions.ts(40,1): error TS2322: Type
!!! error TS2322: Object literal may only specify known properties, and 'a1' does not exist in type '{ tag: "T"; }'.
wrong = { tag: "A", d20: 12 }
~~~~~~~
!!! error TS2322: Type '{ tag: "A"; d20: 12; }' is not assignable to type 'ADT'.
!!! error TS2322: Type '{ tag: "A"; d20: number; }' is not assignable to type 'ADT'.
!!! error TS2322: Object literal may only specify known properties, and 'd20' does not exist in type '{ tag: "A"; a1: string; }'.
wrong = { tag: "D" }
~~~~~
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/excessPropertyCheckWithUnions.types
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ let wrong: ADT = { tag: "T", a1: "extra" }
>"extra" : "extra"

wrong = { tag: "A", d20: 12 }
>wrong = { tag: "A", d20: 12 } : { tag: "A"; d20: 12; }
>wrong = { tag: "A", d20: 12 } : { tag: "A"; d20: number; }
>wrong : ADT
>{ tag: "A", d20: 12 } : { tag: "A"; d20: 12; }
>{ tag: "A", d20: 12 } : { tag: "A"; d20: number; }
>tag : string
>"A" : "A"
>d20 : number
Expand Down
25 changes: 25 additions & 0 deletions tests/cases/compiler/contextuallyTypedByDiscriminableUnion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// @noImplicitAny: true
type ADT = {
kind: "a",
method(x: string): number;
} | {
kind: "b",
method(x: number): string;
};


function invoke(item: ADT) {
if (item.kind === "a") {
item.method("");
}
else {
item.method(42);
}
}

invoke({
kind: "a",
method(a) {
return +a;
}
});