Skip to content

Commit

Permalink
Fix contextual discrimination for omitted members (microsoft#43937)
Browse files Browse the repository at this point in the history 
In short, the fix I submitted looked at the union ofproperties, but it
really should have looked at the intersection.

Two sytlistic notes. I couldn't find the best way to get the unique
strings of an array like `[...new Set()]` would, so I created a small
helper function, but didn't put it in a great place. Also, before the
second concatenated array of discriminators at least matched the first
in complexity, but now it's much worse. I don't think that section is
particularly easy to read, but I also don't see a significantly reusable
part.

fixes microsoft#41759
  • Loading branch information
@erikbrinkman
erikbrinkman committed Mar 4, 2022
1 parent ae62da9 commit 751c114
Show file tree
Hide file tree
Showing 9 changed files with 237 additions and 67 deletions.
25 changes: 21 additions & 4 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26656,6 +26656,13 @@ namespace ts {
return false;
}

function uniqueStrings(strings: readonly __String[]): __String[] {
const unique = new Set(strings);
const result: __String[] = [];
unique.forEach(str => result.push(str));
return result;
}

function discriminateContextualTypeByObjectMembers(node: ObjectLiteralExpression, contextualType: UnionType) {
return getMatchingUnionConstituentForObjectLiteral(contextualType, node) || discriminateTypeByDiscriminableItems(contextualType,
concatenate(
Expand All @@ -26664,8 +26671,13 @@ namespace ts {
prop => ([() => getContextFreeTypeOfExpression((prop as PropertyAssignment).initializer), prop.symbol.escapedName] as [() => Type, __String])
),
map(
filter(getPropertiesOfType(contextualType), s => !!(s.flags & SymbolFlags.Optional) && !!node?.symbol?.members && !node.symbol.members.has(s.escapedName) && isDiscriminantProperty(contextualType, s.escapedName)),
s => [() => undefinedType, s.escapedName] as [() => Type, __String]
uniqueStrings(flatMap(contextualType.types, memberType =>
map(
filter(getPropertiesOfType(memberType), s => !!(s.flags & SymbolFlags.Optional) && !!node?.symbol?.members && !node.symbol.members.has(s.escapedName) && isDiscriminantProperty(contextualType, s.escapedName)),
s => s.escapedName
)
)),
name => [() => undefinedType, name] as [() => Type, __String]
)
),
isTypeAssignableTo,
Expand All @@ -26681,8 +26693,13 @@ namespace ts {
prop => ([!(prop as JsxAttribute).initializer ? (() => trueType) : (() => getContextFreeTypeOfExpression((prop as JsxAttribute).initializer!)), prop.symbol.escapedName] as [() => Type, __String])
),
map(
filter(getPropertiesOfType(contextualType), s => !!(s.flags & SymbolFlags.Optional) && !!node?.symbol?.members && !node.symbol.members.has(s.escapedName) && isDiscriminantProperty(contextualType, s.escapedName)),
s => [() => undefinedType, s.escapedName] as [() => Type, __String]
uniqueStrings(flatMap(contextualType.types, memberType =>
map(
filter(getPropertiesOfType(memberType), s => !!(s.flags & SymbolFlags.Optional) && !!node?.symbol?.members && !node.symbol.members.has(s.escapedName) && isDiscriminantProperty(contextualType, s.escapedName)),
s => s.escapedName
)
)),
name => [() => undefinedType, name] as [() => Type, __String]
)
),
isTypeAssignableTo,
Expand Down
16 changes: 15 additions & 1 deletion tests/baselines/reference/discriminantPropertyInference.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ type DiscriminatorFalse = {
cb: (x: number) => void;
}

type Props = DiscriminatorTrue | DiscriminatorFalse;
type Unrelated = {
val: number;
}

declare function f(options: DiscriminatorTrue | DiscriminatorFalse): any;

Expand All @@ -37,6 +39,14 @@ f({
f({
cb: n => n.toFixed()
});


declare function g(options: DiscriminatorTrue | DiscriminatorFalse | Unrelated): any;

// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
g({
cb: n => n.toFixed()
});


//// [discriminantPropertyInference.js]
Expand All @@ -60,3 +70,7 @@ f({
f({
cb: function (n) { return n.toFixed(); }
});
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
g({
cb: function (n) { return n.toFixed(); }
});
73 changes: 48 additions & 25 deletions tests/baselines/reference/discriminantPropertyInference.symbols
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,74 +23,97 @@ type DiscriminatorFalse = {
>x : Symbol(x, Decl(discriminantPropertyInference.ts, 9, 9))
}

type Props = DiscriminatorTrue | DiscriminatorFalse;
>Props : Symbol(Props, Decl(discriminantPropertyInference.ts, 10, 1))
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(discriminantPropertyInference.ts, 0, 0))
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(discriminantPropertyInference.ts, 5, 1))
type Unrelated = {
>Unrelated : Symbol(Unrelated, Decl(discriminantPropertyInference.ts, 10, 1))

val: number;
>val : Symbol(val, Decl(discriminantPropertyInference.ts, 12, 18))
}

declare function f(options: DiscriminatorTrue | DiscriminatorFalse): any;
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
>options : Symbol(options, Decl(discriminantPropertyInference.ts, 14, 19))
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))
>options : Symbol(options, Decl(discriminantPropertyInference.ts, 16, 19))
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(discriminantPropertyInference.ts, 0, 0))
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(discriminantPropertyInference.ts, 5, 1))

