Skip to content

Improved checking of destructuring with literal initializers #4598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Sep 11, 2015
Merged

Improved checking of destructuring with literal initializers #4598

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e78478b
More flexible rules when destructuring have literal initializers
ahejlsberg Sep 1, 2015
c436736
Adding a few comments
ahejlsberg Sep 1, 2015
f28e424
Adding tests
ahejlsberg Sep 1, 2015
66e3aba
Accepting new baselines
ahejlsberg Sep 1, 2015
dc8ad6e
Store binding pattern in contextual type
ahejlsberg Sep 5, 2015
e40b86f
Accepting new baselines
ahejlsberg Sep 5, 2015
bb81797
Check for excess properties
ahejlsberg Sep 5, 2015
a0ddd43
Modifying tests
ahejlsberg Sep 5, 2015
f801420
Accepting new baselines
ahejlsberg Sep 5, 2015
31f8a81
Adding test for excess/missing properties
ahejlsberg Sep 5, 2015
546da60
Correct propagation of includePatternInType flag
ahejlsberg Sep 6, 2015
27380f4
Adding test
ahejlsberg Sep 6, 2015
055363c
Accepting new baselines
ahejlsberg Sep 6, 2015
d5ff9ee
Error when binding element has no value in initializer and no default
ahejlsberg Sep 10, 2015
a95c423
Revising test
ahejlsberg Sep 10, 2015
273b9ff
Accepting new baselines
ahejlsberg Sep 10, 2015
262f122
Adding "." to error message
ahejlsberg Sep 11, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 47 additions & 10 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2318,8 +2318,8 @@ namespace ts {
// Use type of the specified property, or otherwise, for a numeric name, the type of the numeric index signature,
// or otherwise the type of the string index signature.
type = getTypeOfPropertyOfType(parentType, name.text) ||
isNumericLiteralName(name.text) && getIndexTypeOfType(parentType, IndexKind.Number) ||
getIndexTypeOfType(parentType, IndexKind.String);
isNumericLiteralName(name.text) && getIndexTypeOfType(parentType, IndexKind.Number) ||
getIndexTypeOfType(parentType, IndexKind.String);
if (!type) {
error(name, Diagnostics.Type_0_has_no_property_1_and_no_string_index_signature, typeToString(parentType), declarationNameToString(name));
return unknownType;
Expand Down Expand Up @@ -2456,7 +2456,6 @@ namespace ts {
let unionOfElements = getUnionType(elementTypes);
return languageVersion >= ScriptTarget.ES6 ? createIterableType(unionOfElements) : createArrayType(unionOfElements);
}

// If the pattern has at least one element, and no rest element, then it should imply a tuple type.
return createTupleType(elementTypes);
}
Expand Down Expand Up @@ -6607,12 +6606,18 @@ namespace ts {
}
}
if (isBindingPattern(declaration.name)) {
return getTypeFromBindingPattern(<BindingPattern>declaration.name);
return createImpliedType(getTypeFromBindingPattern(<BindingPattern>declaration.name));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this step applies both for top level initializers as well as initializers nested in binding patterns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it applies generally. The level of nesting shouldn't matter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any tests that exercise it in the nested case?

}
}
return undefined;
}

function createImpliedType(type: Type): Type {
var result = clone(type);
result.flags |= TypeFlags.ImpliedType;
return result;
}

