From 66f30c9841bebaf8e0c62da4ed36a64ee7b35ca0 Mon Sep 17 00:00:00 2001 From: Richard Knoll Date: Tue, 6 Sep 2016 17:02:23 -0700 Subject: [PATCH] PR feedback --- src/harness/fourslash.ts | 2 +- src/services/services.ts | 188 ++++++------------ .../goToImplementationInterfaceMethod_00.ts | 1 + .../goToImplementationInterfaceMethod_08.ts | 2 +- .../goToImplementationInterfaceMethod_09.ts | 4 + .../goToImplementationNamespace_00.ts | 33 ++- .../goToImplementationNamespace_01.ts | 10 +- .../goToImplementationNamespace_02.ts | 2 +- .../goToImplementationNamespace_03.ts | 30 ++- .../goToImplementationNamespace_04.ts | 40 ++-- .../goToImplementationNamespace_05.ts | 26 --- .../goToImplementationNamespace_06.ts | 28 --- .../goToImplementationNamespace_07.ts | 28 --- 13 files changed, 146 insertions(+), 248 deletions(-) delete mode 100644 tests/cases/fourslash/goToImplementationNamespace_05.ts delete mode 100644 tests/cases/fourslash/goToImplementationNamespace_06.ts delete mode 100644 tests/cases/fourslash/goToImplementationNamespace_07.ts diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index c577d2dd4bb..80baca88a70 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -3017,7 +3017,7 @@ namespace FourSlashInterface { public typeDefinitionCountIs(expectedCount: number) { this.state.verifyTypeDefinitionsCount(this.negative, expectedCount); } - + public implementationCountIs(expectedCount: number) { this.state.verifyImplementationsCount(this.negative, expectedCount); } diff --git a/src/services/services.ts b/src/services/services.ts index 79172287e6e..013e093336e 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -5010,38 +5010,25 @@ namespace ts { function getImplementationAtPosition(fileName: string, position: number): ImplementationLocation[] { synchronizeHostData(); - const entries: ImplementationLocation[] = []; const node = getTouchingPropertyName(getValidSourceFile(fileName), position); const typeChecker = program.getTypeChecker(); // If invoked directly on a shorthand property assignment, then return // the declaration of the symbol being assigned (not the symbol being assigned to). if (node.parent.kind === SyntaxKind.ShorthandPropertyAssignment) { - const entry = getReferenceEntryForShorthandPropertyAssignment(node, typeChecker); - entries.push({ - textSpan: entry.textSpan, - fileName: entry.fileName - }); + return getReferenceEntryForShorthandPropertyAssignment(node, typeChecker); } - - // For most symbols, the definition is the same as the implementation so we can just - // call "Go to Definition". This case should handle anything that is not a type - // reference to or member of an interface, class, or union/intersection type. - else if (definitionIsImplementation(node, typeChecker)) { - const definitions = getDefinitionAtPosition(fileName, position); - forEach(definitions, (definition: DefinitionInfo) => { - entries.push({ - textSpan: definition.textSpan, - fileName: definition.fileName - }); - }); + else if (node.kind === SyntaxKind.SuperKeyword || isSuperPropertyOrElementAccess(node.parent)) { + // References to and accesses on the super keyword only have one possible implementation, so no + // need to "Find all References" + const symbol = typeChecker.getSymbolAtLocation(node); + if (symbol.valueDeclaration) { + return [getReferenceEntryFromNode(symbol.valueDeclaration)]; + } } - - // Interfaces, classes, and unions/intersection types separate the implementation and - // definition so "Go to Definition" is not sufficient. This case handles invocations - // on type references and members of those types. else { - // Perform "Find all References" and filter them down to implementations only + const entries: ImplementationLocation[] = []; + // Perform "Find all References" and retrieve only those that are implementations const result = getReferencedSymbolsForNode(node, program.getSourceFiles(), /*findInStrings*/false, /*findInComments*/false, /*implementations*/true); forEach(result, referencedSymbol => { @@ -5054,44 +5041,8 @@ namespace ts { }); } }); + return entries; } - - return entries; - } - - /** - * Returns true if the implementation for this node is the same as its definition - */ - function definitionIsImplementation(node: Node, typeChecker: TypeChecker) { - if (node.kind === SyntaxKind.SuperKeyword || node.kind === SyntaxKind.ThisKeyword) { - return true; - } - - if (node.parent.kind === SyntaxKind.PropertyAccessExpression || node.parent.kind === SyntaxKind.ElementAccessExpression) { - const expression = (node.parent).expression; - - // Members of "this" and "super" only have one possible implementation, so no need to find - // all references. Similarly, for the left hand side of the expression it only really - // makes sense to return the definition - if (node === expression || expression.kind === SyntaxKind.SuperKeyword || expression.kind === SyntaxKind.ThisKeyword) { - return true; - } - - // Check to see if this is a property that can have multiple implementations by determining - // if the parent is an interface (or class, or union/intersection) - const expressionType = typeChecker.getTypeAtLocation(expression); - if (expressionType && expressionType.getFlags() & (TypeFlags.Class | TypeFlags.Interface | TypeFlags.UnionOrIntersection)) { - return false; - } - - // Also check the right hand side to see if this is a type being accessed on a namespace/module. - // For example, SomeModule.SomeType - const rightHandType = typeChecker.getTypeAtLocation(node); - return rightHandType && !(rightHandType.getFlags() & (TypeFlags.Class | TypeFlags.Interface | TypeFlags.UnionOrIntersection)); - } - - const symbol = typeChecker.getSymbolAtLocation(node); - return symbol && !isClassOrInterfaceReference(symbol) && !(symbol.parent && isClassOrInterfaceReference(symbol.parent)); } function getReferenceEntryForShorthandPropertyAssignment(node: Node, typeChecker: TypeChecker) { @@ -5099,21 +5050,15 @@ namespace ts { const shorthandSymbol = typeChecker.getShorthandAssignmentValueSymbol(refSymbol.valueDeclaration); if (shorthandSymbol) { - const shorthandDeclarations = shorthandSymbol.getDeclarations(); - if (shorthandDeclarations.length === 1) { - return getReferenceEntryFromNode(shorthandDeclarations[0]); - } - else if (shorthandDeclarations.length > 1) { - // This can happen when the property being assigned is a constructor for a - // class that also has interface declarations with the same name. We just want - // the class itself + const result: ReferenceEntry[] = []; - return forEach(shorthandDeclarations, declaration => { - if (declaration.kind === SyntaxKind.ClassDeclaration) { - return getReferenceEntryFromNode(declaration); - } - }); + for (const declaration of shorthandSymbol.getDeclarations()) { + if (getMeaningFromDeclaration(declaration) & SemanticMeaning.Value) { + result.push(getReferenceEntryFromNode(declaration)); + } } + + return result; } } @@ -5121,11 +5066,14 @@ namespace ts { return toCheck.getFlags() & (SymbolFlags.Class | SymbolFlags.Interface); } - function isIdentifierOfImplementation(node: Identifier): boolean { + function isNameOfImplementation(node: Node): boolean { const parent = node.parent; - if (isIdentifierName(node) || isFunctionDeclarationIdentifierName(node)) { - // PropertyAccessExpression? + if (isDeclarationName(node)) { + if (isVariableLike(parent)) { + return !!parent.initializer; + } + switch (parent.kind) { case SyntaxKind.FunctionDeclaration: case SyntaxKind.FunctionExpression: @@ -5134,31 +5082,15 @@ namespace ts { case SyntaxKind.GetAccessor: case SyntaxKind.SetAccessor: return !!(parent).body; - case SyntaxKind.PropertyDeclaration: - return !!(parent).initializer; case SyntaxKind.PropertyAssignment: + case SyntaxKind.ClassDeclaration: + case SyntaxKind.ClassExpression: + case SyntaxKind.EnumDeclaration: return true; } } - else if (isVariableLike(parent) && parent.name === node) { - return !!parent.initializer; - } - return isIdentifierOfClass(node) || isIdentifierOfEnumDeclaration(node); - } - - function isFunctionDeclarationIdentifierName(node: Identifier): boolean { - return node.parent.kind === SyntaxKind.FunctionDeclaration && - (node.parent).name === node; - } - - function isIdentifierOfClass(node: Identifier) { - return (node.parent.kind === SyntaxKind.ClassDeclaration || node.parent.kind === SyntaxKind.ClassExpression) && - (node.parent).name === node; - } - - function isIdentifierOfEnumDeclaration(node: Identifier) { - return node.parent.kind === SyntaxKind.EnumDeclaration && (node.parent).name === node; + return false; } function isTypeAssertionExpression(node: Node): node is TypeAssertion { @@ -5927,12 +5859,11 @@ namespace ts { if (node.kind === SyntaxKind.SuperKeyword) { return getReferencesForSuperKeyword(node); } - } // `getSymbolAtLocation` normally returns the symbol of the class when given the constructor keyword, - // so we have to specify that we want the constructor symbol. - const symbol = typeChecker.getSymbolAtLocation(node); + // so we have to specify that we want the constructor symbol. + const symbol = typeChecker.getSymbolAtLocation(node); if (!implementations && !symbol && node.kind === SyntaxKind.StringLiteral) { return getReferencesForStringLiteral(node, sourceFiles); @@ -6325,12 +6256,7 @@ namespace ts { /*searchLocationIsConstructor*/ searchLocation.kind === SyntaxKind.ConstructorKeyword, parentSymbols); if (relatedSymbol) { - const referenceEntry = implementations ? getImplementationReferenceEntryForNode(referenceLocation) : getReferenceEntryFromNode(referenceLocation); - - if (referenceEntry) { - const referencedSymbol = getReferencedSymbol(relatedSymbol); - referencedSymbol.references.push(referenceEntry); - } + addReferenceToRelatedSymbol(referenceLocation, relatedSymbol); } /* Because in short-hand property assignment, an identifier which stored as name of the short-hand property assignment * has two meaning : property name and property value. Therefore when we do findAllReference at the position where @@ -6339,11 +6265,7 @@ namespace ts { * position of property accessing, the referenceEntry of such position will be handled in the first case. */ else if (!(referenceSymbol.flags & SymbolFlags.Transient) && searchSymbols.indexOf(shorthandValueSymbol) >= 0) { - const referenceEntry = implementations ? getImplementationReferenceEntryForNode(referenceSymbolDeclaration.name) : getReferenceEntryFromNode(referenceSymbolDeclaration.name); - if (referenceEntry) { - const referencedSymbol = getReferencedSymbol(shorthandValueSymbol); - referencedSymbol.references.push(referenceEntry); - } + addReferenceToRelatedSymbol(referenceSymbolDeclaration.name, shorthandValueSymbol); } else if (searchLocation.kind === SyntaxKind.ConstructorKeyword) { findAdditionalConstructorReferences(referenceSymbol, referenceLocation); @@ -6448,25 +6370,43 @@ namespace ts { return result[index]; } + + function addReferenceToRelatedSymbol(node: Node, relatedSymbol: Symbol) { + if (implementations) { + const referenceEntries = getImplementationReferenceEntryForNode(node); + if (referenceEntries && referenceEntries.length) { + const referencedSymbol = getReferencedSymbol(relatedSymbol); + for (const referenceEntry of referenceEntries) { + referencedSymbol.references.push(referenceEntry); + } + } + } + else { + const referenceEntry = getReferenceEntryFromNode(node); + if (referenceEntry) { + getReferencedSymbol(relatedSymbol).references.push(referenceEntry); + } + } + } } - function getImplementationReferenceEntryForNode(refNode: Node): ReferenceEntry { - // Check to make sure this reference is either part of a sub class or a class that we explicitly - // inherit from in the class hierarchy + function getImplementationReferenceEntryForNode(refNode: Node): ReferenceEntry[] { + // Check if we found a function/propertyAssignment/method with an implementation or initializer + if (isNameOfImplementation(refNode)) { + return [getReferenceEntryFromNode(refNode.parent)]; + } + if (refNode.kind === SyntaxKind.Identifier) { - // Check if we found a function/propertyAssignment/method with an implementation or initializer - if (isIdentifierOfImplementation(refNode)) { - return getReferenceEntryFromNode(refNode.parent); - } - else if (refNode.parent.kind === SyntaxKind.ShorthandPropertyAssignment) { + if (refNode.parent.kind === SyntaxKind.ShorthandPropertyAssignment) { // Go ahead and dereference the shorthand assignment by going to its definition - return getReferenceEntryForShorthandPropertyAssignment(refNode, typeChecker); + const referenceEntries = getReferenceEntryForShorthandPropertyAssignment(refNode, typeChecker); + return referenceEntries && referenceEntries.length ? referenceEntries : undefined; } // Check if the node is within an extends or implements clause const containingHeritageClause = getContainingClassHeritageClause(refNode); if (containingHeritageClause) { - return getReferenceEntryFromNode(containingHeritageClause.parent); + return [getReferenceEntryFromNode(containingHeritageClause.parent)]; } // If we got a type reference, try and see if the reference applies to any expressions that can implement an interface @@ -6474,17 +6414,17 @@ namespace ts { if (containingTypeReference) { const parent = containingTypeReference.parent; if (isVariableLike(parent) && parent.type === containingTypeReference && parent.initializer && isImplementationExpression(parent.initializer)) { - return getReferenceEntryFromNode(parent.initializer); + return [getReferenceEntryFromNode(parent.initializer)]; } else if (isFunctionLike(parent) && parent.type === containingTypeReference && parent.body && parent.body.kind === SyntaxKind.Block) { - return forEachReturnStatement(parent.body, (returnStatement) => { + return [forEachReturnStatement(parent.body, (returnStatement) => { if (returnStatement.expression && isImplementationExpression(returnStatement.expression)) { return getReferenceEntryFromNode(returnStatement.expression); } - }); + })]; } else if (isTypeAssertionExpression(parent) && isImplementationExpression(parent.expression)) { - return getReferenceEntryFromNode(parent.expression); + return [getReferenceEntryFromNode(parent.expression)]; } } } diff --git a/tests/cases/fourslash/goToImplementationInterfaceMethod_00.ts b/tests/cases/fourslash/goToImplementationInterfaceMethod_00.ts index 71e6266aa20..ca5498c3492 100644 --- a/tests/cases/fourslash/goToImplementationInterfaceMethod_00.ts +++ b/tests/cases/fourslash/goToImplementationInterfaceMethod_00.ts @@ -7,6 +7,7 @@ //// } //// //// var bar: Foo = { [|hello: helloImpl|] }; +//// var baz: Foo = { [|"hello": helloImpl|] }; //// //// function helloImpl () {} //// diff --git a/tests/cases/fourslash/goToImplementationInterfaceMethod_08.ts b/tests/cases/fourslash/goToImplementationInterfaceMethod_08.ts index 7d0be4a38a6..e3db4b5157d 100644 --- a/tests/cases/fourslash/goToImplementationInterfaceMethod_08.ts +++ b/tests/cases/fourslash/goToImplementationInterfaceMethod_08.ts @@ -15,7 +15,7 @@ //// } //// //// class SubBar extends Bar { -//// hello() {} +//// [|hello() {}|] //// } diff --git a/tests/cases/fourslash/goToImplementationInterfaceMethod_09.ts b/tests/cases/fourslash/goToImplementationInterfaceMethod_09.ts index cb32f0203d0..922ea026888 100644 --- a/tests/cases/fourslash/goToImplementationInterfaceMethod_09.ts +++ b/tests/cases/fourslash/goToImplementationInterfaceMethod_09.ts @@ -15,6 +15,7 @@ //// //// whatever() { //// super.he/*function_call*/llo(); +//// super["hel/*element_access*/lo"](); //// } //// } //// @@ -28,3 +29,6 @@ goTo.marker("function_call"); verify.allRangesAppearInImplementationList(); + +goTo.marker("element_access"); +verify.allRangesAppearInImplementationList(); diff --git a/tests/cases/fourslash/goToImplementationNamespace_00.ts b/tests/cases/fourslash/goToImplementationNamespace_00.ts index edae50425bd..1f2be487e3f 100644 --- a/tests/cases/fourslash/goToImplementationNamespace_00.ts +++ b/tests/cases/fourslash/goToImplementationNamespace_00.ts @@ -1,12 +1,29 @@ /// -// Should handle calls on namespaces +// Should not return results on namespaces and modules -//// [|namespace Foo { -//// export function hello() {} -//// }|] -//// -//// let x = Fo/*reference*/o; +//// namespace Foo { +//// export function hello() {} +//// } +//// +//// namespace Foo.Bar { +//// export function okay() {} +//// } +//// +//// namespace Baz { +//// export function sure() {} +//// } +//// +//// namespace Baz.Bar { +//// export function alright() {} +//// } +//// +//// let w = Fo/*reference0*/o; +//// let x = Foo.B/*reference1*/ar; +//// let w = Ba/*reference2*/z; +//// let x = Baz.B/*reference3*/ar; -goTo.marker("reference"); -verify.allRangesAppearInImplementationList(); \ No newline at end of file +for (let i = 0; i < 4; i++) { + goTo.marker("reference" + i); + verify.implementationCountIs(0); +} \ No newline at end of file diff --git a/tests/cases/fourslash/goToImplementationNamespace_01.ts b/tests/cases/fourslash/goToImplementationNamespace_01.ts index 4d5b30efa0c..ea24336be7e 100644 --- a/tests/cases/fourslash/goToImplementationNamespace_01.ts +++ b/tests/cases/fourslash/goToImplementationNamespace_01.ts @@ -1,12 +1,12 @@ /// -// Should handle calls on modules +// Should handle property access expressions on namespaces -//// [|module Foo { -//// export function hello() {} -//// }|] +//// namespace Foo { +//// [|export function hello() {}|] +//// } //// -//// let x = Fo/*reference*/o; +//// Foo.hell/*reference*/o(); goTo.marker("reference"); verify.allRangesAppearInImplementationList(); \ No newline at end of file diff --git a/tests/cases/fourslash/goToImplementationNamespace_02.ts b/tests/cases/fourslash/goToImplementationNamespace_02.ts index ea24336be7e..c4388df7e8a 100644 --- a/tests/cases/fourslash/goToImplementationNamespace_02.ts +++ b/tests/cases/fourslash/goToImplementationNamespace_02.ts @@ -2,7 +2,7 @@ // Should handle property access expressions on namespaces -//// namespace Foo { +//// module Foo { //// [|export function hello() {}|] //// } //// diff --git a/tests/cases/fourslash/goToImplementationNamespace_03.ts b/tests/cases/fourslash/goToImplementationNamespace_03.ts index c4388df7e8a..38aeb47feae 100644 --- a/tests/cases/fourslash/goToImplementationNamespace_03.ts +++ b/tests/cases/fourslash/goToImplementationNamespace_03.ts @@ -1,12 +1,28 @@ /// -// Should handle property access expressions on namespaces +// Should handle types that are members of a namespace in type references and heritage clauses -//// module Foo { -//// [|export function hello() {}|] -//// } -//// -//// Foo.hell/*reference*/o(); +//// namespace Foo { +//// export interface Bar { +//// hello(): void; +//// } +//// +//// [|class BarImpl implements Bar { +//// hello() {} +//// }|] +//// } +//// +//// [|class Baz implements Foo.Bar { +//// hello() {} +//// }|] +//// +//// var someVar1 : Foo.Bar = [|{ hello: () => {/**1*/} }|]; +//// +//// var someVar2 = [|{ hello: () => {/**2*/} }|]; +//// +//// function whatever(x: Foo.Ba/*reference*/r) { +//// +//// } goTo.marker("reference"); -verify.allRangesAppearInImplementationList(); \ No newline at end of file +verify.allRangesAppearInImplementationList(); diff --git a/tests/cases/fourslash/goToImplementationNamespace_04.ts b/tests/cases/fourslash/goToImplementationNamespace_04.ts index 5f4267da530..fc64275d906 100644 --- a/tests/cases/fourslash/goToImplementationNamespace_04.ts +++ b/tests/cases/fourslash/goToImplementationNamespace_04.ts @@ -1,26 +1,28 @@ /// -// Should handle sub-namespaces +// Should handle types that are members of a module in type references and heritage clauses -//// /*parentNamespace*/namespace Foo { -//// export function hello() {} +//// module Foo { +//// export interface Bar { +//// hello(): void; +//// } +//// +//// [|class BarImpl implements Bar { +//// hello() {} +//// }|] //// } //// -//// /*parentNamespace2*/namespace Foo./*childNamespace*/Bar { -//// export function okay() {} -//// } +//// [|class Baz implements Foo.Bar { +//// hello() {} +//// }|] //// -//// Fo/*parentReference*/o.hello(); -//// Foo.Ba/*childReference*/r.okay(); +//// var someVar1 : Foo.Bar = [|{ hello: () => {/**1*/} }|]; +//// +//// var someVar2 = [|{ hello: () => {/**2*/} }|]; +//// +//// function whatever(x: Foo.Ba/*reference*/r) { +//// +//// } -goTo.marker("parentReference"); -goTo.implementation(0); -verify.caretAtMarker("parentNamespace"); - -goTo.marker("parentReference"); -goTo.implementation(1); -verify.caretAtMarker("parentNamespace2"); - -goTo.marker("childReference"); -goTo.implementation(); -verify.caretAtMarker("childNamespace") \ No newline at end of file +goTo.marker("reference"); +verify.allRangesAppearInImplementationList(); diff --git a/tests/cases/fourslash/goToImplementationNamespace_05.ts b/tests/cases/fourslash/goToImplementationNamespace_05.ts deleted file mode 100644 index 3dfb1fcca49..00000000000 --- a/tests/cases/fourslash/goToImplementationNamespace_05.ts +++ /dev/null @@ -1,26 +0,0 @@ -/// - -// Should handle sub-modules - -//// /*parentModule*/module Foo { -//// export function hello() {} -//// } -//// -//// /*parentModule2*/module Foo./*childModule*/Bar { -//// export function okay() {} -//// } -//// -//// Fo/*parentReference*/o.hello(); -//// Foo.Ba/*childReference*/r.okay(); - -goTo.marker("parentReference"); -goTo.implementation(0); -verify.caretAtMarker("parentModule"); - -goTo.marker("parentReference"); -goTo.implementation(1); -verify.caretAtMarker("parentModule2"); - -goTo.marker("childReference"); -goTo.implementation(); -verify.caretAtMarker("childModule") \ No newline at end of file diff --git a/tests/cases/fourslash/goToImplementationNamespace_06.ts b/tests/cases/fourslash/goToImplementationNamespace_06.ts deleted file mode 100644 index 38aeb47feae..00000000000 --- a/tests/cases/fourslash/goToImplementationNamespace_06.ts +++ /dev/null @@ -1,28 +0,0 @@ -/// - -// Should handle types that are members of a namespace in type references and heritage clauses - -//// namespace Foo { -//// export interface Bar { -//// hello(): void; -//// } -//// -//// [|class BarImpl implements Bar { -//// hello() {} -//// }|] -//// } -//// -//// [|class Baz implements Foo.Bar { -//// hello() {} -//// }|] -//// -//// var someVar1 : Foo.Bar = [|{ hello: () => {/**1*/} }|]; -//// -//// var someVar2 = [|{ hello: () => {/**2*/} }|]; -//// -//// function whatever(x: Foo.Ba/*reference*/r) { -//// -//// } - -goTo.marker("reference"); -verify.allRangesAppearInImplementationList(); diff --git a/tests/cases/fourslash/goToImplementationNamespace_07.ts b/tests/cases/fourslash/goToImplementationNamespace_07.ts deleted file mode 100644 index fc64275d906..00000000000 --- a/tests/cases/fourslash/goToImplementationNamespace_07.ts +++ /dev/null @@ -1,28 +0,0 @@ -/// - -// Should handle types that are members of a module in type references and heritage clauses - -//// module Foo { -//// export interface Bar { -//// hello(): void; -//// } -//// -//// [|class BarImpl implements Bar { -//// hello() {} -//// }|] -//// } -//// -//// [|class Baz implements Foo.Bar { -//// hello() {} -//// }|] -//// -//// var someVar1 : Foo.Bar = [|{ hello: () => {/**1*/} }|]; -//// -//// var someVar2 = [|{ hello: () => {/**2*/} }|]; -//// -//// function whatever(x: Foo.Ba/*reference*/r) { -//// -//// } - -goTo.marker("reference"); -verify.allRangesAppearInImplementationList();