// simple inference
f({
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))

disc: true,
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 17, 3))
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 19, 3))

cb: s => parseInt(s)
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 18, 15))
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 19, 7))
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 20, 15))
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 21, 7))
>parseInt : Symbol(parseInt, Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 19, 7))
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 21, 7))

});

// simple inference
f({
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))

disc: false,
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 23, 3))
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 25, 3))

cb: n => n.toFixed()
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 24, 16))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 25, 7))
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 26, 16))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 27, 7))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 25, 7))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 27, 7))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

});

// simple inference when strict-null-checks are enabled
f({
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))

disc: undefined,
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 29, 3))
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 31, 3))
>undefined : Symbol(undefined)

cb: n => n.toFixed()
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 30, 20))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 31, 7))
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 32, 20))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 33, 7))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 31, 7))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 33, 7))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

});

// requires checking type information since discriminator is missing from object
f({
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))

cb: n => n.toFixed()
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 37, 3))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 38, 7))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 38, 7))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

});


declare function g(options: DiscriminatorTrue | DiscriminatorFalse | Unrelated): any;
>g : Symbol(g, Decl(discriminantPropertyInference.ts, 39, 3))
>options : Symbol(options, Decl(discriminantPropertyInference.ts, 42, 19))
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(discriminantPropertyInference.ts, 0, 0))
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(discriminantPropertyInference.ts, 5, 1))
>Unrelated : Symbol(Unrelated, Decl(discriminantPropertyInference.ts, 10, 1))

// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
g({
>g : Symbol(g, Decl(discriminantPropertyInference.ts, 39, 3))

cb: n => n.toFixed()
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 35, 3))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 36, 7))
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 45, 3))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 46, 7))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 36, 7))
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 46, 7))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

});
Expand Down
30 changes: 28 additions & 2 deletions tests/baselines/reference/discriminantPropertyInference.types
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ type DiscriminatorFalse = {
>x : number
}

type Props = DiscriminatorTrue | DiscriminatorFalse;
>Props : Props
type Unrelated = {
>Unrelated : Unrelated

val: number;
>val : number
}

declare function f(options: DiscriminatorTrue | DiscriminatorFalse): any;
>f : (options: DiscriminatorTrue | DiscriminatorFalse) => any
Expand Down Expand Up @@ -111,3 +115,25 @@ f({

});