function getContextualTypeForReturnExpression(node: Expression): Type {
let func = getContainingFunction(node);
if (func && !func.asteriskToken) {
Expand Down Expand Up @@ -7005,9 +7010,6 @@ namespace ts {

function checkArrayLiteral(node: ArrayLiteralExpression, contextualMapper?: TypeMapper): Type {
let elements = node.elements;
if (!elements.length) {
return createArrayType(undefinedType);
}
let hasSpreadElement = false;
let elementTypes: Type[] = [];
let inDestructuringPattern = isAssignmentTarget(node);
Expand Down Expand Up @@ -7039,12 +7041,28 @@ namespace ts {
hasSpreadElement = hasSpreadElement || e.kind === SyntaxKind.SpreadElementExpression;
}
if (!hasSpreadElement) {
// If array literal is actually a destructuring pattern, mark it as an implied type. We do this such
// that we get the same behavior for "var [x, y] = []" and "[x, y] = []".
if (inDestructuringPattern && elementTypes.length) {
return createImpliedType(createTupleType(elementTypes));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you have to do this for object literals too (the ones that happen to be assignment patterns)? Also, I don't think there are any tests for assignment patterns, just binding patterns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, actually maybe you did not mean to do that. Here you are creating an implied type for an assignment pattern. But I think only binding patterns can imply types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to allow var { x, y } = {} and the corresponding assignment { x, y } = {} then we'd have to do it for object literals as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of think that for parity with arrays, it's nice to do it for object literals too. Though I realize that the main rationale for arrays is to make the right hand side a tuple.

}
let contextualType = getContextualType(node);
if (contextualType && contextualTypeIsTupleLikeType(contextualType) || inDestructuringPattern) {
return createTupleType(elementTypes);
let contextualTupleLikeType = contextualType && contextualTypeIsTupleLikeType(contextualType) ? contextualType : undefined;
if (contextualTupleLikeType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's simpler to just do

if (contextualType && contextualTypeIsTupleLikeType(contextualType)) {
   ...
}

And just refer to contextualType inside. Essentially just inline the condition

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

// If array literal is contextually typed by the implied type of a binding pattern, pad the resulting
// tuple type with elements from the binding tuple type to make the lengths equal.
if (contextualTupleLikeType.flags & TypeFlags.Tuple && contextualTupleLikeType.flags & TypeFlags.ImpliedType) {
let contextualElementTypes = (<TupleType>contextualTupleLikeType).elementTypes;
for (let i = elementTypes.length; i < contextualElementTypes.length; i++) {
elementTypes.push(contextualElementTypes[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, maybe I'm missing something. But it seems like only initialized properties should be copied, otherwise you allow default initializers that are too short to satisfy the destructuring.

}
}
if (elementTypes.length) {
return createTupleType(elementTypes);
}
}
}
return createArrayType(getUnionType(elementTypes));
return createArrayType(elementTypes.length ? getUnionType(elementTypes) : undefinedType)
}

function isNumericName(name: DeclarationName): boolean {
Expand Down Expand Up @@ -7131,6 +7149,14 @@ namespace ts {
}
typeFlags |= type.flags;
let prop = <TransientSymbol>createSymbol(SymbolFlags.Property | SymbolFlags.Transient | member.flags, member.name);
// If object literal is contextually typed by the implied type of a binding pattern, and if the
// binding pattern specifies a default value for the property, make the property optional.
if (contextualType && contextualType.flags & TypeFlags.ImpliedType) {
let impliedProp = getPropertyOfType(contextualType, member.name);
if (impliedProp) {
prop.flags |= impliedProp.flags & SymbolFlags.Optional;
}
}
prop.declarations = member.declarations;
prop.parent = member.parent;
if (member.valueDeclaration) {
Expand All @@ -7157,6 +7183,17 @@ namespace ts {
propertiesArray.push(member);
}

// If object literal is contextually typed by the implied type of a binding pattern, augment the result
// type with those properties for which the binding pattern specifies a default value.
if (contextualType && contextualType.flags & TypeFlags.ImpliedType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add tests that demonstrate that this can cause an error. Something like this, which I don't believe would have errored before:

function foo({ a = 0 } = {}) { }
foo({ a: "" });

Basically something where the property was dropped in the initializer, but then was present again in the argument with the wrong type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did error before (complaining that the object literal is missing property a), but it shouldn't have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it does error now because of the string, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does error now.

for (let prop of getPropertiesOfType(contextualType)) {
if (prop.flags & SymbolFlags.Optional && !hasProperty(propertiesTable, prop.name)) {
propertiesTable[prop.name] = prop;
propertiesArray.push(prop);
}
}
}

let stringIndexType = getIndexType(IndexKind.String);
let numberIndexType = getIndexType(IndexKind.Number);
let result = createAnonymousType(node.symbol, propertiesTable, emptyArray, emptyArray, stringIndexType, numberIndexType);
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1795,6 +1795,7 @@ namespace ts {
/* @internal */
ContainsAnyFunctionType = 0x00800000, // Type is or contains object literal type
ESSymbol = 0x01000000, // Type of symbol primitive introduced in ES6
ImpliedType = 0x02000000, // Type implied by object binding pattern

/* @internal */
Intrinsic = Any | String | Number | Boolean | ESSymbol | Void | Undefined | Null,
Expand Down Expand Up @@ -1864,8 +1865,7 @@ namespace ts {
}

export interface TupleType extends ObjectType {
elementTypes: Type[]; // Element types
baseArrayType: TypeReference; // Array<T> where T is best common type of element types
elementTypes: Type[]; // Element types
}

export interface UnionOrIntersectionType extends Type {
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/arrowFunctionExpressions.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,12 @@ var p8 = ({ a = 1 }) => { };
>1 : number

var p9 = ({ a: { b = 1 } = { b: 1 } }) => { };
>p9 : ({ a: { b = 1 } = { b: 1 } }: { a?: { b: number; }; }) => void
>({ a: { b = 1 } = { b: 1 } }) => { } : ({ a: { b = 1 } = { b: 1 } }: { a?: { b: number; }; }) => void
>p9 : ({ a: { b = 1 } = { b: 1 } }: { a?: { b?: number; }; }) => void
>({ a: { b = 1 } = { b: 1 } }) => { } : ({ a: { b = 1 } = { b: 1 } }: { a?: { b?: number; }; }) => void
>a : any
>b : number
>1 : number
>{ b: 1 } : { b: number; }
>{ b: 1 } : { b?: number; }
>b : number
>1 : number

Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/declarationEmitDestructuring2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ function h1(_a) {

//// [declarationEmitDestructuring2.d.ts]
declare function f({x, y: [a, b, c, d]}?: {
x: number;
y: [number, number, number, number];
x?: number;
y?: [number, number, number, number];
}): void;
declare function g([a, b, c, d]?: [number, number, number, number]): void;
declare function h([a, [b], [[c]], {x, y: [a, b, c], z: {a1, b1}}]: [any, [any], [[any]], {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/declarationEmitDestructuringArrayPattern2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ var [a11, b11, c11] = [];
>a11 : any
>b11 : any
>c11 : any
>[] : undefined[]
>[] : [any, any, any]

var [a2, [b2, { x12, y12: c2 }]=["abc", { x12: 10, y12: false }]] = [1, ["hello", { x12: 5, y12: true }]];
>a2 : number
Expand Down
14 changes: 4 additions & 10 deletions tests/baselines/reference/declarationsAndAssignments.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(5,16): error TS2493: Tuple type '[number, string]' with length '2' cannot be assigned to tuple with length '3'.
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(6,13): error TS2403: Subsequent variable declarations must have the same type. Variable 'z' must be of type 'any', but here has type 'number'.
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(56,17): error TS2322: Type 'number' is not assignable to type 'string'.
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(63,13): error TS2493: Tuple type '[number]' with length '1' cannot be assigned to tuple with length '3'.
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(63,16): error TS2493: Tuple type '[number]' with length '1' cannot be assigned to tuple with length '3'.
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(67,9): error TS2461: Type '{ [x: number]: undefined; }' is not an array type.
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(68,9): error TS2461: Type '{ [x: number]: number; 0: number; 1: number; }' is not an array type.
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(73,11): error TS2459: Type '{}' has no property 'a' and no string index signature.
Expand All @@ -18,15 +16,15 @@ tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(138,6):
tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(138,9): error TS2322: Type 'number' is not assignable to type 'string'.


==== tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts (13 errors) ====
==== tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts (11 errors) ====
function f0() {
var [] = [1, "hello"];
var [x] = [1, "hello"];
var [x, y] = [1, "hello"];
var [x, y, z] = [1, "hello"]; // Error
~
!!! error TS2493: Tuple type '[number, string]' with length '2' cannot be assigned to tuple with length '3'.
var [,, z] = [0, 1, 2];
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'z' must be of type 'any', but here has type 'number'.
var x: number;
var y: string;
}
Expand Down Expand Up @@ -86,10 +84,6 @@ tests/cases/conformance/es6/destructuring/declarationsAndAssignments.ts(138,9):
function f8() {
var [a, b, c] = []; // Ok, [] is an array
var [d, e, f] = [1]; // Error, [1] is a tuple
~
!!! error TS2493: Tuple type '[number]' with length '1' cannot be assigned to tuple with length '3'.
~
!!! error TS2493: Tuple type '[number]' with length '1' cannot be assigned to tuple with length '3'.
}

function f9() {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/destructuringArrayBindingPatternAndAssignment1ES5.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ var [c0, c1] = [...temp];

var [c2] = [];
>c2 : any
>[] : undefined[]
>[] : [any]

var [[[c3]], [[[[c4]]]]] = [[[]], [[[[]]]]]
>c3 : any
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/destructuringArrayBindingPatternAndAssignment1ES6.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ var [c0, c1] = [...temp];

var [c2] = [];
>c2 : any
>[] : undefined[]
>[] : [any]

var [[[c3]], [[[[c4]]]]] = [[[]], [[[[]]]]]
>c3 : any
Expand Down
8 changes: 1 addition & 7 deletions tests/baselines/reference/destructuringArrayBindingPatternAndAssignment2.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment2.ts(3,6): error TS2461: Type 'undefined' is not an array type.
tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment2.ts(3,12): error TS2461: Type 'undefined' is not an array type.
tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment2.ts(4,5): error TS2461: Type 'undefined' is not an array type.
tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment2.ts(9,5): error TS2322: Type '[number, number, string]' is not assignable to type '[number, boolean, string]'.
Types of property '1' are incompatible.
Expand All @@ -13,14 +11,10 @@ tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAss
tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment2.ts(34,5): error TS2461: Type 'F' is not an array type.


==== tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment2.ts (8 errors) ====
==== tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment2.ts (6 errors) ====
// V is an array assignment pattern, S is the type Any or an array-like type (section 3.3.2), and, for each assignment element E in V,
// S is the type Any, or
var [[a0], [[a1]]] = [] // Error
~~~~
!!! error TS2461: Type 'undefined' is not an array type.
~~~~~~
!!! error TS2461: Type 'undefined' is not an array type.
var [[a2], [[a3]]] = undefined // Error
~~~~~~~~~~~~~~
!!! error TS2461: Type 'undefined' is not an array type.
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/destructuringObjectBindingPatternAndAssignment1ES5.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var { b2: { b21 } = { b21: "string" } } = { b2: { b21: "world" } };
>{ b21: "string" } : { b21: string; }
>b21 : string
>"string" : string
>{ b2: { b21: "world" } } : { b2: { b21: string; }; }
>{ b2: { b21: "world" } } : { b2?: { b21: string; }; }
>b2 : { b21: string; }
>{ b21: "world" } : { b21: string; }
>b21 : string
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/destructuringObjectBindingPatternAndAssignment1ES6.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var { b2: { b21 } = { b21: "string" } } = { b2: { b21: "world" } };
>{ b21: "string" } : { b21: string; }
>b21 : string
>"string" : string
>{ b2: { b21: "world" } } : { b2: { b21: string; }; }
>{ b2: { b21: "world" } } : { b2?: { b21: string; }; }
>b2 : { b21: string; }
>{ b21: "world" } : { b21: string; }
>b21 : string
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/destructuringVariableDeclaration1ES5.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ var { b1: { b11 } = { b11: "string" } } = { b1: { b11: "world" } };
>{ b11: "string" } : { b11: string; }
>b11 : string
>"string" : string
>{ b1: { b11: "world" } } : { b1: { b11: string; }; }
>{ b1: { b11: "world" } } : { b1?: { b11: string; }; }
>b1 : { b11: string; }
>{ b11: "world" } : { b11: string; }
>b11 : string
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/destructuringVariableDeclaration1ES6.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ var { b1: { b11 } = { b11: "string" } } = { b1: { b11: "world" } };
>{ b11: "string" } : { b11: string; }
>b11 : string
>"string" : string
>{ b1: { b11: "world" } } : { b1: { b11: string; }; }
>{ b1: { b11: "world" } } : { b1?: { b11: string; }; }
>b1 : { b11: string; }
>{ b11: "world" } : { b11: string; }
>b11 : string
Expand Down
Loading