Skip to content

Commit caa081c

Browse files
erikbrinkmanerikbrinkman
committed
Fix contextual discrimination for omitted members
The first attempt at fixing this issue caused a large regression on one benchmark. This new PR addresses the regression by caching optional members of union types when they're used for discrimination. This seems to reduce the performance hit at the cost of some extra memory, but it's not clear if the fix is worth this regression.
1 parent f4d76e5 commit caa081c

10 files changed

+307
-83
lines changed

‎src/compiler/checker.ts

Lines changed: 89 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -26853,34 +26853,103 @@ namespace ts {
2685326853
return false;
2685426854
}
2685526855

26856+
/** get proprty names from a union type that are optional on any individual type */
26857+
function getCachedDiscriminantOptionalPropertyNames(contextualType: UnionType): __String[] {
26858+
const cached = contextualType.discriminantOptionalPropertyNames;
26859+
if (cached !== undefined) {
26860+
return cached;
26861+
}
26862+
else {
26863+
const optionalNames = new Set(
26864+
flatMap(contextualType.types, (memberType) =>
26865+
map(
26866+
filter(getPropertiesOfType(memberType), s => !!(s.flags & SymbolFlags.Optional)),
26867+
s => s.escapedName
26868+
)
26869+
)
26870+
);
26871+
const potentialNames: __String[] = [];
26872+
optionalNames.forEach((escapedName) => {
26873+
if (isDiscriminantProperty(contextualType, escapedName)) {
26874+
potentialNames.push(escapedName);
26875+
}
26876+
});
26877+
return contextualType.discriminantOptionalPropertyNames = potentialNames;
26878+
}
26879+
}
26880+
2685626881
function discriminateContextualTypeByObjectMembers(node: ObjectLiteralExpression, contextualType: UnionType) {
26857-
return getMatchingUnionConstituentForObjectLiteral(contextualType, node) || discriminateTypeByDiscriminableItems(contextualType,
26858-
concatenate(
26859-
map(
26860-
filter(node.properties, p => !!p.symbol && p.kind === SyntaxKind.PropertyAssignment && isPossiblyDiscriminantValue(p.initializer) && isDiscriminantProperty(contextualType, p.symbol.escapedName)),
26861-
prop => ([() => getContextFreeTypeOfExpression((prop as PropertyAssignment).initializer), prop.symbol.escapedName] as [() => Type, __String])
26882+
const nodeSymbolMembers = node?.symbol?.members;
26883+
return (
26884+
getMatchingUnionConstituentForObjectLiteral(contextualType, node) ||
26885+
discriminateTypeByDiscriminableItems(
26886+
contextualType,
26887+
concatenate(
26888+
map(
26889+
filter(
26890+
node.properties,
26891+
p =>
26892+
!!p.symbol &&
26893+
p.kind === SyntaxKind.PropertyAssignment &&
26894+
isPossiblyDiscriminantValue(p.initializer) &&
26895+
isDiscriminantProperty(contextualType, p.symbol.escapedName)
26896+
),
26897+
prop =>
26898+
[
26899+
() => getContextFreeTypeOfExpression((prop as PropertyAssignment).initializer),
26900+
prop.symbol.escapedName,
26901+
] as [() => Type, __String]
26902+
),
26903+
nodeSymbolMembers
26904+
? map(
26905+
filter(
26906+
getCachedDiscriminantOptionalPropertyNames(contextualType),
26907+
name => !nodeSymbolMembers.has(name)
26908+
),
26909+
name => [() => undefinedType, name] as [() => Type, __String]
26910+
)
26911+
: []
2686226912
),
26863-
map(
26864-
filter(getPropertiesOfType(contextualType), s => !!(s.flags & SymbolFlags.Optional) && !!node?.symbol?.members && !node.symbol.members.has(s.escapedName) && isDiscriminantProperty(contextualType, s.escapedName)),
26865-
s => [() => undefinedType, s.escapedName] as [() => Type, __String]
26866-
)
26867-
),
26868-
isTypeAssignableTo,
26869-
contextualType
26913+
isTypeAssignableTo,
26914+
contextualType
26915+
)
2687026916
);
2687126917
}
2687226918

26873-
function discriminateContextualTypeByJSXAttributes(node: JsxAttributes, contextualType: UnionType) {
26874-
return discriminateTypeByDiscriminableItems(contextualType,
26919+
function discriminateContextualTypeByJSXAttributes(
26920+
node: JsxAttributes,
26921+
contextualType: UnionType
26922+
) {
26923+
const nodeSymbolMembers = node?.symbol?.members;
26924+
return discriminateTypeByDiscriminableItems(
26925+
contextualType,
2687526926
concatenate(
2687626927
map(
26877-
filter(node.properties, p => !!p.symbol && p.kind === SyntaxKind.JsxAttribute && isDiscriminantProperty(contextualType, p.symbol.escapedName) && (!p.initializer || isPossiblyDiscriminantValue(p.initializer))),
26878-
prop => ([!(prop as JsxAttribute).initializer ? (() => trueType) : (() => getContextFreeTypeOfExpression((prop as JsxAttribute).initializer!)), prop.symbol.escapedName] as [() => Type, __String])
26928+
filter(
26929+
node.properties,
26930+
p =>
26931+
!!p.symbol &&
26932+
p.kind === SyntaxKind.JsxAttribute &&
26933+
isDiscriminantProperty(contextualType, p.symbol.escapedName) &&
26934+
(!p.initializer || isPossiblyDiscriminantValue(p.initializer))
26935+
),
26936+
prop =>
26937+
[
26938+
!(prop as JsxAttribute).initializer
26939+
? () => trueType
26940+
: () => getContextFreeTypeOfExpression((prop as JsxAttribute).initializer!),
26941+
prop.symbol.escapedName,
26942+
] as [() => Type, __String]
2687926943
),
26880-
map(
26881-
filter(getPropertiesOfType(contextualType), s => !!(s.flags & SymbolFlags.Optional) && !!node?.symbol?.members && !node.symbol.members.has(s.escapedName) && isDiscriminantProperty(contextualType, s.escapedName)),
26882-
s => [() => undefinedType, s.escapedName] as [() => Type, __String]
26883-
)
26944+
nodeSymbolMembers
26945+
? map(
26946+
filter(
26947+
getCachedDiscriminantOptionalPropertyNames(contextualType),
26948+
(name) => !nodeSymbolMembers.has(name)
26949+
),
26950+
(name) => [() => undefinedType, name] as [() => Type, __String]
26951+
)
26952+
: []
2688426953
),
2688526954
isTypeAssignableTo,
2688626955
contextualType

‎src/compiler/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5532,6 +5532,8 @@ namespace ts {
55325532
keyPropertyName?: __String; // Property with unique unit type that exists in every object/intersection in union type
55335533
/* @internal */
55345534
constituentMap?: ESMap<TypeId, Type>; // Constituents keyed by unit type discriminants
5535+
/* @internal names of discriminant properties that are optional on at least one type */
5536+
discriminantOptionalPropertyNames?: __String[];
55355537
}
55365538

55375539
export interface IntersectionType extends UnionOrIntersectionType {

‎tests/baselines/reference/discriminantPropertyInference.js

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ type DiscriminatorFalse = {
1111
cb: (x: number) => void;
1212
}
1313

14-
type Props = DiscriminatorTrue | DiscriminatorFalse;
14+
type Unrelated = {
15+
val: number;
16+
}
1517

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

@@ -37,6 +39,14 @@ f({
3739
f({
3840
cb: n => n.toFixed()
3941
});
42+
43+
44+
declare function g(options: DiscriminatorTrue | DiscriminatorFalse | Unrelated): any;
45+
46+
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
47+
g({
48+
cb: n => n.toFixed()
49+
});
4050

4151

4252
//// [discriminantPropertyInference.js]
@@ -60,3 +70,7 @@ f({
6070
f({
6171
cb: function (n) { return n.toFixed(); }
6272
});
73+
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
74+
g({
75+
cb: function (n) { return n.toFixed(); }
76+
});

‎tests/baselines/reference/discriminantPropertyInference.symbols

Lines changed: 48 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -23,74 +23,97 @@ type DiscriminatorFalse = {
2323
>x : Symbol(x, Decl(discriminantPropertyInference.ts, 9, 9))
2424
}
2525

26-
type Props = DiscriminatorTrue | DiscriminatorFalse;
27-
>Props : Symbol(Props, Decl(discriminantPropertyInference.ts, 10, 1))
28-
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(discriminantPropertyInference.ts, 0, 0))
29-
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(discriminantPropertyInference.ts, 5, 1))
26+
type Unrelated = {
27+
>Unrelated : Symbol(Unrelated, Decl(discriminantPropertyInference.ts, 10, 1))
28+
29+
val: number;
30+
>val : Symbol(val, Decl(discriminantPropertyInference.ts, 12, 18))
31+
}
3032

3133
declare function f(options: DiscriminatorTrue | DiscriminatorFalse): any;
32-
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
33-
>options : Symbol(options, Decl(discriminantPropertyInference.ts, 14, 19))
34+
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))
35+
>options : Symbol(options, Decl(discriminantPropertyInference.ts, 16, 19))
3436
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(discriminantPropertyInference.ts, 0, 0))
3537
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(discriminantPropertyInference.ts, 5, 1))
3638

3739
// simple inference
3840
f({
39-
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
41+
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))
4042

4143
disc: true,
42-
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 17, 3))
44+
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 19, 3))
4345

4446
cb: s => parseInt(s)
45-
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 18, 15))
46-
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 19, 7))
47+
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 20, 15))
48+
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 21, 7))
4749
>parseInt : Symbol(parseInt, Decl(lib.es5.d.ts, --, --))
48-
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 19, 7))
50+
>s : Symbol(s, Decl(discriminantPropertyInference.ts, 21, 7))
4951