declare function g(options: DiscriminatorTrue | DiscriminatorFalse | Unrelated): any;
>g : (options: DiscriminatorTrue | DiscriminatorFalse | Unrelated) => any
>options : DiscriminatorTrue | DiscriminatorFalse | Unrelated

// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
g({
>g({ cb: n => n.toFixed()}) : any
>g : (options: DiscriminatorTrue | DiscriminatorFalse | Unrelated) => any
>{ cb: n => n.toFixed()} : { cb: (n: number) => string; }

cb: n => n.toFixed()
>cb : (n: number) => string
>n => n.toFixed() : (n: number) => string
>n : number
>n.toFixed() : string
>n.toFixed : (fractionDigits?: number | undefined) => string
>n : number
>toFixed : (fractionDigits?: number | undefined) => string

});

15 changes: 14 additions & 1 deletion tests/baselines/reference/tsxDiscriminantPropertyInference.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,15 @@ type DiscriminatorFalse = {
cb: (x: number) => void;
}

type Unrelated = {
val: number;
}

type Props = DiscriminatorTrue | DiscriminatorFalse;

declare function Comp(props: DiscriminatorTrue | DiscriminatorFalse): JSX.Element;
type UnrelatedProps = Props | Unrelated;

declare function Comp(props: Props): JSX.Element;

// simple inference
void (<Comp disc cb={s => parseInt(s)} />);
Expand All @@ -29,6 +35,11 @@ void (<Comp disc={undefined} cb={n => n.toFixed()} />);

// requires checking type information since discriminator is missing from object
void (<Comp cb={n => n.toFixed()} />);

declare function UnrelatedComp(props: UnrelatedProps): JSX.Element;

// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
void (<Comp cb={n => n.toFixed()} />);


//// [tsxDiscriminantPropertyInference.jsx]
Expand All @@ -40,3 +51,5 @@ void (<Comp disc={false} cb={function (n) { return n.toFixed(); }}/>);
void (<Comp disc={undefined} cb={function (n) { return n.toFixed(); }}/>);
// requires checking type information since discriminator is missing from object
void (<Comp cb={function (n) { return n.toFixed(); }}/>);
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
void (<Comp cb={function (n) { return n.toFixed(); }}/>);
77 changes: 52 additions & 25 deletions tests/baselines/reference/tsxDiscriminantPropertyInference.symbols
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,55 +29,82 @@ type DiscriminatorFalse = {
>x : Symbol(x, Decl(tsxDiscriminantPropertyInference.tsx, 12, 9))
}

type Unrelated = {
>Unrelated : Symbol(Unrelated, Decl(tsxDiscriminantPropertyInference.tsx, 13, 1))

val: number;
>val : Symbol(val, Decl(tsxDiscriminantPropertyInference.tsx, 15, 18))
}

type Props = DiscriminatorTrue | DiscriminatorFalse;
>Props : Symbol(Props, Decl(tsxDiscriminantPropertyInference.tsx, 13, 1))
>Props : Symbol(Props, Decl(tsxDiscriminantPropertyInference.tsx, 17, 1))
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(tsxDiscriminantPropertyInference.tsx, 3, 1))
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(tsxDiscriminantPropertyInference.tsx, 8, 1))

declare function Comp(props: DiscriminatorTrue | DiscriminatorFalse): JSX.Element;
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 15, 52))
>props : Symbol(props, Decl(tsxDiscriminantPropertyInference.tsx, 17, 22))
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(tsxDiscriminantPropertyInference.tsx, 3, 1))
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(tsxDiscriminantPropertyInference.tsx, 8, 1))
type UnrelatedProps = Props | Unrelated;
>UnrelatedProps : Symbol(UnrelatedProps, Decl(tsxDiscriminantPropertyInference.tsx, 19, 52))
>Props : Symbol(Props, Decl(tsxDiscriminantPropertyInference.tsx, 17, 1))
>Unrelated : Symbol(Unrelated, Decl(tsxDiscriminantPropertyInference.tsx, 13, 1))

