Skip to content

Fix get symbol at location to behave correctly for parameter assignments and jsx attributes #20706

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 8 commits into from
Dec 15, 2017
Merged

Fix get symbol at location to behave correctly for parameter assignments and jsx attributes #20706

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
fdded50
Fix get symbol at location to behave correctly got parameter assignme…
weswigham Dec 14, 2017
fa85c9b
Handle all those edge cases that weren;t explicitly handled
weswigham Dec 15, 2017
f53c795
Fix part of bug WRT getTypeAtLocation and for loops
weswigham Dec 15, 2017
f9e67be
Merge branch 'master' into get-symbol-at-location-fix
weswigham Dec 15, 2017
7405a9c
Baseline corrections pursuant to #20710
weswigham Dec 15, 2017
be49853
Restore jsdoc tag interpretation
weswigham Dec 15, 2017
f947fbf
Clean up some code, revert fourslash tests
weswigham Dec 15, 2017
07c7c6d
Cleanup types used by PR feedback
weswigham Dec 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
105 changes: 64 additions & 41 deletions src/compiler/checker.ts
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions src/compiler/declarationEmitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ namespace ts {
let currentIdentifiers: Map<string>;
let isCurrentFileExternalModule: boolean;
let reportedDeclarationError = false;
let errorNameNode: DeclarationName;
let errorNameNode: DeclarationName | QualifiedName;
const emitJsDocComments = compilerOptions.removeComments ? noop : writeJsDocComments;
const emit = compilerOptions.stripInternal ? stripInternal : emitNode;
let needsDeclare = true;
Expand Down Expand Up @@ -1372,7 +1372,7 @@ namespace ts {
// if this is property of type literal,
// or is parameter of method/call/construct/index signature of type literal
// emit only if type is specified
if (node.type) {
if (hasType(node)) {
write(": ");
emitType(node.type);
}
Expand Down
42 changes: 35 additions & 7 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,20 +88,48 @@ namespace ts {
case SyntaxKind.SpreadAssignment:
return visitNode(cbNode, (<SpreadAssignment>node).expression);
case SyntaxKind.Parameter:
return visitNodes(cbNode, cbNodes, node.decorators) ||
visitNodes(cbNode, cbNodes, node.modifiers) ||
visitNode(cbNode, (<ParameterDeclaration>node).dotDotDotToken) ||
Copy link
Member

Choose a reason for hiding this comment

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

why can't these be shared any more? Are there order conflicts in the current, shared version?

Copy link
Member Author

Choose a reason for hiding this comment

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

The union won't allow access to properties that don't exist on every node. Also, this is less polymorphic, so should have slightly better performance.

visitNode(cbNode, (<ParameterDeclaration>node).name) ||
visitNode(cbNode, (<ParameterDeclaration>node).questionToken) ||
visitNode(cbNode, (<ParameterDeclaration>node).type) ||
visitNode(cbNode, (<ParameterDeclaration>node).initializer);
case SyntaxKind.PropertyDeclaration:
return visitNodes(cbNode, cbNodes, node.decorators) ||
visitNodes(cbNode, cbNodes, node.modifiers) ||
visitNode(cbNode, (<PropertyDeclaration>node).name) ||
visitNode(cbNode, (<PropertyDeclaration>node).questionToken) ||
visitNode(cbNode, (<PropertyDeclaration>node).exclamationToken) ||
visitNode(cbNode, (<PropertyDeclaration>node).type) ||
visitNode(cbNode, (<PropertyDeclaration>node).initializer);
case SyntaxKind.PropertySignature:
return visitNodes(cbNode, cbNodes, node.decorators) ||
visitNodes(cbNode, cbNodes, node.modifiers) ||
visitNode(cbNode, (<PropertySignature>node).name) ||
visitNode(cbNode, (<PropertySignature>node).questionToken) ||
visitNode(cbNode, (<PropertySignature>node).type) ||
visitNode(cbNode, (<PropertySignature>node).initializer);
case SyntaxKind.PropertyAssignment:
return visitNodes(cbNode, cbNodes, node.decorators) ||
visitNodes(cbNode, cbNodes, node.modifiers) ||
visitNode(cbNode, (<PropertyAssignment>node).name) ||
visitNode(cbNode, (<PropertyAssignment>node).questionToken) ||
visitNode(cbNode, (<PropertyAssignment>node).initializer);
case SyntaxKind.VariableDeclaration:
return visitNodes(cbNode, cbNodes, node.decorators) ||
visitNodes(cbNode, cbNodes, node.modifiers) ||
visitNode(cbNode, (<VariableDeclaration>node).name) ||
visitNode(cbNode, (<VariableDeclaration>node).exclamationToken) ||
visitNode(cbNode, (<VariableDeclaration>node).type) ||
visitNode(cbNode, (<VariableDeclaration>node).initializer);
case SyntaxKind.BindingElement:
return visitNodes(cbNode, cbNodes, node.decorators) ||
visitNodes(cbNode, cbNodes, node.modifiers) ||
visitNode(cbNode, (<VariableLikeDeclaration>node).propertyName) ||
visitNode(cbNode, (<VariableLikeDeclaration>node).dotDotDotToken) ||
visitNode(cbNode, (<VariableLikeDeclaration>node).name) ||
visitNode(cbNode, (<VariableLikeDeclaration>node).questionToken) ||
visitNode(cbNode, (<VariableLikeDeclaration>node).exclamationToken) ||
visitNode(cbNode, (<VariableLikeDeclaration>node).type) ||
visitNode(cbNode, (<VariableLikeDeclaration>node).initializer);
visitNode(cbNode, (<BindingElement>node).propertyName) ||
visitNode(cbNode, (<BindingElement>node).dotDotDotToken) ||
visitNode(cbNode, (<BindingElement>node).name) ||
visitNode(cbNode, (<BindingElement>node).initializer);
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructorType:
case SyntaxKind.CallSignature:
Expand Down
68 changes: 49 additions & 19 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,40 @@ namespace ts {
| JSDocFunctionType
| EndOfFileToken;

export type HasType =
| SignatureDeclaration
| VariableDeclaration
| ParameterDeclaration
| PropertySignature
| PropertyDeclaration
| TypePredicateNode
| ParenthesizedTypeNode
| TypeOperatorNode
| MappedTypeNode
| AssertionExpression
| TypeAliasDeclaration
| JSDocTypeExpression
| JSDocNonNullableType
| JSDocNullableType
| JSDocOptionalType
| JSDocVariadicType;

export type HasInitializer =
| HasExpressionInitializer
| ForStatement
| ForInStatement
| ForOfStatement
| JsxAttribute;

export type HasExpressionInitializer =
| VariableDeclaration
| ParameterDeclaration
| BindingElement
| PropertySignature
| PropertyDeclaration
| PropertyAssignment
| EnumMember;

/* @internal */
export type MutableNodeArray<T extends Node> = NodeArray<T> & T[];

Expand Down Expand Up @@ -793,6 +827,9 @@ namespace ts {
initializer?: Expression; // Optional initializer
}

/*@internal*/
export type BindingElementGrandparent = BindingElement["parent"]["parent"];

export interface PropertySignature extends TypeElement, JSDocContainer {
kind: SyntaxKind.PropertySignature;
name: PropertyName; // Declared property name
Expand Down Expand Up @@ -848,25 +885,18 @@ namespace ts {
expression: Expression;
}

// SyntaxKind.VariableDeclaration
// SyntaxKind.Parameter
// SyntaxKind.BindingElement
// SyntaxKind.Property
// SyntaxKind.PropertyAssignment
// SyntaxKind.JsxAttribute
// SyntaxKind.ShorthandPropertyAssignment
// SyntaxKind.EnumMember
// SyntaxKind.JSDocPropertyTag
// SyntaxKind.JSDocParameterTag
export interface VariableLikeDeclaration extends NamedDeclaration {
propertyName?: PropertyName;
dotDotDotToken?: DotDotDotToken;
name: DeclarationName;
questionToken?: QuestionToken;
exclamationToken?: ExclamationToken;
type?: TypeNode;
initializer?: Expression;
}
export type VariableLikeDeclaration =
Copy link
Member

Choose a reason for hiding this comment

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

performance? I remember doing this (or similar) and found it added quite a bit of time to our build.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no noticeable difference in self-compilation (maybe slightly better? The difference is in the noise.).

| VariableDeclaration
| ParameterDeclaration
| BindingElement
| PropertyDeclaration
| PropertyAssignment
| PropertySignature
| JsxAttribute
| ShorthandPropertyAssignment
| EnumMember
| JSDocPropertyTag
| JSDocParameterTag;

export interface PropertyLikeDeclaration extends NamedDeclaration {
name: PropertyName;
Expand Down
30 changes: 24 additions & 6 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ namespace ts {
// Return display name of an identifier
// Computed property names will just be emitted as "[<expr>]", where <expr> is the source
// text of the expression in the computed property.
export function declarationNameToString(name: DeclarationName) {
export function declarationNameToString(name: DeclarationName | QualifiedName) {
return getFullWidth(name) === 0 ? "(Missing)" : getTextOfNode(name);
}

Expand Down Expand Up @@ -784,7 +784,7 @@ namespace ts {
case SyntaxKind.PropertySignature:
case SyntaxKind.Parameter:
case SyntaxKind.VariableDeclaration:
return node === (<VariableLikeDeclaration>parent).type;
return node === (parent as HasType).type;
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction:
Expand Down Expand Up @@ -1340,7 +1340,7 @@ namespace ts {
case SyntaxKind.EnumMember:
case SyntaxKind.PropertyAssignment:
case SyntaxKind.BindingElement:
return (<VariableLikeDeclaration>parent).initializer === node;
return (parent as HasInitializer).initializer === node;
case SyntaxKind.ExpressionStatement:
case SyntaxKind.IfStatement:
case SyntaxKind.DoStatement:
Expand Down Expand Up @@ -1650,7 +1650,7 @@ namespace ts {
result = addRange(result, getJSDocParameterTags(node as ParameterDeclaration));
}

if (isVariableLike(node) && node.initializer && hasJSDocNodes(node.initializer)) {
if (isVariableLike(node) && hasInitializer(node) && hasJSDocNodes(node.initializer)) {
result = addRange(result, node.initializer.jsDoc);
}

Expand Down Expand Up @@ -2816,8 +2816,8 @@ namespace ts {
* Gets the effective type annotation of a variable, parameter, or property. If the node was
* parsed in a JavaScript file, gets the type annotation from JSDoc.
*/
export function getEffectiveTypeAnnotationNode(node: VariableLikeDeclaration, checkJSDoc?: boolean): TypeNode | undefined {
if (node.type) {
export function getEffectiveTypeAnnotationNode(node: Node, checkJSDoc?: boolean): TypeNode | undefined {
if (hasType(node)) {
return node.type;
}
if (checkJSDoc || isInJavaScriptFile(node)) {
Expand Down Expand Up @@ -5812,4 +5812,22 @@ namespace ts {
export function hasJSDocNodes(node: Node): node is HasJSDoc {
return !!(node as JSDocContainer).jsDoc && (node as JSDocContainer).jsDoc.length > 0;
}

/** True if has type node attached to it. */
/* @internal */
export function hasType(node: Node): node is HasType {
return !!(node as HasType).type;
}

/** True if has initializer node attached to it. */
/* @internal */
export function hasInitializer(node: Node): node is HasInitializer {
return !!(node as HasInitializer).initializer;
}

/** True if has initializer node attached to it. */
/* @internal */
export function hasOnlyExpressionInitializer(node: Node): node is HasExpressionInitializer {
return hasInitializer(node) && !isForStatement(node) && !isForInStatement(node) && !isForOfStatement(node) && !isJsxAttribute(node);
}
}
2 changes: 1 addition & 1 deletion src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,7 +1325,7 @@ namespace ts.Completions {
// through type declaration or inference.
// Also proceed if rootDeclaration is a parameter and if its containing function expression/arrow function is contextually typed -
// type of parameter will flow in from the contextual type of the function
let canGetType = rootDeclaration.initializer || rootDeclaration.type || rootDeclaration.parent.parent.kind === SyntaxKind.ForOfStatement;
let canGetType = hasInitializer(rootDeclaration) || hasType(rootDeclaration) || rootDeclaration.parent.parent.kind === SyntaxKind.ForOfStatement;
if (!canGetType && rootDeclaration.kind === SyntaxKind.Parameter) {
if (isExpression(rootDeclaration.parent)) {
canGetType = !!typeChecker.getContextualType(<Expression>rootDeclaration.parent);
Expand Down
16 changes: 7 additions & 9 deletions src/services/findAllReferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@ namespace ts.FindAllReferences.Core {
const containingTypeReference = getContainingTypeReference(refNode);
if (containingTypeReference && state.markSeenContainingTypeReference(containingTypeReference)) {
const parent = containingTypeReference.parent;
if (isVariableLike(parent) && parent.type === containingTypeReference && parent.initializer && isImplementationExpression(parent.initializer)) {
if (hasType(parent) && parent.type === containingTypeReference && hasInitializer(parent) && isImplementationExpression(parent.initializer)) {
addReference(parent.initializer);
}
else if (isFunctionLike(parent) && parent.type === containingTypeReference && (parent as FunctionLikeDeclaration).body) {
Expand Down Expand Up @@ -1670,14 +1670,12 @@ namespace ts.FindAllReferences.Core {
if (!node) {
return false;
}
else if (isVariableLike(node)) {
if (node.initializer) {
return true;
}
else if (node.kind === SyntaxKind.VariableDeclaration) {
const parentStatement = getParentStatementOfVariableDeclaration(<VariableDeclaration>node);
return parentStatement && hasModifier(parentStatement, ModifierFlags.Ambient);
}
else if (isVariableLike(node) && hasInitializer(node)) {
return true;
}
else if (node.kind === SyntaxKind.VariableDeclaration) {
const parentStatement = getParentStatementOfVariableDeclaration(<VariableDeclaration>node);
return parentStatement && hasModifier(parentStatement, ModifierFlags.Ambient);
}
else if (isFunctionLike(node)) {
return !!(node as FunctionLikeDeclaration).body || hasModifier(node, ModifierFlags.Ambient);
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/ES5For-of31.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ var a: string, b: number;

for ({ a: b = 1, b: a = ""} of []) {
>{ a: b = 1, b: a = ""} : { a?: number; b?: string; }
>a : undefined
>a : number
>b = 1 : 1
>b : number
>1 : 1
>b : undefined
>b : string
>a = "" : ""
>a : string
>"" : ""
Expand Down
13 changes: 4 additions & 9 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,9 @@ declare namespace ts {
interface JSDocContainer {
}
type HasJSDoc = ParameterDeclaration | CallSignatureDeclaration | ConstructSignatureDeclaration | MethodSignature | PropertySignature | ArrowFunction | ParenthesizedExpression | SpreadAssignment | ShorthandPropertyAssignment | PropertyAssignment | FunctionExpression | LabeledStatement | ExpressionStatement | VariableStatement | FunctionDeclaration | ConstructorDeclaration | MethodDeclaration | PropertyDeclaration | AccessorDeclaration | ClassLikeDeclaration | InterfaceDeclaration | TypeAliasDeclaration | EnumMember | EnumDeclaration | ModuleDeclaration | ImportEqualsDeclaration | IndexSignatureDeclaration | FunctionTypeNode | ConstructorTypeNode | JSDocFunctionType | EndOfFileToken;
type HasType = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertySignature | PropertyDeclaration | TypePredicateNode | ParenthesizedTypeNode | TypeOperatorNode | MappedTypeNode | AssertionExpression | TypeAliasDeclaration | JSDocTypeExpression | JSDocNonNullableType | JSDocNullableType | JSDocOptionalType | JSDocVariadicType;
Copy link
Member

Choose a reason for hiding this comment

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

are these aliases meant to be public? I guess maybe they are required for export to services/

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make them public (like HasJSDoc), because knowing what nodes have a .type or a .initializer is a useful set of nodes, IMO. It can be internal, since I don't think any public functions use them?

Copy link
Member

Choose a reason for hiding this comment

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

I don't care much either way, just checking.

type HasInitializer = HasExpressionInitializer | ForStatement | ForInStatement | ForOfStatement | JsxAttribute;
type HasExpressionInitializer = VariableDeclaration | ParameterDeclaration | BindingElement | PropertySignature | PropertyDeclaration | PropertyAssignment | EnumMember;
interface NodeArray<T extends Node> extends ReadonlyArray<T>, TextRange {
hasTrailingComma?: boolean;
}
Expand Down Expand Up @@ -604,15 +607,7 @@ declare namespace ts {
kind: SyntaxKind.SpreadAssignment;
expression: Expression;
}
interface VariableLikeDeclaration extends NamedDeclaration {
propertyName?: PropertyName;
dotDotDotToken?: DotDotDotToken;
name: DeclarationName;
questionToken?: QuestionToken;
exclamationToken?: ExclamationToken;
type?: TypeNode;
initializer?: Expression;
}
type VariableLikeDeclaration = VariableDeclaration | ParameterDeclaration | BindingElement | PropertyDeclaration | PropertyAssignment | PropertySignature | JsxAttribute | ShorthandPropertyAssignment | EnumMember | JSDocPropertyTag | JSDocParameterTag;
interface PropertyLikeDeclaration extends NamedDeclaration {
name: PropertyName;
}
Expand Down
13 changes: 4 additions & 9 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,9 @@ declare namespace ts {
interface JSDocContainer {
}
type HasJSDoc = ParameterDeclaration | CallSignatureDeclaration | ConstructSignatureDeclaration | MethodSignature | PropertySignature | ArrowFunction | ParenthesizedExpression | SpreadAssignment | ShorthandPropertyAssignment | PropertyAssignment | FunctionExpression | LabeledStatement | ExpressionStatement | VariableStatement | FunctionDeclaration | ConstructorDeclaration | MethodDeclaration | PropertyDeclaration | AccessorDeclaration | ClassLikeDeclaration | InterfaceDeclaration | TypeAliasDeclaration | EnumMember | EnumDeclaration | ModuleDeclaration | ImportEqualsDeclaration | IndexSignatureDeclaration | FunctionTypeNode | ConstructorTypeNode | JSDocFunctionType | EndOfFileToken;
type HasType = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertySignature | PropertyDeclaration | TypePredicateNode | ParenthesizedTypeNode | TypeOperatorNode | MappedTypeNode | AssertionExpression | TypeAliasDeclaration | JSDocTypeExpression | JSDocNonNullableType | JSDocNullableType | JSDocOptionalType | JSDocVariadicType;
type HasInitializer = HasExpressionInitializer | ForStatement | ForInStatement | ForOfStatement | JsxAttribute;
type HasExpressionInitializer = VariableDeclaration | ParameterDeclaration | BindingElement | PropertySignature | PropertyDeclaration | PropertyAssignment | EnumMember;
interface NodeArray<T extends Node> extends ReadonlyArray<T>, TextRange {
hasTrailingComma?: boolean;
}
Expand Down Expand Up @@ -604,15 +607,7 @@ declare namespace ts {
kind: SyntaxKind.SpreadAssignment;
expression: Expression;
}
interface VariableLikeDeclaration extends NamedDeclaration {
propertyName?: PropertyName;
dotDotDotToken?: DotDotDotToken;
name: DeclarationName;
questionToken?: QuestionToken;
exclamationToken?: ExclamationToken;
type?: TypeNode;
initializer?: Expression;
}
type VariableLikeDeclaration = VariableDeclaration | ParameterDeclaration | BindingElement | PropertyDeclaration | PropertyAssignment | PropertySignature | JsxAttribute | ShorthandPropertyAssignment | EnumMember | JSDocPropertyTag | JSDocParameterTag;
interface PropertyLikeDeclaration extends NamedDeclaration {
name: PropertyName;
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/constructorWithIncompleteTypeAnnotation.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,9 +475,9 @@ module TypeScriptAllInOne {
for (var x in { x: 0, y: 1 }) {
>x : string
>{ x: 0, y: 1 } : { x: number; y: number; }
>x : string
>x : number
>0 : 0
>y : string
>y : number
>1 : 1

!
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/contextualTypeObjectSpreadExpression.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ i = { ...{ a: "a" } };
>i : I
>{ ...{ a: "a" } } : { a: "a"; }
>{ a: "a" } : { a: "a"; }
>a : string
>a : "a"
>"a" : "a"

Loading