Skip to content

Get references and rename to include property in the search when short hand property assignment infers the property in the declaration #15850

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

Closed
sheetalkamat wants to merge 8 commits into from
Closed

Get references and rename to include property in the search when short hand property assignment infers the property in the declaration #15850

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
89add94
Initial non working test for reference groups
sheetalkamat May 5, 2017
94a2c48
Reference for value symbol of short hand property is included when fi…
sheetalkamat May 5, 2017
31e07c3
If binding element symbol is in search, make sure to add reference to…
sheetalkamat May 5, 2017
f49ad71
Add search for the property symbol if it gets inferred from short han…
sheetalkamat May 5, 2017
7164675
Add search for value symbol of the short hand property if it infers t…
sheetalkamat May 5, 2017
f49d74a
Add test case for symbols and types from destructuring property assig…
sheetalkamat May 9, 2017
395c9a9
Update the get symbol at location to handle property from destructuri…
sheetalkamat May 9, 2017
57adfce
Add test for #10791
sheetalkamat May 15, 2017
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
87 changes: 61 additions & 26 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ namespace ts {
return node ? getSymbolAtLocation(node) : undefined;
},
getShorthandAssignmentValueSymbol: node => {
node = getParseTreeNode(node);
node = getParseTreeNode(node, isShorthandPropertyAssignment);
return node ? getShorthandAssignmentValueSymbol(node) : undefined;
},
getExportSpecifierLocalTargetSymbol: node => {
Expand All @@ -132,10 +132,6 @@ namespace ts {
node = getParseTreeNode(node);
return node ? getTypeOfNode(node) : unknownType;
},
getPropertySymbolOfDestructuringAssignment: location => {
location = getParseTreeNode(location, isIdentifier);
return location ? getPropertySymbolOfDestructuringAssignment(location) : undefined;
},
signatureToString: (signature, enclosingDeclaration?, flags?, kind?) => {
return signatureToString(signature, getParseTreeNode(enclosingDeclaration), flags, kind);
},
Expand Down Expand Up @@ -10733,11 +10729,6 @@ namespace ts {
getTypeOfExpression(node.right);
}

function isDestructuringAssignmentTarget(parent: Node) {
return parent.parent.kind === SyntaxKind.BinaryExpression && (parent.parent as BinaryExpression).left === parent ||
parent.parent.kind === SyntaxKind.ForOfStatement && (parent.parent as ForOfStatement).initializer === parent;
}

function getAssignedTypeOfArrayLiteralElement(node: ArrayLiteralExpression, element: Expression): Type {
return getTypeOfDestructuredArrayElement(getAssignedType(node), indexOf(node.elements, element));
}
Expand Down Expand Up @@ -16824,12 +16815,7 @@ namespace ts {
if (exprOrAssignment.kind === SyntaxKind.ShorthandPropertyAssignment) {
const prop = <ShorthandPropertyAssignment>exprOrAssignment;
if (prop.objectAssignmentInitializer) {
// In strict null checking mode, if a default value of a non-undefined type is specified, remove
// undefined from the final type.
if (strictNullChecks &&
!(getFalsyFlags(checkExpression(prop.objectAssignmentInitializer)) & TypeFlags.Undefined)) {
sourceType = getTypeWithFacts(sourceType, TypeFacts.NEUndefined);
}
sourceType = updateTypeFromShorthandAssignmentInitializer(prop.objectAssignmentInitializer, sourceType);
checkBinaryLikeExpression(prop.name, prop.equalsToken, prop.objectAssignmentInitializer, checkMode);
}
target = (<ShorthandPropertyAssignment>exprOrAssignment).name;
Expand All @@ -16851,6 +16837,16 @@ namespace ts {
return checkReferenceAssignment(target, sourceType, checkMode);
}

function updateTypeFromShorthandAssignmentInitializer(objectAssignmentInitializer: Expression, sourceType: Type) {
// In strict null checking mode, if a default value of a non-undefined type is specified, remove
// undefined from the final type.
if (strictNullChecks &&
!(getFalsyFlags(checkExpression(objectAssignmentInitializer)) & TypeFlags.Undefined)) {

@ghost ghost May 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it safe if checkExpression is called from inside getTypeOfNode? Could that cause duplicate diagnostics?

sourceType = getTypeWithFacts(sourceType, TypeFacts.NEUndefined);

@ghost ghost May 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just return getTypeWithFacts(...) early.

}
return sourceType;
}

function checkReferenceAssignment(target: Expression, sourceType: Type, checkMode?: CheckMode): Type {
const targetType = checkExpression(target, checkMode);
const error = target.parent.kind === SyntaxKind.SpreadAssignment ?
Expand Down Expand Up @@ -22267,6 +22263,25 @@ namespace ts {
return undefined;
}

function isDeclarationNameOfPropertyFromDestructuringAssignmentTarget(node: Node): node is PropertyName {
return (isShorthandPropertyAssignment(node.parent) || isPropertyAssignment(node.parent)) &&
isObjectLiteralExpression(node.parent.parent) &&

@ghost ghost May 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this check necessary? Maybe we should add parent: ObjectLiteral to PropertyAssignment and ShorthandPropertyAssignment.

isFromDestructuringAssignmentPatternTarget(node.parent.parent);
}

function getSymbolOfNodeFromDeclarationName(node: Node) {
// If the location is name of property from object literal destructuring pattern
// Get the property symbol from the object literal's type
// In below eg. get 'property' from type of elems iterating type
// for ({ property: p2 } of elems) { }
if (isDeclarationNameOfPropertyFromDestructuringAssignmentTarget(node)) {
return getPropertySymbolOfDestructuringAssignment(node);
}

// This is a declaration, call getSymbolOfNode
return getSymbolOfNode(node.parent);
}

function getSymbolAtLocation(node: Node) {
if (node.kind === SyntaxKind.SourceFile) {
return isExternalModule(<SourceFile>node) ? getMergedSymbol(node.symbol) : undefined;
Expand All @@ -22279,10 +22294,10 @@ namespace ts {

if (isDeclarationNameOrImportPropertyName(node)) {
// This is a declaration, call getSymbolOfNode
return getSymbolOfNode(node.parent);
return getSymbolOfNodeFromDeclarationName(node);
}
else if (isLiteralComputedPropertyDeclarationName(node)) {
return getSymbolOfNode(node.parent.parent);
return getSymbolOfNodeFromDeclarationName(node.parent);
}

if (node.kind === SyntaxKind.Identifier) {
Expand Down Expand Up @@ -22359,14 +22374,11 @@ namespace ts {
return undefined;
}

function getShorthandAssignmentValueSymbol(location: Node): Symbol {
function getShorthandAssignmentValueSymbol(location: ShorthandPropertyAssignment): Symbol {
// The function returns a value symbol of an identifier in the short-hand property assignment.
// This is necessary as an identifier in short-hand property assignment can contains two meaning:
// property name and property value.
if (location && location.kind === SyntaxKind.ShorthandPropertyAssignment) {
return resolveEntityName((<ShorthandPropertyAssignment>location).name, SymbolFlags.Value | SymbolFlags.Alias);
}
return undefined;
return resolveEntityName(location.name, SymbolFlags.Value | SymbolFlags.Alias);
}

/** Returns the target of an export specifier without following aliases */
Expand Down Expand Up @@ -22395,6 +22407,9 @@ namespace ts {
}

if (isPartOfExpression(node)) {
if (isFromDestructuringAssignmentPatternTarget(node)) {
return getTypeOfArrayLiteralOrObjectLiteralDestructuringAssignment(node);
}
return getRegularTypeOfExpression(<Expression>node);
}

Expand Down Expand Up @@ -22424,7 +22439,27 @@ namespace ts {
return getTypeOfSymbol(symbol);
}

if (isDeclarationNameOrImportPropertyName(node)) {
if (isDeclarationNameOrImportPropertyName(node) || isLiteralComputedPropertyDeclarationName(node)) {
const propertyName = isDeclarationName(node) ? node : node.parent;
// If the location is name of property from object literal destructuring pattern
// Get the property symbol from the object literal's type and get the type from there
// In below eg. get type of 'property' from type of elems iterating type
// for ({ property: p2 } of elems) { }
if (isDeclarationNameOfPropertyFromDestructuringAssignmentTarget(propertyName)) {
// Get the type of the object or array literal and then look for property of given name in the type
const objectLiteralType = getTypeOfArrayLiteralOrObjectLiteralDestructuringAssignment(<Expression>propertyName.parent.parent);
const text = getTextOfPropertyName(propertyName);
let type = isTypeAny(objectLiteralType)
? objectLiteralType
: getTypeOfPropertyOfType(objectLiteralType, text) ||
isNumericLiteralName(text) && getIndexTypeOfType(objectLiteralType, IndexKind.Number) ||
getIndexTypeOfType(objectLiteralType, IndexKind.String);
if (type && isShorthandPropertyAssignment(propertyName.parent) && propertyName.parent.objectAssignmentInitializer) {
// Update the type with strict null check flags
type = updateTypeFromShorthandAssignmentInitializer(propertyName.parent.objectAssignmentInitializer, type);

@ghost ghost May 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: Just return early.

}
return type;
}
const symbol = getSymbolAtLocation(node);
return symbol && getTypeOfSymbol(symbol);
}
Expand Down Expand Up @@ -22484,10 +22519,10 @@ namespace ts {
// }
// 'property1' at location 'a' from:
// [a] = [ property1, property2 ]
function getPropertySymbolOfDestructuringAssignment(location: Identifier) {
function getPropertySymbolOfDestructuringAssignment(location: PropertyName) {
// Get the type of the object or array literal and then look for property of given name in the type
const typeOfObjectLiteral = getTypeOfArrayLiteralOrObjectLiteralDestructuringAssignment(<Expression>location.parent.parent);
return typeOfObjectLiteral && getPropertyOfType(typeOfObjectLiteral, location.text);
return typeOfObjectLiteral && getPropertyOfType(typeOfObjectLiteral, getTextOfPropertyName(location));
}

function getRegularTypeOfExpression(expr: Expression): Type {
Expand Down
3 changes: 1 addition & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2529,9 +2529,8 @@ namespace ts {
getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[];
getSymbolAtLocation(node: Node): Symbol;
getSymbolsOfParameterPropertyDeclaration(parameter: ParameterDeclaration, parameterName: string): Symbol[];
getShorthandAssignmentValueSymbol(location: Node): Symbol;
getShorthandAssignmentValueSymbol(location: ShorthandPropertyAssignment): Symbol;
getExportSpecifierLocalTargetSymbol(location: ExportSpecifier): Symbol;
getPropertySymbolOfDestructuringAssignment(location: Identifier): Symbol;

@ghost ghost May 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a breaking change, can you document why it's removed and what people should do instead?

getTypeAtLocation(node: Node): Type;
getTypeFromTypeNode(node: TypeNode): Type;
signatureToString(signature: Signature, enclosingDeclaration?: Node, flags?: TypeFormatFlags, kind?: SignatureKind): string;
Expand Down
32 changes: 32 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3754,6 +3754,38 @@ namespace ts {
|| kind === SyntaxKind.ObjectLiteralExpression;
}

export function isDestructuringAssignmentTarget(node: Node) {
// [a,b,c] from:
// [a, b, c] = someExpression;
return isAssignmentExpression(node.parent, /*excludeCompoundAssignment*/ true) && node.parent.left === node ||
// [a, b, c] from:
// for([a, b, c] of expression)
node.parent.kind === SyntaxKind.ForOfStatement && (node.parent as ForOfStatement).initializer === node;
}

export function isFromDestructuringAssignmentPatternTarget(node: Node): node is AssignmentPattern {
if (isAssignmentPattern(node)) {

@ghost ghost May 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: I would use an early return here.

// [a,b,c] from:
// [a, b, c] = someExpression;
// or
// [a, b, c] from:
// for([a, b, c] of expression)
if (isDestructuringAssignmentTarget(node)) {
return true;
}

// [a, b, c] of
// [x, [a, b, c] ] = someExpression
// or
// {x, a: {a, b, c} } = someExpression
if (isFromDestructuringAssignmentPatternTarget(node.parent.kind === SyntaxKind.PropertyAssignment ? node.parent.parent : node.parent)) {
return true;
}
}

return false;
}

export function isBindingElement(node: Node): node is BindingElement {
return node.kind === SyntaxKind.BindingElement;
}
Expand Down
16 changes: 8 additions & 8 deletions src/services/breakpoints.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ namespace ts.BreakpointResolver {
// Destructuring pattern in destructuring assignment
// [a, b, c] of
// [a, b, c] = expression
if (isArrayLiteralOrObjectLiteralDestructuringPattern(node)) {
if (isFromDestructuringAssignmentPatternTarget(node)) {
return spanInArrayLiteralOrObjectLiteralDestructuringPattern(<DestructuringPattern>node);
}

Expand All @@ -268,7 +268,7 @@ namespace ts.BreakpointResolver {
node.kind === SyntaxKind.SpreadElement ||
node.kind === SyntaxKind.PropertyAssignment ||
node.kind === SyntaxKind.ShorthandPropertyAssignment) &&
isArrayLiteralOrObjectLiteralDestructuringPattern(node.parent)) {
isFromDestructuringAssignmentPatternTarget(node.parent)) {
return textSpan(node);
}

Expand All @@ -278,13 +278,13 @@ namespace ts.BreakpointResolver {
// [a, b, c] or {a, b, c} of
// [a, b, c] = expression or
// {a, b, c} = expression
if (isArrayLiteralOrObjectLiteralDestructuringPattern(binaryExpression.left)) {
if (isFromDestructuringAssignmentPatternTarget(binaryExpression.left)) {
return spanInArrayLiteralOrObjectLiteralDestructuringPattern(
<ArrayLiteralExpression | ObjectLiteralExpression>binaryExpression.left);
}

if (binaryExpression.operatorToken.kind === SyntaxKind.EqualsToken &&
isArrayLiteralOrObjectLiteralDestructuringPattern(binaryExpression.parent)) {
isFromDestructuringAssignmentPatternTarget(binaryExpression.parent)) {
// Set breakpoint on assignment expression element of destructuring pattern
// a = expression of
// [a = expression, b, c] = someExpression or
Expand Down Expand Up @@ -330,7 +330,7 @@ namespace ts.BreakpointResolver {
// If this is name of property assignment, set breakpoint in the initializer
if (node.parent.kind === SyntaxKind.PropertyAssignment &&
(<PropertyDeclaration>node.parent).name === node &&
!isArrayLiteralOrObjectLiteralDestructuringPattern(node.parent.parent)) {
!isFromDestructuringAssignmentPatternTarget(node.parent.parent)) {
return spanInNode((<PropertyDeclaration>node.parent).initializer);
}

Expand All @@ -357,7 +357,7 @@ namespace ts.BreakpointResolver {

if (node.parent.kind === SyntaxKind.BinaryExpression) {
const binaryExpression = <BinaryExpression>node.parent;
if (isArrayLiteralOrObjectLiteralDestructuringPattern(binaryExpression.left) &&
if (isFromDestructuringAssignmentPatternTarget(binaryExpression.left) &&
(binaryExpression.right === node ||
binaryExpression.operatorToken === node)) {
// If initializer of destructuring assignment move to previous token
Expand Down Expand Up @@ -617,7 +617,7 @@ namespace ts.BreakpointResolver {

// Default to parent node
default:
if (isArrayLiteralOrObjectLiteralDestructuringPattern(node.parent)) {
if (isFromDestructuringAssignmentPatternTarget(node.parent)) {
// Breakpoint in last binding element or binding pattern if it contains no elements
const objectLiteral = <ObjectLiteralExpression>node.parent;
return textSpan(lastOrUndefined(objectLiteral.properties) || objectLiteral);
Expand All @@ -634,7 +634,7 @@ namespace ts.BreakpointResolver {
return textSpan(lastOrUndefined(bindingPattern.elements) || bindingPattern);

default:
if (isArrayLiteralOrObjectLiteralDestructuringPattern(node.parent)) {
if (isFromDestructuringAssignmentPatternTarget(node.parent)) {
// Breakpoint in last binding element or binding pattern if it contains no elements
const arrayLiteral = <ArrayLiteralExpression>node.parent;
return textSpan(lastOrUndefined(arrayLiteral.elements) || arrayLiteral);
Expand Down
Loading