From e8b72aa7d9e76bb5773bc5b51b95277b386edd66 Mon Sep 17 00:00:00 2001 From: Andy Date: Mon, 13 Aug 2018 14:08:00 -0700 Subject: [PATCH] Error on accessing private property through destructuring assignment, and mark property used (#26381) * Error on accessing private property through destructuring assignment, and mark property used * Factor out getTypeOfObjectLiteralDestructuringProperty --- src/compiler/checker.ts | 85 ++++++++++--------- ...destructuringAssignment_private.errors.txt | 17 ++++ .../destructuringAssignment_private.js | 21 +++++ .../destructuringAssignment_private.symbols | 26 ++++++ .../destructuringAssignment_private.types | 44 ++++++++++ ...dLocals_destructuringAssignment.errors.txt | 22 +++++ .../noUnusedLocals_destructuringAssignment.js | 35 ++++++++ ...usedLocals_destructuringAssignment.symbols | 38 +++++++++ ...UnusedLocals_destructuringAssignment.types | 43 ++++++++++ .../destructuringAssignment_private.ts | 7 ++ .../noUnusedLocals_destructuringAssignment.ts | 17 ++++ 11 files changed, 316 insertions(+), 39 deletions(-) create mode 100644 tests/baselines/reference/destructuringAssignment_private.errors.txt create mode 100644 tests/baselines/reference/destructuringAssignment_private.js create mode 100644 tests/baselines/reference/destructuringAssignment_private.symbols create mode 100644 tests/baselines/reference/destructuringAssignment_private.types create mode 100644 tests/baselines/reference/noUnusedLocals_destructuringAssignment.errors.txt create mode 100644 tests/baselines/reference/noUnusedLocals_destructuringAssignment.js create mode 100644 tests/baselines/reference/noUnusedLocals_destructuringAssignment.symbols create mode 100644 tests/baselines/reference/noUnusedLocals_destructuringAssignment.types create mode 100644 tests/cases/compiler/destructuringAssignment_private.ts create mode 100644 tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 2eed799837..f2c1c71eb6 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -17736,17 +17736,15 @@ namespace ts { * Check whether the requested property access is valid. * Returns true if node is a valid property access, and false otherwise. * @param node The node to be checked. - * @param left The left hand side of the property access (e.g.: the super in `super.foo`). - * @param type The type of left. - * @param prop The symbol for the right hand side of the property access. + * @param isSuper True if the access is from `super.`. + * @param type The type of the object whose property is being accessed. (Not the type of the property.) + * @param prop The symbol for the property being accessed. */ - function checkPropertyAccessibility(node: PropertyAccessExpression | QualifiedName | VariableLikeDeclaration | ImportTypeNode, left: Expression | QualifiedName | ImportTypeNode, type: Type, prop: Symbol): boolean { + function checkPropertyAccessibility( + node: PropertyAccessExpression | QualifiedName | PropertyAccessExpression | VariableDeclaration | ParameterDeclaration | ImportTypeNode | PropertyAssignment | ShorthandPropertyAssignment | BindingElement, + isSuper: boolean, type: Type, prop: Symbol): boolean { const flags = getDeclarationModifierFlagsFromSymbol(prop); - const errorNode = node.kind === SyntaxKind.PropertyAccessExpression || node.kind === SyntaxKind.VariableDeclaration ? - node.name : - node.kind === SyntaxKind.ImportType ? - node : - (node).right; + const errorNode = node.kind === SyntaxKind.QualifiedName ? node.right : node.kind === SyntaxKind.ImportType ? node : node.name; if (getCheckFlags(prop) & CheckFlags.ContainsPrivate) { // Synthetic property with private constituent property @@ -17754,7 +17752,7 @@ namespace ts { return false; } - if (left.kind === SyntaxKind.SuperKeyword) { + if (isSuper) { // TS 1.0 spec (April 2014): 4.8.2 // - In a constructor, instance member function, instance member accessor, or // instance member variable initializer where this references a derived class instance, @@ -17807,7 +17805,7 @@ namespace ts { // Property is known to be protected at this point // All protected properties of a supertype are accessible in a super access - if (left.kind === SyntaxKind.SuperKeyword) { + if (isSuper) { return true; } @@ -17940,7 +17938,7 @@ namespace ts { checkPropertyNotUsedBeforeDeclaration(prop, node, right); markPropertyAsReferenced(prop, node, left.kind === SyntaxKind.ThisKeyword); getNodeLinks(node).resolvedSymbol = prop; - checkPropertyAccessibility(node, left, apparentType, prop); + checkPropertyAccessibility(node, left.kind === SyntaxKind.SuperKeyword, apparentType, prop); if (assignmentKind) { if (isReferenceToReadonlyEntity(node, prop) || isReferenceThroughNamespaceImport(node)) { error(right, Diagnostics.Cannot_assign_to_0_because_it_is_a_constant_or_a_read_only_property, idText(right)); @@ -18174,16 +18172,16 @@ namespace ts { function isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName | ImportTypeNode, propertyName: __String): boolean { switch (node.kind) { case SyntaxKind.PropertyAccessExpression: - return isValidPropertyAccessWithType(node, node.expression, propertyName, getWidenedType(checkExpression(node.expression))); + return isValidPropertyAccessWithType(node, node.expression.kind === SyntaxKind.SuperKeyword, propertyName, getWidenedType(checkExpression(node.expression))); case SyntaxKind.QualifiedName: - return isValidPropertyAccessWithType(node, node.left, propertyName, getWidenedType(checkExpression(node.left))); + return isValidPropertyAccessWithType(node, /*isSuper*/ false, propertyName, getWidenedType(checkExpression(node.left))); case SyntaxKind.ImportType: - return isValidPropertyAccessWithType(node, node, propertyName, getTypeFromTypeNode(node)); + return isValidPropertyAccessWithType(node, /*isSuper*/ false, propertyName, getTypeFromTypeNode(node)); } } function isValidPropertyAccessForCompletions(node: PropertyAccessExpression | ImportTypeNode, type: Type, property: Symbol): boolean { - return isValidPropertyAccessWithType(node, node.kind === SyntaxKind.ImportType ? node : node.expression, property.escapedName, type) + return isValidPropertyAccessWithType(node, node.kind !== SyntaxKind.ImportType && node.expression.kind === SyntaxKind.SuperKeyword, property.escapedName, type) && (!(property.flags & SymbolFlags.Method) || isValidMethodAccess(property, type)); } function isValidMethodAccess(method: Symbol, actualThisType: Type): boolean { @@ -18206,7 +18204,7 @@ namespace ts { function isValidPropertyAccessWithType( node: PropertyAccessExpression | QualifiedName | ImportTypeNode, - left: LeftHandSideExpression | QualifiedName | ImportTypeNode, + isSuper: boolean, propertyName: __String, type: Type): boolean { @@ -18214,9 +18212,9 @@ namespace ts { return true; } const prop = getPropertyOfType(type, propertyName); - return prop ? checkPropertyAccessibility(node, left, type, prop) + return prop ? checkPropertyAccessibility(node, isSuper, type, prop) // In js files properties of unions are allowed in completion - : isInJavaScriptFile(node) && (type.flags & TypeFlags.Union) !== 0 && (type).types.some(elementType => isValidPropertyAccessWithType(node, left, propertyName, elementType)); + : isInJavaScriptFile(node) && (type.flags & TypeFlags.Union) !== 0 && (type).types.some(elementType => isValidPropertyAccessWithType(node, isSuper, propertyName, elementType)); } /** @@ -21093,19 +21091,19 @@ namespace ts { return booleanType; } - function checkObjectLiteralAssignment(node: ObjectLiteralExpression, sourceType: Type): Type { + function checkObjectLiteralAssignment(node: ObjectLiteralExpression, sourceType: Type, rightIsThis?: boolean): Type { const properties = node.properties; if (strictNullChecks && properties.length === 0) { return checkNonNullType(sourceType, node); } for (const p of properties) { - checkObjectLiteralDestructuringPropertyAssignment(sourceType, p, properties); + checkObjectLiteralDestructuringPropertyAssignment(sourceType, p, properties, rightIsThis); } return sourceType; } /** Note: If property cannot be a SpreadAssignment, then allProperties does not need to be provided */ - function checkObjectLiteralDestructuringPropertyAssignment(objectLiteralType: Type, property: ObjectLiteralElementLike, allProperties?: NodeArray) { + function checkObjectLiteralDestructuringPropertyAssignment(objectLiteralType: Type, property: ObjectLiteralElementLike, allProperties?: NodeArray, rightIsThis = false) { if (property.kind === SyntaxKind.PropertyAssignment || property.kind === SyntaxKind.ShorthandPropertyAssignment) { const name = property.name; if (name.kind === SyntaxKind.ComputedPropertyName) { @@ -21115,20 +21113,10 @@ namespace ts { return undefined; } - const text = getTextOfPropertyName(name); - const type = isTypeAny(objectLiteralType) - ? objectLiteralType - : getTypeOfPropertyOfType(objectLiteralType, text) || - isNumericLiteralName(text) && getIndexTypeOfType(objectLiteralType, IndexKind.Number) || - getIndexTypeOfType(objectLiteralType, IndexKind.String); + const type = getTypeOfObjectLiteralDestructuringProperty(objectLiteralType, name, property, rightIsThis); if (type) { - if (property.kind === SyntaxKind.ShorthandPropertyAssignment) { - return checkDestructuringAssignment(property, type); - } - else { - // non-shorthand property assignments should always have initializers - return checkDestructuringAssignment(property.initializer, type); - } + // non-shorthand property assignments should always have initializers + return checkDestructuringAssignment(property.kind === SyntaxKind.ShorthandPropertyAssignment ? property : property.initializer, type); } else { error(name, Diagnostics.Type_0_has_no_property_1_and_no_string_index_signature, typeToString(objectLiteralType), declarationNameToString(name)); @@ -21153,6 +21141,25 @@ namespace ts { } } + function getTypeOfObjectLiteralDestructuringProperty(objectLiteralType: Type, name: PropertyName, property: PropertyAssignment | ShorthandPropertyAssignment, rightIsThis: boolean) { + if (isTypeAny(objectLiteralType)) { + return objectLiteralType; + } + + let type: Type | undefined; + const text = getTextOfPropertyName(name); + if (text) { // TODO: GH#26379 + const prop = getPropertyOfType(objectLiteralType, text); + if (prop) { + markPropertyAsReferenced(prop, property, rightIsThis); + checkPropertyAccessibility(property, /*isSuper*/ false, objectLiteralType, prop); + type = getTypeOfSymbol(prop); + } + type = type || (isNumericLiteralName(text) ? getIndexTypeOfType(objectLiteralType, IndexKind.Number) : undefined); + } + return type || getIndexTypeOfType(objectLiteralType, IndexKind.String); + } + function checkArrayLiteralAssignment(node: ArrayLiteralExpression, sourceType: Type, checkMode?: CheckMode): Type { const elements = node.elements; if (languageVersion < ScriptTarget.ES2015 && compilerOptions.downlevelIteration) { @@ -21214,7 +21221,7 @@ namespace ts { return undefined; } - function checkDestructuringAssignment(exprOrAssignment: Expression | ShorthandPropertyAssignment, sourceType: Type, checkMode?: CheckMode): Type { + function checkDestructuringAssignment(exprOrAssignment: Expression | ShorthandPropertyAssignment, sourceType: Type, checkMode?: CheckMode, rightIsThis?: boolean): Type { let target: Expression; if (exprOrAssignment.kind === SyntaxKind.ShorthandPropertyAssignment) { const prop = exprOrAssignment; @@ -21238,7 +21245,7 @@ namespace ts { target = (target).left; } if (target.kind === SyntaxKind.ObjectLiteralExpression) { - return checkObjectLiteralAssignment(target, sourceType); + return checkObjectLiteralAssignment(target, sourceType, rightIsThis); } if (target.kind === SyntaxKind.ArrayLiteralExpression) { return checkArrayLiteralAssignment(target, sourceType, checkMode); @@ -21337,7 +21344,7 @@ namespace ts { function checkBinaryLikeExpression(left: Expression, operatorToken: Node, right: Expression, checkMode?: CheckMode, errorNode?: Node): Type { const operator = operatorToken.kind; if (operator === SyntaxKind.EqualsToken && (left.kind === SyntaxKind.ObjectLiteralExpression || left.kind === SyntaxKind.ArrayLiteralExpression)) { - return checkDestructuringAssignment(left, checkExpression(right, checkMode), checkMode); + return checkDestructuringAssignment(left, checkExpression(right, checkMode), checkMode, right.kind === SyntaxKind.ThisKeyword); } let leftType: Type; if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken) { @@ -24400,7 +24407,7 @@ namespace ts { const property = getPropertyOfType(parentType!, nameText)!; // TODO: GH#18217 markPropertyAsReferenced(property, /*nodeForCheckWriteOnly*/ undefined, /*isThisAccess*/ false); // A destructuring is never a write-only reference. if (parent.initializer && property && !isComputedPropertyName(name)) { - checkPropertyAccessibility(parent, parent.initializer, parentType!, property); + checkPropertyAccessibility(parent, parent.initializer.kind === SyntaxKind.SuperKeyword, parentType!, property); } } } diff --git a/tests/baselines/reference/destructuringAssignment_private.errors.txt b/tests/baselines/reference/destructuringAssignment_private.errors.txt new file mode 100644 index 0000000000..f17f1610d0 --- /dev/null +++ b/tests/baselines/reference/destructuringAssignment_private.errors.txt @@ -0,0 +1,17 @@ +tests/cases/compiler/destructuringAssignment_private.ts(6,10): error TS2341: Property 'x' is private and only accessible within class 'C'. +tests/cases/compiler/destructuringAssignment_private.ts(7,4): error TS2341: Property 'o' is private and only accessible within class 'C'. + + +==== tests/cases/compiler/destructuringAssignment_private.ts (2 errors) ==== + class C { + private x = 0; + private o = [{ a: 1 }]; + } + let x: number; + ([{ a: { x } }] = [{ a: new C() }]); + ~ +!!! error TS2341: Property 'x' is private and only accessible within class 'C'. + ({ o: [{ a: x }]} = new C()); + ~ +!!! error TS2341: Property 'o' is private and only accessible within class 'C'. + \ No newline at end of file diff --git a/tests/baselines/reference/destructuringAssignment_private.js b/tests/baselines/reference/destructuringAssignment_private.js new file mode 100644 index 0000000000..6201ad3578 --- /dev/null +++ b/tests/baselines/reference/destructuringAssignment_private.js @@ -0,0 +1,21 @@ +//// [destructuringAssignment_private.ts] +class C { + private x = 0; + private o = [{ a: 1 }]; +} +let x: number; +([{ a: { x } }] = [{ a: new C() }]); +({ o: [{ a: x }]} = new C()); + + +//// [destructuringAssignment_private.js] +var C = /** @class */ (function () { + function C() { + this.x = 0; + this.o = [{ a: 1 }]; + } + return C; +}()); +var x; +(x = [{ a: new C() }][0].a.x); +(x = new C().o[0].a); diff --git a/tests/baselines/reference/destructuringAssignment_private.symbols b/tests/baselines/reference/destructuringAssignment_private.symbols new file mode 100644 index 0000000000..b3a7abce5b --- /dev/null +++ b/tests/baselines/reference/destructuringAssignment_private.symbols @@ -0,0 +1,26 @@ +=== tests/cases/compiler/destructuringAssignment_private.ts === +class C { +>C : Symbol(C, Decl(destructuringAssignment_private.ts, 0, 0)) + + private x = 0; +>x : Symbol(C.x, Decl(destructuringAssignment_private.ts, 0, 9)) + + private o = [{ a: 1 }]; +>o : Symbol(C.o, Decl(destructuringAssignment_private.ts, 1, 18)) +>a : Symbol(a, Decl(destructuringAssignment_private.ts, 2, 18)) +} +let x: number; +>x : Symbol(x, Decl(destructuringAssignment_private.ts, 4, 3)) + +([{ a: { x } }] = [{ a: new C() }]); +>a : Symbol(a, Decl(destructuringAssignment_private.ts, 5, 3)) +>x : Symbol(x, Decl(destructuringAssignment_private.ts, 5, 8)) +>a : Symbol(a, Decl(destructuringAssignment_private.ts, 5, 20)) +>C : Symbol(C, Decl(destructuringAssignment_private.ts, 0, 0)) + +({ o: [{ a: x }]} = new C()); +>o : Symbol(o, Decl(destructuringAssignment_private.ts, 6, 2)) +>a : Symbol(a, Decl(destructuringAssignment_private.ts, 6, 8)) +>x : Symbol(x, Decl(destructuringAssignment_private.ts, 4, 3)) +>C : Symbol(C, Decl(destructuringAssignment_private.ts, 0, 0)) + diff --git a/tests/baselines/reference/destructuringAssignment_private.types b/tests/baselines/reference/destructuringAssignment_private.types new file mode 100644 index 0000000000..f126a973f1 --- /dev/null +++ b/tests/baselines/reference/destructuringAssignment_private.types @@ -0,0 +1,44 @@ +=== tests/cases/compiler/destructuringAssignment_private.ts === +class C { +>C : C + + private x = 0; +>x : number +>0 : 0 + + private o = [{ a: 1 }]; +>o : { a: number; }[] +>[{ a: 1 }] : { a: number; }[] +>{ a: 1 } : { a: number; } +>a : number +>1 : 1 +} +let x: number; +>x : number + +([{ a: { x } }] = [{ a: new C() }]); +>([{ a: { x } }] = [{ a: new C() }]) : [{ a: C; }] +>[{ a: { x } }] = [{ a: new C() }] : [{ a: C; }] +>[{ a: { x } }] : [{ a: { x: number; }; }] +>{ a: { x } } : { a: { x: number; }; } +>a : { x: number; } +>{ x } : { x: number; } +>x : number +>[{ a: new C() }] : [{ a: C; }] +>{ a: new C() } : { a: C; } +>a : C +>new C() : C +>C : typeof C + +({ o: [{ a: x }]} = new C()); +>({ o: [{ a: x }]} = new C()) : C +>{ o: [{ a: x }]} = new C() : C +>{ o: [{ a: x }]} : { o: [{ a: number; }]; } +>o : [{ a: number; }] +>[{ a: x }] : [{ a: number; }] +>{ a: x } : { a: number; } +>a : number +>x : number +>new C() : C +>C : typeof C + diff --git a/tests/baselines/reference/noUnusedLocals_destructuringAssignment.errors.txt b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.errors.txt new file mode 100644 index 0000000000..634a36ad03 --- /dev/null +++ b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.errors.txt @@ -0,0 +1,22 @@ +tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts(10,13): error TS6133: 'f' is declared but its value is never read. + + +==== tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts (1 errors) ==== + class C { + private x = 0; + + m(): number { + let x: number; + ({ x } = this); + return x; + } + + private f(): Function { + ~ +!!! error TS6133: 'f' is declared but its value is never read. + let f: Function; + ({ f } = this); + return f; + } + } + \ No newline at end of file diff --git a/tests/baselines/reference/noUnusedLocals_destructuringAssignment.js b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.js new file mode 100644 index 0000000000..805957e44d --- /dev/null +++ b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.js @@ -0,0 +1,35 @@ +//// [noUnusedLocals_destructuringAssignment.ts] +class C { + private x = 0; + + m(): number { + let x: number; + ({ x } = this); + return x; + } + + private f(): Function { + let f: Function; + ({ f } = this); + return f; + } +} + + +//// [noUnusedLocals_destructuringAssignment.js] +var C = /** @class */ (function () { + function C() { + this.x = 0; + } + C.prototype.m = function () { + var x; + (x = this.x); + return x; + }; + C.prototype.f = function () { + var f; + (f = this.f); + return f; + }; + return C; +}()); diff --git a/tests/baselines/reference/noUnusedLocals_destructuringAssignment.symbols b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.symbols new file mode 100644 index 0000000000..11c6f4964d --- /dev/null +++ b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.symbols @@ -0,0 +1,38 @@ +=== tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts === +class C { +>C : Symbol(C, Decl(noUnusedLocals_destructuringAssignment.ts, 0, 0)) + + private x = 0; +>x : Symbol(C.x, Decl(noUnusedLocals_destructuringAssignment.ts, 0, 9)) + + m(): number { +>m : Symbol(C.m, Decl(noUnusedLocals_destructuringAssignment.ts, 1, 18)) + + let x: number; +>x : Symbol(x, Decl(noUnusedLocals_destructuringAssignment.ts, 4, 11)) + + ({ x } = this); +>x : Symbol(x, Decl(noUnusedLocals_destructuringAssignment.ts, 5, 10)) +>this : Symbol(C, Decl(noUnusedLocals_destructuringAssignment.ts, 0, 0)) + + return x; +>x : Symbol(x, Decl(noUnusedLocals_destructuringAssignment.ts, 4, 11)) + } + + private f(): Function { +>f : Symbol(C.f, Decl(noUnusedLocals_destructuringAssignment.ts, 7, 5)) +>Function : Symbol(Function, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --)) + + let f: Function; +>f : Symbol(f, Decl(noUnusedLocals_destructuringAssignment.ts, 10, 11)) +>Function : Symbol(Function, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --)) + + ({ f } = this); +>f : Symbol(f, Decl(noUnusedLocals_destructuringAssignment.ts, 11, 10)) +>this : Symbol(C, Decl(noUnusedLocals_destructuringAssignment.ts, 0, 0)) + + return f; +>f : Symbol(f, Decl(noUnusedLocals_destructuringAssignment.ts, 10, 11)) + } +} + diff --git a/tests/baselines/reference/noUnusedLocals_destructuringAssignment.types b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.types new file mode 100644 index 0000000000..7c5d54c816 --- /dev/null +++ b/tests/baselines/reference/noUnusedLocals_destructuringAssignment.types @@ -0,0 +1,43 @@ +=== tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts === +class C { +>C : C + + private x = 0; +>x : number +>0 : 0 + + m(): number { +>m : () => number + + let x: number; +>x : number + + ({ x } = this); +>({ x } = this) : this +>{ x } = this : this +>{ x } : { x: number; } +>x : number +>this : this + + return x; +>x : number + } + + private f(): Function { +>f : () => Function + + let f: Function; +>f : Function + + ({ f } = this); +>({ f } = this) : this +>{ f } = this : this +>{ f } : { f: Function; } +>f : Function +>this : this + + return f; +>f : Function + } +} + diff --git a/tests/cases/compiler/destructuringAssignment_private.ts b/tests/cases/compiler/destructuringAssignment_private.ts new file mode 100644 index 0000000000..bdc057851a --- /dev/null +++ b/tests/cases/compiler/destructuringAssignment_private.ts @@ -0,0 +1,7 @@ +class C { + private x = 0; + private o = [{ a: 1 }]; +} +let x: number; +([{ a: { x } }] = [{ a: new C() }]); +({ o: [{ a: x }]} = new C()); diff --git a/tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts b/tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts new file mode 100644 index 0000000000..376dc930d2 --- /dev/null +++ b/tests/cases/compiler/noUnusedLocals_destructuringAssignment.ts @@ -0,0 +1,17 @@ +// @noUnusedLocals: true + +class C { + private x = 0; + + m(): number { + let x: number; + ({ x } = this); + return x; + } + + private f(): Function { + let f: Function; + ({ f } = this); + return f; + } +}