Skip to content

Commit 35bbfdc

Browse files
sandersnsandersn
committed
More precise property-overwritten-by-spread errors
Trying to do this check in getSpreadType just doesn't have enough information, so I moved it to checkObjectLiteral, which is a better place for issuing errors anyway. Unfortunately, the approach is kind of expensive in that it 1. creates a new map for each property and 2. iterates over all properties of the spread type, even if it's a union. I have some ideas to improve (1) that might work out. I'm not sure how bad (2) is since we're going to iterate over all properties of all constituents of a union. Fixes #36779
1 parent 3fdce8e commit 35bbfdc

7 files changed

Lines changed: 143 additions & 17 deletions

‎src/compiler/checker.ts‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13154,7 +13154,7 @@ namespace ts {
1315413154
* this function should be called in a left folding style, with left = previous result of getSpreadType
1315513155
* and right = the new element to be spread.
1315613156
*/
13157-
function getSpreadType(left: Type, right: Type, symbol: Symbol | undefined, objectFlags: ObjectFlags, readonly: boolean, isParentTypeNullable?: boolean): Type {
13157+
function getSpreadType(left: Type, right: Type, symbol: Symbol | undefined, objectFlags: ObjectFlags, readonly: boolean): Type {
1315813158
if (left.flags & TypeFlags.Any || right.flags & TypeFlags.Any) {
1315913159
return anyType;
1316013160
}
@@ -13170,16 +13170,16 @@ namespace ts {
1317013170
if (left.flags & TypeFlags.Union) {
1317113171
const merged = tryMergeUnionOfObjectTypeAndEmptyObject(left as UnionType, readonly);
1317213172
if (merged) {
13173-
return getSpreadType(merged, right, symbol, objectFlags, readonly, isParentTypeNullable);
13173+
return getSpreadType(merged, right, symbol, objectFlags, readonly);
1317413174
}
13175-
return mapType(left, t => getSpreadType(t, right, symbol, objectFlags, readonly, isParentTypeNullable));
13175+
return mapType(left, t => getSpreadType(t, right, symbol, objectFlags, readonly));
1317613176
}
1317713177
if (right.flags & TypeFlags.Union) {
1317813178
const merged = tryMergeUnionOfObjectTypeAndEmptyObject(right as UnionType, readonly);
1317913179
if (merged) {
13180-
return getSpreadType(left, merged, symbol, objectFlags, readonly, maybeTypeOfKind(right, TypeFlags.Nullable));
13180+
return getSpreadType(left, merged, symbol, objectFlags, readonly);
1318113181
}
13182-
return mapType(right, t => getSpreadType(left, t, symbol, objectFlags, readonly, maybeTypeOfKind(right, TypeFlags.Nullable)));
13182+
return mapType(right, t => getSpreadType(left, t, symbol, objectFlags, readonly));
1318313183
}
1318413184
if (right.flags & (TypeFlags.BooleanLike | TypeFlags.NumberLike | TypeFlags.BigIntLike | TypeFlags.StringLike | TypeFlags.EnumLike | TypeFlags.NonPrimitive | TypeFlags.Index)) {
1318513185
return left;
@@ -13243,14 +13243,6 @@ namespace ts {
1324313243
result.nameType = getSymbolLinks(leftProp).nameType;
1324413244
members.set(leftProp.escapedName, result);
1324513245
}
13246-
else if (strictNullChecks &&
13247-
!isParentTypeNullable &&
13248-
symbol &&
13249-
!isFromSpreadAssignment(leftProp, symbol) &&
13250-
isFromSpreadAssignment(rightProp, symbol) &&
13251-
!maybeTypeOfKind(rightType, TypeFlags.Nullable)) {
13252-
error(leftProp.valueDeclaration, Diagnostics._0_is_specified_more_than_once_so_this_usage_will_be_overwritten, unescapeLeadingUnderscores(leftProp.escapedName));
13253-
}
1325413246
}
1325513247
else {
1325613248
members.set(leftProp.escapedName, getSpreadSymbol(leftProp, readonly));
@@ -16781,10 +16773,6 @@ namespace ts {
1678116773
return match === -1 || discriminable.indexOf(/*searchElement*/ true, match + 1) !== -1 ? defaultValue : target.types[match];
1678216774
}
1678316775

16784-
function isFromSpreadAssignment(prop: Symbol, container: Symbol) {
16785-
return prop.valueDeclaration?.parent !== container.valueDeclaration;
16786-
}
16787-
1678816776
/**
1678916777
* A type is 'weak' if it is an object type with at least one optional property
1679016778
* and no required properties, call/construct signatures or index signatures
@@ -22560,6 +22548,7 @@ namespace ts {
2256022548
let patternWithComputedProperties = false;
2256122549
let hasComputedStringProperty = false;
2256222550
let hasComputedNumberProperty = false;
22551+
const propertyDeclarations = createMap<Symbol>();
2256322552
propertiesTable = createSymbolTable();
2256422553

2256522554
let offset = 0;
@@ -22626,6 +22615,7 @@ namespace ts {
2262622615
prop.type = type;
2262722616
prop.target = member;
2262822617
member = prop;
22618+
propertyDeclarations.set(prop.escapedName as string, prop);
2262922619
}
2263022620
else if (memberDecl.kind === SyntaxKind.SpreadAssignment) {
2263122621
if (languageVersion < ScriptTarget.ES2015) {
@@ -22643,6 +22633,16 @@ namespace ts {
2264322633
error(memberDecl, Diagnostics.Spread_types_may_only_be_created_from_object_types);
2264422634
return errorType;
2264522635
}
22636+
for (const right of getPropertiesOfType(type)) {
22637+
const rightType = getTypeOfSymbol(right);
22638+
const left = propertyDeclarations.get(right.escapedName as string);
22639+
if (strictNullChecks &&
22640+
left &&
22641+
!maybeTypeOfKind(rightType, TypeFlags.Nullable)) {
22642+
error(left.valueDeclaration, Diagnostics._0_is_specified_more_than_once_so_this_usage_will_be_overwritten, unescapeLeadingUnderscores(left.escapedName));
22643+
}
22644+
}
22645+
2264622646
spread = getSpreadType(spread, type, node.symbol, objectFlags, inConstContext);
2264722647
offset = i + 1;
2264822648
continue;

‎tests/baselines/reference/objectSpreadSetonlyAccessor.errors.txt‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
tests/cases/conformance/types/spread/objectSpreadSetonlyAccessor.ts(2,34): error TS2783: 'foo' is specified more than once, so this usage will be overwritten.
2+
3+
4+
==== tests/cases/conformance/types/spread/objectSpreadSetonlyAccessor.ts (1 errors) ====
5+
const o1: { foo: number, bar: undefined } = { foo: 1, ... { set bar(_v: number) { } } }
6+
const o2: { foo: undefined } = { foo: 1, ... { set foo(_v: number) { } } }
7+
~~~~~~
8+
!!! error TS2783: 'foo' is specified more than once, so this usage will be overwritten.
9+

‎tests/baselines/reference/spreadOverwritesPropertyStrict.errors.txt‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,13 @@ tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts(15,14): e
2323
~~~~
2424
!!! error TS2783: 'x' is specified more than once, so this usage will be overwritten.
2525
}
26+
function i(b: boolean, t: { command: string, ok: string }) {
27+
return { command: "hi", ...(b ? t : {}) } // ok
28+
}
29+
function j() {
30+
return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
31+
}
32+
function k(b: boolean, t: { command: string, ok: string }) {
33+
return { command: "hi", ...{ spoiler: true }, ...(b ? t : {}) } // ok
34+
}
2635

‎tests/baselines/reference/spreadOverwritesPropertyStrict.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@ function f(obj: { x: number } | undefined) {
1515
function h(obj: { x: number } | { x: string }) {
1616
return { x: 1, ...obj } // error
1717
}
18+
function i(b: boolean, t: { command: string, ok: string }) {
19+
return { command: "hi", ...(b ? t : {}) } // ok
20+
}
21+
function j() {
22+
return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
23+
}
24+
function k(b: boolean, t: { command: string, ok: string }) {
25+
return { command: "hi", ...{ spoiler: true }, ...(b ? t : {}) } // ok
26+
}
1827

1928

2029
//// [spreadOverwritesPropertyStrict.js]
@@ -44,3 +53,12 @@ function f(obj) {
4453
function h(obj) {
4554
return __assign({ x: 1 }, obj); // error
4655
}
56+
function i(b, t) {
57+
return __assign({ command: "hi" }, (b ? t : {})); // ok
58+
}
59+
function j() {
60+
return __assign({ command: "hi" }, { command: "bye" }); // ok
61+
}
62+
function k(b, t) {
63+
return __assign(__assign({ command: "hi" }, { spoiler: true }), (b ? t : {})); // ok
64+
}

‎tests/baselines/reference/spreadOverwritesPropertyStrict.symbols‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,4 +62,36 @@ function h(obj: { x: number } | { x: string }) {
6262
>x : Symbol(x, Decl(spreadOverwritesPropertyStrict.ts, 14, 12))
6363
>obj : Symbol(obj, Decl(spreadOverwritesPropertyStrict.ts, 13, 11))
6464
}
65+
function i(b: boolean, t: { command: string, ok: string }) {
66+
>i : Symbol(i, Decl(spreadOverwritesPropertyStrict.ts, 15, 1))
67+
>b : Symbol(b, Decl(spreadOverwritesPropertyStrict.ts, 16, 11))
68+
>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 16, 22))
69+
>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 16, 27))
70+
>ok : Symbol(ok, Decl(spreadOverwritesPropertyStrict.ts, 16, 44))
71+
72+
return { command: "hi", ...(b ? t : {}) } // ok
73+
>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 17, 12))
74+
>b : Symbol(b, Decl(spreadOverwritesPropertyStrict.ts, 16, 11))
75+
>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 16, 22))
76+
}
77+
function j() {
78+
>j : Symbol(j, Decl(spreadOverwritesPropertyStrict.ts, 18, 1))
79+
80+
return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
81+
>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 20, 17))
82+
>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 20, 40))
83+
}
84+
function k(b: boolean, t: { command: string, ok: string }) {
85+
>k : Symbol(k, Decl(spreadOverwritesPropertyStrict.ts, 21, 1))
86+
>b : Symbol(b, Decl(spreadOverwritesPropertyStrict.ts, 22, 11))
87+
>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 22, 22))
88+
>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 22, 27))
89+
>ok : Symbol(ok, Decl(spreadOverwritesPropertyStrict.ts, 22, 44))
90+
91+
return { command: "hi", ...{ spoiler: true }, ...(b ? t : {}) } // ok
92+
>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 23, 12))
93+
>spoiler : Symbol(spoiler, Decl(spreadOverwritesPropertyStrict.ts, 23, 32))
94+
>b : Symbol(b, Decl(spreadOverwritesPropertyStrict.ts, 22, 11))
95+
>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 22, 22))
96+
}
6597

‎tests/baselines/reference/spreadOverwritesPropertyStrict.types‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,53 @@ function h(obj: { x: number } | { x: string }) {
7777
>1 : 1
7878
>obj : { x: number; } | { x: string; }
7979
}
80+
function i(b: boolean, t: { command: string, ok: string }) {
81+
>i : (b: boolean, t: { command: string; ok: string; }) => { command: string; ok: string; } | { command: string; }
82+
>b : boolean
83+
>t : { command: string; ok: string; }
84+
>command : string
85+
>ok : string
86+
87+
return { command: "hi", ...(b ? t : {}) } // ok
88+
>{ command: "hi", ...(b ? t : {}) } : { command: string; ok: string; } | { command: string; }
89+
>command : string
90+
>"hi" : "hi"
91+
>(b ? t : {}) : { command: string; ok: string; } | {}
92+
>b ? t : {} : { command: string; ok: string; } | {}
93+
>b : boolean
94+
>t : { command: string; ok: string; }
95+
>{} : {}
96+
}
97+
function j() {
98+
>j : () => { command: string; }
99+
100+
return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
101+
>{ ...{ command: "hi" } , ...{ command: "bye" } } : { command: string; }
102+
>{ command: "hi" } : { command: string; }
103+
>command : string
104+
>"hi" : "hi"
105+
>{ command: "bye" } : { command: string; }
106+
>command : string
107+
>"bye" : "bye"
108+
}
109+
function k(b: boolean, t: { command: string, ok: string }) {
110+
>k : (b: boolean, t: { command: string; ok: string; }) => { command: string; ok: string; spoiler: boolean; } | { spoiler: boolean; command: string; }
111+
>b : boolean
112+
>t : { command: string; ok: string; }
113+
>command : string
114+
>ok : string
115+
116+
return { command: "hi", ...{ spoiler: true }, ...(b ? t : {}) } // ok
117+
>{ command: "hi", ...{ spoiler: true }, ...(b ? t : {}) } : { command: string; ok: string; spoiler: boolean; } | { spoiler: boolean; command: string; }
118+
>command : string
119+
>"hi" : "hi"
120+
>{ spoiler: true } : { spoiler: boolean; }
121+
>spoiler : boolean
122+
>true : true
123+
>(b ? t : {}) : { command: string; ok: string; } | {}
124+
>b ? t : {} : { command: string; ok: string; } | {}
125+
>b : boolean
126+
>t : { command: string; ok: string; }
127+
>{} : {}
128+
}
80129

‎tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,12 @@ function f(obj: { x: number } | undefined) {
1515
function h(obj: { x: number } | { x: string }) {
1616
return { x: 1, ...obj } // error
1717
}
18+
function i(b: boolean, t: { command: string, ok: string }) {
19+
return { command: "hi", ...(b ? t : {}) } // ok
20+
}
21+
function j() {
22+
return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
23+
}
24+
function k(b: boolean, t: { command: string, ok: string }) {
25+
return { command: "hi", ...{ spoiler: true }, ...(b ? t : {}) } // ok
26+
}

0 commit comments

Comments
 (0)