declare function Comp(props: Props): JSX.Element;
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 21, 40))
>props : Symbol(props, Decl(tsxDiscriminantPropertyInference.tsx, 23, 22))
>Props : Symbol(Props, Decl(tsxDiscriminantPropertyInference.tsx, 17, 1))
>JSX : Symbol(JSX, Decl(tsxDiscriminantPropertyInference.tsx, 0, 0))
>Element : Symbol(JSX.Element, Decl(tsxDiscriminantPropertyInference.tsx, 1, 15))

// simple inference
void (<Comp disc cb={s => parseInt(s)} />);
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 15, 52))
>disc : Symbol(disc, Decl(tsxDiscriminantPropertyInference.tsx, 20, 11))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 20, 16))
>s : Symbol(s, Decl(tsxDiscriminantPropertyInference.tsx, 20, 21))
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 21, 40))
>disc : Symbol(disc, Decl(tsxDiscriminantPropertyInference.tsx, 26, 11))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 26, 16))
>s : Symbol(s, Decl(tsxDiscriminantPropertyInference.tsx, 26, 21))
>parseInt : Symbol(parseInt, Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(tsxDiscriminantPropertyInference.tsx, 20, 21))
>s : Symbol(s, Decl(tsxDiscriminantPropertyInference.tsx, 26, 21))

// simple inference
void (<Comp disc={false} cb={n => n.toFixed()} />);
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 15, 52))
>disc : Symbol(disc, Decl(tsxDiscriminantPropertyInference.tsx, 23, 11))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 23, 24))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 23, 29))
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 21, 40))
>disc : Symbol(disc, Decl(tsxDiscriminantPropertyInference.tsx, 29, 11))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 29, 24))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 29, 29))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 23, 29))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 29, 29))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

// simple inference when strict-null-checks are enabled
void (<Comp disc={undefined} cb={n => n.toFixed()} />);
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 15, 52))
>disc : Symbol(disc, Decl(tsxDiscriminantPropertyInference.tsx, 26, 11))
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 21, 40))
>disc : Symbol(disc, Decl(tsxDiscriminantPropertyInference.tsx, 32, 11))
>undefined : Symbol(undefined)
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 26, 28))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 26, 33))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 32, 28))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 32, 33))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 26, 33))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 32, 33))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

// requires checking type information since discriminator is missing from object
void (<Comp cb={n => n.toFixed()} />);
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 15, 52))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 29, 11))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 29, 16))
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 21, 40))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 35, 11))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 35, 16))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 35, 16))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

declare function UnrelatedComp(props: UnrelatedProps): JSX.Element;
>UnrelatedComp : Symbol(UnrelatedComp, Decl(tsxDiscriminantPropertyInference.tsx, 35, 38))
>props : Symbol(props, Decl(tsxDiscriminantPropertyInference.tsx, 37, 31))
>UnrelatedProps : Symbol(UnrelatedProps, Decl(tsxDiscriminantPropertyInference.tsx, 19, 52))
>JSX : Symbol(JSX, Decl(tsxDiscriminantPropertyInference.tsx, 0, 0))
>Element : Symbol(JSX.Element, Decl(tsxDiscriminantPropertyInference.tsx, 1, 15))

// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
void (<Comp cb={n => n.toFixed()} />);
>Comp : Symbol(Comp, Decl(tsxDiscriminantPropertyInference.tsx, 21, 40))
>cb : Symbol(cb, Decl(tsxDiscriminantPropertyInference.tsx, 40, 11))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 40, 16))
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 29, 16))
>n : Symbol(n, Decl(tsxDiscriminantPropertyInference.tsx, 40, 16))
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

0 comments on commit 751c114

Please sign in to comment.