5052
});
5153

5254
// simple inference
5355
f({
54-
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
56+
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))
5557

5658
disc: false,
57-
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 23, 3))
59+
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 25, 3))
5860

5961
cb: n => n.toFixed()
60-
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 24, 16))
61-
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 25, 7))
62+
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 26, 16))
63+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 27, 7))
6264
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
63-
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 25, 7))
65+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 27, 7))
6466
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
6567

6668
});
6769

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

7274
disc: undefined,
73-
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 29, 3))
75+
>disc : Symbol(disc, Decl(discriminantPropertyInference.ts, 31, 3))
7476
>undefined : Symbol(undefined)
7577

7678
cb: n => n.toFixed()
77-
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 30, 20))
78-
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 31, 7))
79+
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 32, 20))
80+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 33, 7))
7981
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
80-
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 31, 7))
82+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 33, 7))
8183
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
8284

8385
});
8486

8587
// requires checking type information since discriminator is missing from object
8688
f({
87-
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 12, 52))
89+
>f : Symbol(f, Decl(discriminantPropertyInference.ts, 14, 1))
90+
91+
cb: n => n.toFixed()
92+
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 37, 3))
93+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 38, 7))
94+
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
95+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 38, 7))
96+
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
97+
98+
});
99+
100+
101+
declare function g(options: DiscriminatorTrue | DiscriminatorFalse | Unrelated): any;
102+
>g : Symbol(g, Decl(discriminantPropertyInference.ts, 39, 3))
103+
>options : Symbol(options, Decl(discriminantPropertyInference.ts, 42, 19))
104+
>DiscriminatorTrue : Symbol(DiscriminatorTrue, Decl(discriminantPropertyInference.ts, 0, 0))
105+
>DiscriminatorFalse : Symbol(DiscriminatorFalse, Decl(discriminantPropertyInference.ts, 5, 1))
106+
>Unrelated : Symbol(Unrelated, Decl(discriminantPropertyInference.ts, 10, 1))
107+
108+
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
109+
g({
110+
>g : Symbol(g, Decl(discriminantPropertyInference.ts, 39, 3))
88111

89112
cb: n => n.toFixed()
90-
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 35, 3))
91-
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 36, 7))
113+
>cb : Symbol(cb, Decl(discriminantPropertyInference.ts, 45, 3))
114+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 46, 7))
92115
>n.toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
93-
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 36, 7))
116+
>n : Symbol(n, Decl(discriminantPropertyInference.ts, 46, 7))
94117
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
95118

