Skip to content

Commit cd00855

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 cd00855

10 files changed

+315
-82
lines changed

‎src/compiler/checker.ts

+92-19
Original file line numberDiff line numberDiff line change
@@ -26853,34 +26853,107 @@ namespace ts {
2685326853
return false;
2685426854
}
2685526855

26856+
/**
26857+
* get discriminators for missing properties on node that are optional for any member of the union context type
26858+
*
26859+
* Because we know in advance which types each member can potentially
26860+
* discriminate against, we only have to choose one property among the
26861+
* set that discriminates between the same types in the union.
26862+
*
26863+
* @internal
26864+
*/
26865+
function getOptionalPropertyDiscriminators(node: ObjectLiteralExpression | JsxAttributes, contextualType: UnionType): [() => Type, __String][] | undefined {
26866+
const nodeSymbolMembers = node?.symbol?.members;
26867+
if (!nodeSymbolMembers) {
26868+
return undefined;
26869+
}
26870+
26871+
let discriminantNames = contextualType.discriminantOptionalPropertyNames;
26872+
if (!discriminantNames) {
26873+
const optionalNames = new Map<__String, number[]>();
26874+
let index = 0;
26875+
for (const memberType of contextualType.types) {
26876+
for (const s of getPropertiesOfType(memberType)) {
26877+
if (s.flags & SymbolFlags.Optional) {
26878+
const name = s.escapedName;
26879+
const inds = optionalNames.get(name);
26880+
inds ? inds.push(index) : optionalNames.set(name, [index]);
26881+
}
26882+
}
26883+
++index;
26884+
}
26885+
26886+
const grouped = new Map<string, __String[]>();
26887+
optionalNames.forEach((inds, escapedName) => {
26888+
const key = inds.join(" ");
26889+
const names = grouped.get(key);
26890+
names ? names.push(escapedName) : grouped.set(key, [escapedName]);
26891+
});
26892+
const names: __String[][] = [];
26893+
grouped.forEach(nameGroup => names.push(nameGroup));
26894+
discriminantNames = contextualType.discriminantOptionalPropertyNames = names;
26895+
}
26896+
26897+
return map(
26898+
filter(
26899+
map(discriminantNames, names => names.find(name => !nodeSymbolMembers.has(name))),
26900+
name => !!name // remove undefineds to select only missing members
26901+
),
26902+
name => [() => undefinedType, name] as [() => Type, __String],
26903+
);
26904+
}
26905+
2685626906
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])
26907+
return (
26908+
getMatchingUnionConstituentForObjectLiteral(contextualType, node) ||
26909+
discriminateTypeByDiscriminableItems(
26910+
contextualType,
26911+
concatenate(
26912+
map(
26913+
filter(
26914+
node.properties,
26915+
p =>
26916+
!!p.symbol &&
26917+
p.kind === SyntaxKind.PropertyAssignment &&
26918+
isPossiblyDiscriminantValue(p.initializer) &&
26919+
isDiscriminantProperty(contextualType, p.symbol.escapedName)
26920+
),
26921+
prop =>
26922+
[
26923+
() => getContextFreeTypeOfExpression((prop as PropertyAssignment).initializer),
26924+
prop.symbol.escapedName,
26925+
] as [() => Type, __String]
26926+
),
26927+
getOptionalPropertyDiscriminators(node, contextualType),
2686226928
),
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
26929+
isTypeAssignableTo,
26930+
contextualType
26931+
)
2687026932
);
2687126933
}
2687226934

2687326935
function discriminateContextualTypeByJSXAttributes(node: JsxAttributes, contextualType: UnionType) {
26874-
return discriminateTypeByDiscriminableItems(contextualType,
26936+
return discriminateTypeByDiscriminableItems(
26937+
contextualType,
2687526938
concatenate(
2687626939
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])
26940+
filter(
26941+
node.properties,
26942+
p =>
26943+
!!p.symbol &&
26944+
p.kind === SyntaxKind.JsxAttribute &&
26945+
isDiscriminantProperty(contextualType, p.symbol.escapedName) &&
26946+
(!p.initializer || isPossiblyDiscriminantValue(p.initializer))
26947+
),
26948+
prop =>
26949+
[
26950+
!(prop as JsxAttribute).initializer
26951+
? () => trueType
26952+
: () => getContextFreeTypeOfExpression((prop as JsxAttribute).initializer!),
26953+
prop.symbol.escapedName,
26954+
] as [() => Type, __String]
2687926955
),
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-
)
26956+
getOptionalPropertyDiscriminators(node, contextualType),
2688426957
),
2688526958
isTypeAssignableTo,
2688626959
contextualType

‎src/compiler/types.ts

+7
Original file line numberDiff line numberDiff line change
@@ -5532,6 +5532,13 @@ 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+
/**
5536+
* names of discriminant properties that are optional on at least one type
5537+
*
5538+
* These are grouped by the set of types they discriminate to speed up checks
5539+
* @internal
5540+
*/
5541+
discriminantOptionalPropertyNames?: __String[][];
55355542
}
55365543

55375544
export interface IntersectionType extends UnionOrIntersectionType {

‎tests/baselines/reference/discriminantPropertyInference.js

+15-1
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

+48-25
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

+28-2
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

+14-1
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)