From 5515dcea34d1ff584467ad842ffb4c979f781a7f Mon Sep 17 00:00:00 2001 From: Anders Hejlsberg Date: Sun, 30 Oct 2016 16:01:15 -0700 Subject: [PATCH] Improved error messages for invalid assignments to identifiers --- src/compiler/binder.ts | 4 +- src/compiler/checker.ts | 81 ++++++++++++++++------------ src/compiler/diagnosticMessages.json | 8 +++ src/compiler/types.ts | 4 -- src/compiler/utilities.ts | 56 ++++++++++++------- 5 files changed, 94 insertions(+), 59 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index c05d0135f14..801bd2c35ba 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -1192,9 +1192,9 @@ namespace ts { } else { forEachChild(node, bind); - if (operator === SyntaxKind.EqualsToken && !isAssignmentTarget(node)) { + if (isAssignmentOperator(operator) && !isAssignmentTarget(node)) { bindAssignmentTargetFlow(node.left); - if (node.left.kind === SyntaxKind.ElementAccessExpression) { + if (operator === SyntaxKind.EqualsToken && node.left.kind === SyntaxKind.ElementAccessExpression) { const elementAccess = node.left; if (isNarrowableOperand(elementAccess.expression)) { currentFlow = createFlowArrayMutation(currentFlow, node); diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index a556814aedd..5e23f56b947 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -8851,7 +8851,7 @@ namespace ts { // Assignments only narrow the computed type if the declared type is a union type. Thus, we // only need to evaluate the assigned type if the declared type is a union type. if (isMatchingReference(reference, node)) { - if (node.parent.kind === SyntaxKind.PrefixUnaryExpression || node.parent.kind === SyntaxKind.PostfixUnaryExpression) { + if (getAssignmentTargetKind(node) === AssignmentKind.Compound) { const flowType = getTypeAtFlowNode(flow.antecedent); return createFlowType(getBaseTypeOfLiteralType(getTypeFromFlowType(flowType)), isIncomplete(flowType)); } @@ -9298,10 +9298,10 @@ namespace ts { } } else { - const invokedExpression = skipParenthesizedNodes(callExpression.expression); + const invokedExpression = skipParentheses(callExpression.expression); if (invokedExpression.kind === SyntaxKind.ElementAccessExpression || invokedExpression.kind === SyntaxKind.PropertyAccessExpression) { const accessExpression = invokedExpression as ElementAccessExpression | PropertyAccessExpression; - const possibleReference = skipParenthesizedNodes(accessExpression.expression); + const possibleReference = skipParentheses(accessExpression.expression); if (isMatchingReference(reference, possibleReference)) { return getNarrowedType(type, predicate.type, assumeTrue); } @@ -9361,13 +9361,6 @@ namespace ts { return getTypeOfSymbol(symbol); } - function skipParenthesizedNodes(expression: Expression): Expression { - while (expression.kind === SyntaxKind.ParenthesizedExpression) { - expression = (expression as ParenthesizedExpression).expression; - } - return expression; - } - function getControlFlowContainer(node: Node): Node { while (true) { node = node.parent; @@ -9425,6 +9418,9 @@ namespace ts { function checkIdentifier(node: Identifier): Type { const symbol = getResolvedSymbol(node); + if (symbol === unknownSymbol) { + return unknownType; + } // As noted in ECMAScript 6 language spec, arrow functions never have an arguments objects. // Although in down-level emit of arrow function, we emit it using function expression which means that @@ -9446,6 +9442,7 @@ namespace ts { if (node.flags & NodeFlags.AwaitContext) { getNodeLinks(container).flags |= NodeCheckFlags.CaptureArguments; } + return getTypeOfSymbol(symbol); } if (symbol.flags & SymbolFlags.Alias && !isInTypeQuery(node) && !isConstEnumOrConstEnumOnlyModule(resolveAlias(symbol))) { @@ -9498,9 +9495,22 @@ namespace ts { const type = getTypeOfSymbol(localOrExportSymbol); const declaration = localOrExportSymbol.valueDeclaration; + const assignmentKind = getAssignmentTargetKind(node); + + if (assignmentKind) { + if (!(localOrExportSymbol.flags & SymbolFlags.Variable)) { + error(node, Diagnostics.Cannot_assign_to_0_because_it_is_not_a_variable, symbolToString(symbol)); + return unknownType; + } + if (isReadonlySymbol(localOrExportSymbol)) { + error(node, Diagnostics.Cannot_assign_to_0_because_it_is_a_constant_or_a_read_only_property, symbolToString(symbol)); + return unknownType; + } + } + // We only narrow variables and parameters occurring in a non-assignment position. For all other // entities we simply return the declared type. - if (!(localOrExportSymbol.flags & SymbolFlags.Variable) || isAssignmentTarget(node) || !declaration) { + if (!(localOrExportSymbol.flags & SymbolFlags.Variable) || assignmentKind === AssignmentKind.Definite || !declaration) { return type; } // The declaration container is the innermost function that encloses the declaration of the variable @@ -9542,7 +9552,7 @@ namespace ts { // Return the declared type to reduce follow-on errors return type; } - return flowType; + return assignmentKind ? getBaseTypeOfLiteralType(flowType) : flowType; } function isInsideFunction(node: Node, threshold: Node): boolean { @@ -11397,6 +11407,20 @@ namespace ts { return checkPropertyAccessExpressionOrQualifiedName(node, node.left, node.right); } + function reportNonexistentProperty(propNode: Identifier, containingType: Type) { + let errorInfo: DiagnosticMessageChain; + if (containingType.flags & TypeFlags.Union && !(containingType.flags & TypeFlags.Primitive)) { + for (const subtype of (containingType as UnionType).types) { + if (!getPropertyOfType(subtype, propNode.text)) { + errorInfo = chainDiagnosticMessages(errorInfo, Diagnostics.Property_0_does_not_exist_on_type_1, declarationNameToString(propNode), typeToString(subtype)); + break; + } + } + } + errorInfo = chainDiagnosticMessages(errorInfo, Diagnostics.Property_0_does_not_exist_on_type_1, declarationNameToString(propNode), typeToString(containingType)); + diagnostics.add(createDiagnosticForNodeFromMessageChain(propNode, errorInfo)); + } + function checkPropertyAccessExpressionOrQualifiedName(node: PropertyAccessExpression | QualifiedName, left: Expression | QualifiedName, right: Identifier) { const type = checkNonNullExpression(left); if (isTypeAny(type) || type === silentNeverType) { @@ -11445,20 +11469,6 @@ namespace ts { return propType; } return getFlowTypeOfReference(node, propType, /*assumeInitialized*/ true, /*flowContainer*/ undefined); - - function reportNonexistentProperty(propNode: Identifier, containingType: Type) { - let errorInfo: DiagnosticMessageChain; - if (containingType.flags & TypeFlags.Union && !(containingType.flags & TypeFlags.Primitive)) { - for (const subtype of (containingType as UnionType).types) { - if (!getPropertyOfType(subtype, propNode.text)) { - errorInfo = chainDiagnosticMessages(errorInfo, Diagnostics.Property_0_does_not_exist_on_type_1, declarationNameToString(propNode), typeToString(subtype)); - break; - } - } - } - errorInfo = chainDiagnosticMessages(errorInfo, Diagnostics.Property_0_does_not_exist_on_type_1, declarationNameToString(propNode), typeToString(containingType)); - diagnostics.add(createDiagnosticForNodeFromMessageChain(propNode, errorInfo)); - } } function isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName, propertyName: string): boolean { @@ -11505,7 +11515,7 @@ namespace ts { * that references a for-in variable for an object with numeric property names. */ function isForInVariableForNumericPropertyNames(expr: Expression) { - const e = skipParenthesizedNodes(expr); + const e = skipParentheses(expr); if (e.kind === SyntaxKind.Identifier) { const symbol = getResolvedSymbol(e); if (symbol.flags & SymbolFlags.Variable) { @@ -13486,7 +13496,7 @@ namespace ts { function isReferenceThroughNamespaceImport(expr: Expression): boolean { if (expr.kind === SyntaxKind.PropertyAccessExpression || expr.kind === SyntaxKind.ElementAccessExpression) { - const node = skipParenthesizedNodes((expr as PropertyAccessExpression | ElementAccessExpression).expression); + const node = skipParentheses((expr as PropertyAccessExpression | ElementAccessExpression).expression); if (node.kind === SyntaxKind.Identifier) { const symbol = getNodeLinks(node).resolvedSymbol; if (symbol.flags & SymbolFlags.Alias) { @@ -13500,11 +13510,14 @@ namespace ts { function checkReferenceExpression(expr: Expression, invalidReferenceMessage: DiagnosticMessage, constantVariableMessage: DiagnosticMessage): boolean { // References are combinations of identifiers, parentheses, and property accesses. - const node = skipParenthesizedNodes(expr); + const node = skipParentheses(expr); if (node.kind !== SyntaxKind.Identifier && node.kind !== SyntaxKind.PropertyAccessExpression && node.kind !== SyntaxKind.ElementAccessExpression) { error(expr, invalidReferenceMessage); return false; } + if (node.kind === SyntaxKind.Identifier) { + return true; + } // Because we get the symbol from the resolvedSymbol property, it might be of kind // SymbolFlags.ExportValue. In this case it is necessary to get the actual export // symbol, which will have the correct flags set on it. @@ -13514,10 +13527,10 @@ namespace ts { if (symbol !== unknownSymbol && symbol !== argumentsSymbol) { // Only variables (and not functions, classes, namespaces, enum objects, or enum members) // are considered references when referenced using a simple identifier. - if (node.kind === SyntaxKind.Identifier && !(symbol.flags & SymbolFlags.Variable)) { - error(expr, invalidReferenceMessage); - return false; - } + // if (node.kind === SyntaxKind.Identifier && !(symbol.flags & SymbolFlags.Variable)) { + // error(expr, invalidReferenceMessage); + // return false; + // } if (isReferenceToReadonlyEntity(node, symbol) || isReferenceThroughNamespaceImport(node)) { error(expr, constantVariableMessage); return false; @@ -14255,7 +14268,7 @@ namespace ts { } function isTypeAssertion(node: Expression) { - node = skipParenthesizedNodes(node); + node = skipParentheses(node); return node.kind === SyntaxKind.TypeAssertionExpression || node.kind === SyntaxKind.AsExpression; } diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index c08245eee91..85068a4fad5 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -1763,6 +1763,14 @@ "category": "Error", "code": 2538 }, + "Cannot assign to '{0}' because it is not a variable.": { + "category": "Error", + "code": 2539 + }, + "Cannot assign to '{0}' because it is a constant or a read-only property.": { + "category": "Error", + "code": 2540 + }, "JSX element attributes type '{0}' may not be a union type.": { "category": "Error", "code": 2600 diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 2ea4dc71d88..041c0beb075 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -956,10 +956,6 @@ namespace ts { operator: PostfixUnaryOperator; } - export interface PostfixExpression extends UnaryExpression { - _postfixExpressionBrand: any; - } - export interface LeftHandSideExpression extends IncrementExpression { _leftHandSideExpressionBrand: any; } diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 87f6cd5e4aa..f9fe30b4e53 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1624,29 +1624,47 @@ namespace ts { return node && node.dotDotDotToken !== undefined; } + export const enum AssignmentKind { + None, Definite, Compound + } + + export function getAssignmentTargetKind(node: Node): AssignmentKind { + let parent = node.parent; + while (true) { + switch (parent.kind) { + case SyntaxKind.BinaryExpression: + const binaryOperator = (parent).operatorToken.kind; + return isAssignmentOperator(binaryOperator) && (parent).left === node ? + binaryOperator === SyntaxKind.EqualsToken ? AssignmentKind.Definite : AssignmentKind.Compound : + AssignmentKind.None; + case SyntaxKind.PrefixUnaryExpression: + case SyntaxKind.PostfixUnaryExpression: + const unaryOperator = (parent).operator; + return unaryOperator === SyntaxKind.PlusPlusToken || unaryOperator === SyntaxKind.MinusMinusToken ? AssignmentKind.Compound : AssignmentKind.None; + case SyntaxKind.ForInStatement: + case SyntaxKind.ForOfStatement: + return (parent).initializer === node ? AssignmentKind.Definite : AssignmentKind.None; + case SyntaxKind.ParenthesizedExpression: + case SyntaxKind.ArrayLiteralExpression: + case SyntaxKind.SpreadElementExpression: + node = parent; + break; + case SyntaxKind.PropertyAssignment: + case SyntaxKind.ShorthandPropertyAssignment: + node = parent.parent; + break; + default: + return AssignmentKind.None; + } + parent = node.parent; + } + } + // A node is an assignment target if it is on the left hand side of an '=' token, if it is parented by a property // assignment in an object literal that is an assignment target, or if it is parented by an array literal that is // an assignment target. Examples include 'a = xxx', '{ p: a } = xxx', '[{ p: a}] = xxx'. export function isAssignmentTarget(node: Node): boolean { - while (node.parent.kind === SyntaxKind.ParenthesizedExpression) { - node = node.parent; - } - while (true) { - const parent = node.parent; - if (parent.kind === SyntaxKind.ArrayLiteralExpression || parent.kind === SyntaxKind.SpreadElementExpression) { - node = parent; - continue; - } - if (parent.kind === SyntaxKind.PropertyAssignment || parent.kind === SyntaxKind.ShorthandPropertyAssignment) { - node = parent.parent; - continue; - } - return parent.kind === SyntaxKind.BinaryExpression && - isAssignmentOperator((parent).operatorToken.kind) && - (parent).left === node || - (parent.kind === SyntaxKind.ForInStatement || parent.kind === SyntaxKind.ForOfStatement) && - (parent).initializer === node; - } + return getAssignmentTargetKind(node) !== AssignmentKind.None; } export function isNodeDescendantOf(node: Node, ancestor: Node): boolean {