96119
});

‎tests/baselines/reference/discriminantPropertyInference.types

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,12 @@ type DiscriminatorFalse = {
2525
>x : number
2626
}
2727

28-
type Props = DiscriminatorTrue | DiscriminatorFalse;
29-
>Props : Props
28+
type Unrelated = {
29+
>Unrelated : Unrelated
30+
31+
val: number;
32+
>val : number
33+
}
3034

3135
declare function f(options: DiscriminatorTrue | DiscriminatorFalse): any;
3236
>f : (options: DiscriminatorTrue | DiscriminatorFalse) => any
@@ -111,3 +115,25 @@ f({
111115

112116
});
113117

118+
119+
declare function g(options: DiscriminatorTrue | DiscriminatorFalse | Unrelated): any;
120+
>g : (options: DiscriminatorTrue | DiscriminatorFalse | Unrelated) => any
121+
>options : DiscriminatorTrue | DiscriminatorFalse | Unrelated
122+
123+
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
124+
g({
125+
>g({ cb: n => n.toFixed()}) : any
126+
>g : (options: DiscriminatorTrue | DiscriminatorFalse | Unrelated) => any
127+
>{ cb: n => n.toFixed()} : { cb: (n: number) => string; }
128+
129+
cb: n => n.toFixed()
130+
>cb : (n: number) => string
131+
>n => n.toFixed() : (n: number) => string
132+
>n : number
133+
>n.toFixed() : string
134+
>n.toFixed : (fractionDigits?: number | undefined) => string
135+
>n : number
136+
>toFixed : (fractionDigits?: number | undefined) => string
137+
138+
});
139+

