More precise property-overwritten-by-spread errors (#37192)

sandersn · web-flow · commit b481dd4d4b7c · 2020-03-03T15:10:19.000-08:00
* 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 * another test and rename
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -13175,7 +13175,7 @@ namespace ts {
          * this function should be called in a left folding style, with left = previous result of getSpreadType
          * and right = the new element to be spread.
          */
-        function getSpreadType(left: Type, right: Type, symbol: Symbol | undefined, objectFlags: ObjectFlags, readonly: boolean, isParentTypeNullable?: boolean): Type {
+        function getSpreadType(left: Type, right: Type, symbol: Symbol | undefined, objectFlags: ObjectFlags, readonly: boolean): Type {
             if (left.flags & TypeFlags.Any || right.flags & TypeFlags.Any) {
                 return anyType;
             }
@@ -13191,16 +13191,16 @@ namespace ts {
             if (left.flags & TypeFlags.Union) {
                 const merged = tryMergeUnionOfObjectTypeAndEmptyObject(left as UnionType, readonly);
                 if (merged) {
-                    return getSpreadType(merged, right, symbol, objectFlags, readonly, isParentTypeNullable);
+                    return getSpreadType(merged, right, symbol, objectFlags, readonly);
                 }
-                return mapType(left, t => getSpreadType(t, right, symbol, objectFlags, readonly, isParentTypeNullable));
+                return mapType(left, t => getSpreadType(t, right, symbol, objectFlags, readonly));
             }
             if (right.flags & TypeFlags.Union) {
                 const merged = tryMergeUnionOfObjectTypeAndEmptyObject(right as UnionType, readonly);
                 if (merged) {
-                    return getSpreadType(left, merged, symbol, objectFlags, readonly, maybeTypeOfKind(right, TypeFlags.Nullable));
+                    return getSpreadType(left, merged, symbol, objectFlags, readonly);
                 }
-                return mapType(right, t => getSpreadType(left, t, symbol, objectFlags, readonly, maybeTypeOfKind(right, TypeFlags.Nullable)));
+                return mapType(right, t => getSpreadType(left, t, symbol, objectFlags, readonly));
             }
             if (right.flags & (TypeFlags.BooleanLike | TypeFlags.NumberLike | TypeFlags.BigIntLike | TypeFlags.StringLike | TypeFlags.EnumLike | TypeFlags.NonPrimitive | TypeFlags.Index)) {
                 return left;
@@ -13264,14 +13264,6 @@ namespace ts {
                         result.nameType = getSymbolLinks(leftProp).nameType;
                         members.set(leftProp.escapedName, result);
                     }
-                    else if (strictNullChecks &&
-                             !isParentTypeNullable &&
-                             symbol &&
-                             !isFromSpreadAssignment(leftProp, symbol) &&
-                             isFromSpreadAssignment(rightProp, symbol) &&
-                             !maybeTypeOfKind(rightType, TypeFlags.Nullable)) {
-                        error(leftProp.valueDeclaration, Diagnostics._0_is_specified_more_than_once_so_this_usage_will_be_overwritten, unescapeLeadingUnderscores(leftProp.escapedName));
-                    }
                 }
                 else {
                     members.set(leftProp.escapedName, getSpreadSymbol(leftProp, readonly));
@@ -16802,10 +16794,6 @@ namespace ts {
             return match === -1 || discriminable.indexOf(/*searchElement*/ true, match + 1) !== -1 ? defaultValue : target.types[match];
         }
 
-        function isFromSpreadAssignment(prop: Symbol, container: Symbol) {
-            return prop.valueDeclaration?.parent !== container.valueDeclaration;
-        }
-
         /**
          * A type is 'weak' if it is an object type with at least one optional property
          * and no required properties, call/construct signatures or index signatures
@@ -22566,6 +22554,7 @@ namespace ts {
             checkGrammarObjectLiteralExpression(node, inDestructuringPattern);
 
             let propertiesTable: SymbolTable;
+            const allPropertiesTable = createSymbolTable();
             let propertiesArray: Symbol[] = [];
             let spread: Type = emptyObjectType;
 
@@ -22647,6 +22636,7 @@ namespace ts {
                     prop.type = type;
                     prop.target = member;
                     member = prop;
+                    allPropertiesTable.set(prop.escapedName, prop);
                 }
                 else if (memberDecl.kind === SyntaxKind.SpreadAssignment) {
                     if (languageVersion < ScriptTarget.ES2015) {
@@ -22664,6 +22654,16 @@ namespace ts {
                         error(memberDecl, Diagnostics.Spread_types_may_only_be_created_from_object_types);
                         return errorType;
                     }
+                    for (const right of getPropertiesOfType(type)) {
+                        const rightType = getTypeOfSymbol(right);
+                        const left = allPropertiesTable.get(right.escapedName);
+                        if (strictNullChecks &&
+                            left &&
+                            !maybeTypeOfKind(rightType, TypeFlags.Nullable)) {
+                            error(left.valueDeclaration, Diagnostics._0_is_specified_more_than_once_so_this_usage_will_be_overwritten, unescapeLeadingUnderscores(left.escapedName));
+                        }
+                    }
+
                     spread = getSpreadType(spread, type, node.symbol, objectFlags, inConstContext);
                     offset = i + 1;
                     continue;
diff --git a/tests/baselines/reference/objectSpreadSetonlyAccessor.errors.txt b/tests/baselines/reference/objectSpreadSetonlyAccessor.errors.txt
@@ -0,0 +1,9 @@
+tests/cases/conformance/types/spread/objectSpreadSetonlyAccessor.ts(2,34): error TS2783: 'foo' is specified more than once, so this usage will be overwritten.
+
+
+==== tests/cases/conformance/types/spread/objectSpreadSetonlyAccessor.ts (1 errors) ====
+    const o1: { foo: number, bar: undefined } = { foo: 1, ... { set bar(_v: number) { } } }
+    const o2: { foo: undefined } = { foo: 1, ... { set foo(_v: number) { } } }
+                                     ~~~~~~
+!!! error TS2783: 'foo' is specified more than once, so this usage will be overwritten.
+    
diff --git a/tests/baselines/reference/spreadOverwritesPropertyStrict.errors.txt b/tests/baselines/reference/spreadOverwritesPropertyStrict.errors.txt
@@ -1,8 +1,9 @@
 tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts(3,17): error TS2783: 'b' is specified more than once, so this usage will be overwritten.
 tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts(15,14): error TS2783: 'x' is specified more than once, so this usage will be overwritten.
+tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts(24,14): error TS2783: 'command' is specified more than once, so this usage will be overwritten.
 
 
-==== tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts (2 errors) ====
+==== tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts (3 errors) ====
     declare var ab: { a: number, b: number };
     declare var abq: { a: number, b?: number };
     var unused1 = { b: 1, ...ab } // error
@@ -23,4 +24,15 @@ tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts(15,14): e
                  ~~~~
 !!! error TS2783: 'x' is specified more than once, so this usage will be overwritten.
     }
+    function i(b: boolean, t: { command: string, ok: string }) {
+        return { command: "hi", ...(b ? t : {}) } // ok
+    }
+    function j() {
+        return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
+    }
+    function k(t: { command: string, ok: string }) {
+        return { command: "hi", ...{ spoiler: true }, spoiler2: true, ...t } // error
+                 ~~~~~~~~~~~~~
+!!! error TS2783: 'command' is specified more than once, so this usage will be overwritten.
+    }
     
diff --git a/tests/baselines/reference/spreadOverwritesPropertyStrict.js b/tests/baselines/reference/spreadOverwritesPropertyStrict.js
@@ -15,6 +15,15 @@ function f(obj: { x: number } | undefined) {
 function h(obj: { x: number } | { x: string }) {
     return { x: 1, ...obj } // error
 }
+function i(b: boolean, t: { command: string, ok: string }) {
+    return { command: "hi", ...(b ? t : {}) } // ok
+}
+function j() {
+    return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
+}
+function k(t: { command: string, ok: string }) {
+    return { command: "hi", ...{ spoiler: true }, spoiler2: true, ...t } // error
+}
 
 
 //// [spreadOverwritesPropertyStrict.js]
@@ -44,3 +53,12 @@ function f(obj) {
 function h(obj) {
     return __assign({ x: 1 }, obj); // error
 }
+function i(b, t) {
+    return __assign({ command: "hi" }, (b ? t : {})); // ok
+}
+function j() {
+    return __assign({ command: "hi" }, { command: "bye" }); // ok
+}
+function k(t) {
+    return __assign(__assign(__assign({ command: "hi" }, { spoiler: true }), { spoiler2: true }), t); // error
+}
diff --git a/tests/baselines/reference/spreadOverwritesPropertyStrict.symbols b/tests/baselines/reference/spreadOverwritesPropertyStrict.symbols
@@ -62,4 +62,35 @@ function h(obj: { x: number } | { x: string }) {
 >x : Symbol(x, Decl(spreadOverwritesPropertyStrict.ts, 14, 12))
 >obj : Symbol(obj, Decl(spreadOverwritesPropertyStrict.ts, 13, 11))
 }
+function i(b: boolean, t: { command: string, ok: string }) {
+>i : Symbol(i, Decl(spreadOverwritesPropertyStrict.ts, 15, 1))
+>b : Symbol(b, Decl(spreadOverwritesPropertyStrict.ts, 16, 11))
+>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 16, 22))
+>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 16, 27))
+>ok : Symbol(ok, Decl(spreadOverwritesPropertyStrict.ts, 16, 44))
+
+    return { command: "hi", ...(b ? t : {}) } // ok
+>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 17, 12))
+>b : Symbol(b, Decl(spreadOverwritesPropertyStrict.ts, 16, 11))
+>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 16, 22))
+}
+function j() {
+>j : Symbol(j, Decl(spreadOverwritesPropertyStrict.ts, 18, 1))
+
+    return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
+>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 20, 17))
+>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 20, 40))
+}
+function k(t: { command: string, ok: string }) {
+>k : Symbol(k, Decl(spreadOverwritesPropertyStrict.ts, 21, 1))
+>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 22, 11))
+>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 22, 15))
+>ok : Symbol(ok, Decl(spreadOverwritesPropertyStrict.ts, 22, 32))
+
+    return { command: "hi", ...{ spoiler: true }, spoiler2: true, ...t } // error
+>command : Symbol(command, Decl(spreadOverwritesPropertyStrict.ts, 23, 12))
+>spoiler : Symbol(spoiler, Decl(spreadOverwritesPropertyStrict.ts, 23, 32))
+>spoiler2 : Symbol(spoiler2, Decl(spreadOverwritesPropertyStrict.ts, 23, 49))
+>t : Symbol(t, Decl(spreadOverwritesPropertyStrict.ts, 22, 11))
+}
 
diff --git a/tests/baselines/reference/spreadOverwritesPropertyStrict.types b/tests/baselines/reference/spreadOverwritesPropertyStrict.types
@@ -77,4 +77,50 @@ function h(obj: { x: number } | { x: string }) {
 >1 : 1
 >obj : { x: number; } | { x: string; }
 }
+function i(b: boolean, t: { command: string, ok: string }) {
+>i : (b: boolean, t: { command: string; ok: string; }) => { command: string; ok: string; } | { command: string; }
+>b : boolean
+>t : { command: string; ok: string; }
+>command : string
+>ok : string
+
+    return { command: "hi", ...(b ? t : {}) } // ok
+>{ command: "hi", ...(b ? t : {}) } : { command: string; ok: string; } | { command: string; }
+>command : string
+>"hi" : "hi"
+>(b ? t : {}) : { command: string; ok: string; } | {}
+>b ? t : {} : { command: string; ok: string; } | {}
+>b : boolean
+>t : { command: string; ok: string; }
+>{} : {}
+}
+function j() {
+>j : () => { command: string; }
+
+    return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
+>{ ...{ command: "hi" } , ...{ command: "bye" } } : { command: string; }
+>{ command: "hi" } : { command: string; }
+>command : string
+>"hi" : "hi"
+>{ command: "bye" } : { command: string; }
+>command : string
+>"bye" : "bye"
+}
+function k(t: { command: string, ok: string }) {
+>k : (t: { command: string; ok: string; }) => { command: string; ok: string; spoiler2: boolean; spoiler: boolean; }
+>t : { command: string; ok: string; }
+>command : string
+>ok : string
+
+    return { command: "hi", ...{ spoiler: true }, spoiler2: true, ...t } // error
+>{ command: "hi", ...{ spoiler: true }, spoiler2: true, ...t } : { command: string; ok: string; spoiler2: boolean; spoiler: boolean; }
+>command : string
+>"hi" : "hi"
+>{ spoiler: true } : { spoiler: boolean; }
+>spoiler : boolean
+>true : true
+>spoiler2 : boolean
+>true : true
+>t : { command: string; ok: string; }
+}
 
diff --git a/tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts b/tests/cases/conformance/types/spread/spreadOverwritesPropertyStrict.ts
@@ -15,3 +15,12 @@ function f(obj: { x: number } | undefined) {
 function h(obj: { x: number } | { x: string }) {
     return { x: 1, ...obj } // error
 }
+function i(b: boolean, t: { command: string, ok: string }) {
+    return { command: "hi", ...(b ? t : {}) } // ok
+}
+function j() {
+    return { ...{ command: "hi" } , ...{ command: "bye" } } // ok
+}
+function k(t: { command: string, ok: string }) {
+    return { command: "hi", ...{ spoiler: true }, spoiler2: true, ...t } // error
+}