‎tests/baselines/reference/tsxDiscriminantPropertyInference.js

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,15 @@ type DiscriminatorFalse = {
1414
cb: (x: number) => void;
1515
}
1616

17+
type Unrelated = {
18+
val: number;
19+
}
20+
1721
type Props = DiscriminatorTrue | DiscriminatorFalse;
1822

19-
declare function Comp(props: DiscriminatorTrue | DiscriminatorFalse): JSX.Element;
23+
type UnrelatedProps = Props | Unrelated;
24+
25+
declare function Comp(props: Props): JSX.Element;
2026

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

3036
// requires checking type information since discriminator is missing from object
3137
void (<Comp cb={n => n.toFixed()} />);
38+
39+
declare function UnrelatedComp(props: UnrelatedProps): JSX.Element;
40+
41+
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
42+
void (<Comp cb={n => n.toFixed()} />);
3243

3344

3445
//// [tsxDiscriminantPropertyInference.jsx]
@@ -40,3 +51,5 @@ void (<Comp disc={false} cb={function (n) { return n.toFixed(); }}/>);
4051
void (<Comp disc={undefined} cb={function (n) { return n.toFixed(); }}/>);
4152
// requires checking type information since discriminator is missing from object
4253
void (<Comp cb={function (n) { return n.toFixed(); }}/>);
54+
// requires checking properties of all types, rather than properties of just the union type (e.g. only intersection)
55+
void (<Comp cb={function (n) { return n.toFixed(); }}/>);

0 commit comments

Comments
 (0)