Merge pull request #3367 from Microsoft/fixDeFaultOfFindAllRefsToMaster

Fix findAllRefs, getHighlightSpans, renameLocs, renameInfo for default exports and functions expressions

Author: DanielRosenwasser

src/harness/fourslash.ts

@@ -743,7 +743,7 @@ module FourSlash {
                 var reference = references[i];
                 if (reference && reference.fileName === fileName && reference.textSpan.start === start && ts.textSpanEnd(reference.textSpan) === end) {
                     if (typeof isWriteAccess !== "undefined" && reference.isWriteAccess !== isWriteAccess) {
-                        this.raiseError('verifyReferencesAtPositionListContains failed - item isWriteAccess value doe not match, actual: ' + reference.isWriteAccess + ', expected: ' + isWriteAccess + '.');
+                        this.raiseError('verifyReferencesAtPositionListContains failed - item isWriteAccess value does not match, actual: ' + reference.isWriteAccess + ', expected: ' + isWriteAccess + '.');
                     }
                     return;
                 }
@@ -884,8 +884,16 @@ module FourSlash {
                     this.activeFile.fileName, this.currentCaretPosition, findInStrings, findInComments);
 
                 var ranges = this.getRanges();
+
+                if (!references) {
+                    if (ranges.length !== 0) {
+                        this.raiseError(`Expected ${ranges.length} rename locations; got none.`);
+                    }
+                    return;
+                }
+
                 if (ranges.length !== references.length) {
-                    this.raiseError(this.assertionMessage("Rename locations", references.length, ranges.length));
+                    this.raiseError("Rename location count does not match result.\n\nExpected: " + JSON.stringify(ranges) + "\n\nActual:" + JSON.stringify(references));
                 }
 
                 ranges = ranges.sort((r1, r2) => r1.start - r2.start);
@@ -898,9 +906,7 @@ module FourSlash {
                     if (reference.textSpan.start !== range.start ||
                         ts.textSpanEnd(reference.textSpan) !== range.end) {
 
-                        this.raiseError(this.assertionMessage("Rename location",
-                            "[" + reference.textSpan.start + "," + ts.textSpanEnd(reference.textSpan) + ")",
-                            "[" + range.start + "," + range.end + ")"));
+                        this.raiseError("Rename location results do not match.\n\nExpected: " + JSON.stringify(ranges) + "\n\nActual:" + JSON.stringify(references));
                     }
                 }
             }

src/services/services.ts

@@ -4265,7 +4265,7 @@ namespace ts {
                     isLiteralNameOfPropertyDeclarationOrIndexAccess(node) ||
                     isNameOfExternalModuleImportOrDeclaration(node)) {
 
-                    let referencedSymbols = getReferencedSymbolsForNodes(node, sourceFilesToSearch, /*findInStrings:*/ false, /*findInComments:*/ false);
+                    let referencedSymbols = getReferencedSymbolsForNode(node, sourceFilesToSearch, /*findInStrings:*/ false, /*findInComments:*/ false);
                     return convertReferencedSymbols(referencedSymbols);
                 }
 
@@ -4917,10 +4917,10 @@ namespace ts {
             }
 
             Debug.assert(node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.NumericLiteral || node.kind === SyntaxKind.StringLiteral);
-            return getReferencedSymbolsForNodes(node, program.getSourceFiles(), findInStrings, findInComments);
+            return getReferencedSymbolsForNode(node, program.getSourceFiles(), findInStrings, findInComments);
         }
 
-        function getReferencedSymbolsForNodes(node: Node, sourceFiles: SourceFile[], findInStrings: boolean, findInComments: boolean): ReferencedSymbol[] {
+        function getReferencedSymbolsForNode(node: Node, sourceFiles: SourceFile[], findInStrings: boolean, findInComments: boolean): ReferencedSymbol[] {
             let typeChecker = program.getTypeChecker();
 
             // Labels
@@ -4955,7 +4955,7 @@ namespace ts {
 
             let declarations = symbol.declarations;
 
-            // The symbol was an internal symbol and does not have a declaration e.g.undefined symbol
+            // The symbol was an internal symbol and does not have a declaration e.g. undefined symbol
             if (!declarations || !declarations.length) {
                 return undefined;
             }
@@ -4965,8 +4965,9 @@ namespace ts {
             // Compute the meaning from the location and the symbol it references
             let searchMeaning = getIntersectingMeaningFromDeclarations(getMeaningFromLocation(node), declarations);
 
-            // Get the text to search for, we need to normalize it as external module names will have quote
-            let declaredName = getDeclaredName(symbol, node);
+            // Get the text to search for.
+            // Note: if this is an external module symbol, the name doesn't include quotes.
+            let declaredName = getDeclaredName(typeChecker, symbol, node);
 
             // Try to get the smallest valid scope that we can limit our search to;
             // otherwise we'll need to search globally (i.e. include each file).
@@ -5013,76 +5014,43 @@ namespace ts {
                 };
             }
 
-            function isImportOrExportSpecifierName(location: Node): boolean {
-                return location.parent &&
-                    (location.parent.kind === SyntaxKind.ImportSpecifier || location.parent.kind === SyntaxKind.ExportSpecifier) &&
-                    (<ImportOrExportSpecifier>location.parent).propertyName === location;
-            }
-
             function isImportOrExportSpecifierImportSymbol(symbol: Symbol) {
                 return (symbol.flags & SymbolFlags.Alias) && forEach(symbol.declarations, declaration => {
                     return declaration.kind === SyntaxKind.ImportSpecifier || declaration.kind === SyntaxKind.ExportSpecifier;
                 });
             }
 
-            function getDeclaredName(symbol: Symbol, location: Node) {
-                // Special case for function expressions, whose names are solely local to their bodies.
-                let functionExpression = forEach(symbol.declarations, d => d.kind === SyntaxKind.FunctionExpression ? <FunctionExpression>d : undefined);
-
-                // When a name gets interned into a SourceFile's 'identifiers' Map,
-                // its name is escaped and stored in the same way its symbol name/identifier
-                // name should be stored. Function expressions, however, are a special case,
-                // because despite sometimes having a name, the binder unconditionally binds them
-                // to a symbol with the name "__function".
-                let name: string;
-                if (functionExpression && functionExpression.name) {
-                    name = functionExpression.name.text;
-                }
-
-                // If this is an export or import specifier it could have been renamed using the as syntax.
-                // if so we want to search for whatever under the cursor, the symbol is pointing to the alias (name)
-                // so check for the propertyName.
-                if (isImportOrExportSpecifierName(location)) {
-                    return location.getText();
-                }
-
-                name = typeChecker.symbolToString(symbol);
-
-                return stripQuotes(name);
-            }
-
             function getInternedName(symbol: Symbol, location: Node, declarations: Declaration[]): string {
-                // If this is an export or import specifier it could have been renamed using the as syntax.
-                // if so we want to search for whatever under the cursor, the symbol is pointing to the alias (name)
-                // so check for the propertyName.
+                // If this is an export or import specifier it could have been renamed using the 'as' syntax.
+                // If so we want to search for whatever under the cursor.
                 if (isImportOrExportSpecifierName(location)) {
                     return location.getText();
                 }
 
-                // Special case for function expressions, whose names are solely local to their bodies.
-                let functionExpression = forEach(declarations, d => d.kind === SyntaxKind.FunctionExpression ? <FunctionExpression>d : undefined);
-
-                // When a name gets interned into a SourceFile's 'identifiers' Map,
-                // its name is escaped and stored in the same way its symbol name/identifier
-                // name should be stored. Function expressions, however, are a special case,
-                // because despite sometimes having a name, the binder unconditionally binds them
-                // to a symbol with the name "__function".
-                let name = functionExpression && functionExpression.name
-                    ? functionExpression.name.text
-                    : symbol.name;
-
-                return stripQuotes(name);
-            }
+                // Try to get the local symbol if we're dealing with an 'export default'
+                // since that symbol has the "true" name.
+                let localExportDefaultSymbol = getLocalSymbolForExportDefault(symbol);
+                symbol = localExportDefaultSymbol || symbol;
 
-            function stripQuotes(name: string) {
-                let length = name.length;
-                if (length >= 2 && name.charCodeAt(0) === CharacterCodes.doubleQuote && name.charCodeAt(length - 1) === CharacterCodes.doubleQuote) {
-                    return name.substring(1, length - 1);
-                };
-                return name;
+                return stripQuotes(symbol.name);
             }
 
+            /**
+             * Determines the smallest scope in which a symbol may have named references.
+             * Note that not every construct has been accounted for. This function can
+             * probably be improved.
+             *
+             * @returns undefined if the scope cannot be determined, implying that
+             * a reference to a symbol can occur anywhere.
+             */
             function getSymbolScope(symbol: Symbol): Node {
+                // If this is the symbol of a function expression, then named references
+                // are limited to its own scope.
+                let valueDeclaration = symbol.valueDeclaration;
+                if (valueDeclaration && valueDeclaration.kind === SyntaxKind.FunctionExpression) {
+                    return valueDeclaration;
+                }
+
                 // If this is private property or method, the scope is the containing class
                 if (symbol.flags & (SymbolFlags.Property | SymbolFlags.Method)) {
                     let privateDeclaration = forEach(symbol.getDeclarations(), d => (d.flags & NodeFlags.Private) ? d : undefined);
@@ -6724,12 +6692,13 @@ namespace ts {
                             }
                         }
 
+                        let displayName = getDeclaredName(typeChecker, symbol, node);
                         let kind = getSymbolKind(symbol, node);
                         if (kind) {
                             return {
                                 canRename: true,
                                 localizedErrorMessage: undefined,
-                                displayName: symbol.name,
+                                displayName,
                                 fullDisplayName: typeChecker.getFullyQualifiedName(symbol),
                                 kind: kind,
                                 kindModifiers: getSymbolModifiers(symbol),

src/services/utilities.ts

@@ -652,4 +652,34 @@ namespace ts {
             typechecker.getSymbolDisplayBuilder().buildSignatureDisplay(signature, writer, enclosingDeclaration, flags);
         });
     }
+
+    export function getDeclaredName(typeChecker: TypeChecker, symbol: Symbol, location: Node): string {
+        // If this is an export or import specifier it could have been renamed using the 'as' syntax.
+        // If so we want to search for whatever is under the cursor.
+        if (isImportOrExportSpecifierName(location)) {
+            return location.getText();
+        }
+
+        // Try to get the local symbol if we're dealing with an 'export default'
+        // since that symbol has the "true" name.
+        let localExportDefaultSymbol = getLocalSymbolForExportDefault(symbol);
+
+        let name = typeChecker.symbolToString(localExportDefaultSymbol || symbol);
+
+        return stripQuotes(name);
+    }
+
+    export function isImportOrExportSpecifierName(location: Node): boolean {
+        return location.parent &&
+            (location.parent.kind === SyntaxKind.ImportSpecifier || location.parent.kind === SyntaxKind.ExportSpecifier) &&
+            (<ImportOrExportSpecifier>location.parent).propertyName === location;
+    }
+
+    export function stripQuotes(name: string) {
+        let length = name.length;
+        if (length >= 2 && name.charCodeAt(0) === CharacterCodes.doubleQuote && name.charCodeAt(length - 1) === CharacterCodes.doubleQuote) {
+            return name.substring(1, length - 1);
+        };
+        return name;
+    }
 }

tests/cases/fourslash/findAllRefsForDefaultExport01.ts

@@ -0,0 +1,18 @@
+/// <reference path='fourslash.ts'/>
+
+////export default class [|DefaultExportedClass|] {
+////}
+////
+////var x: [|DefaultExportedClass|];
+////
+////var y = new [|DefaultExportedClass|];
+
+let ranges = test.ranges()
+for (let range of ranges) {
+    goTo.position(range.start);
+
+    verify.referencesCountIs(ranges.length);
+    for (let expectedReference of ranges) {
+        verify.referencesAtPositionContains(expectedReference);
+    }
+}

tests/cases/fourslash/findAllRefsForDefaultExport02.ts

@@ -0,0 +1,19 @@
+/// <reference path='fourslash.ts'/>
+
+////export default function [|DefaultExportedFunction|]() {
+////    return [|DefaultExportedFunction|]
+////}
+////
+////var x: typeof [|DefaultExportedFunction|];
+////
+////var y = [|DefaultExportedFunction|]();
+
+let ranges = test.ranges()
+for (let range of ranges) {
+    goTo.position(range.start);
+
+    verify.referencesCountIs(ranges.length);
+    for (let expectedReference of ranges) {
+        verify.referencesAtPositionContains(expectedReference);
+    }
+}

tests/cases/fourslash/findAllRefsForDefaultExport03.ts

@@ -0,0 +1,25 @@
+/// <reference path='fourslash.ts'/>
+
+////function [|f|]() {
+////    return 100;
+////}
+////
+////export default [|f|];
+////
+////var x: typeof [|f|];
+////
+////var y = [|f|]();
+////
+////namespace [|f|] {
+////    var local = 100;
+////}
+
+let ranges = test.ranges()
+for (let range of ranges) {
+    goTo.position(range.start);
+
+    verify.referencesCountIs(ranges.length);
+    for (let expectedReference of ranges) {
+        verify.referencesAtPositionContains(expectedReference);
+    }
+}

tests/cases/fourslash/findAllRefsForDefaultExport04.ts

@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts'/>
+
+////function f() {
+////    return 100;
+////}
+////
+////export default [|f|];
+////
+////var x: typeof f;
+////
+////var y = f();
+////
+////namespace /**/[|f|] {
+////}
+
+// The function 'f' and the namespace 'f' don't get merged,
+// but the 'export default' site, includes both meanings.
+
+// Here we are testing whether the 'export default'
+// site is included in the references to the namespace.
+
+goTo.marker();
+let ranges = test.ranges();
+verify.referencesCountIs(ranges.length);
+
+for (let expectedReference of ranges) {
+    verify.referencesAtPositionContains(expectedReference);
+}

tests/cases/fourslash/findAllRefsForDefaultExport05.ts

@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts'/>
+
+////function /**/[|f|]() {
+////    return 100;
+////}
+////
+////export default [|f|];
+////
+////var x: typeof [|f|];
+////
+////var y = [|f|]();
+////
+////namespace f {
+////}
+
+// The function 'f' and the namespace 'f' don't get merged,
+// but the 'export default' site, includes both meanings.
+
+// Here we are testing whether the 'export default' site
+// and all value-uses of 'f' are included in the references to the function.
+
+goTo.marker();
+let ranges = test.ranges();
+verify.referencesCountIs(ranges.length);
+
+for (let expectedReference of ranges) {
+    verify.referencesAtPositionContains(expectedReference);
+}

tests/cases/fourslash/findAllRefsForDefaultExport06.ts

@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts'/>
+
+////function [|f|]() {
+////    return 100;
+////}
+////
+////export default /**/[|f|];
+////
+////var x: typeof [|f|];
+////
+////var y = [|f|]();
+////
+////namespace [|f|] {
+////}
+
+// The function 'f' and the namespace 'f' don't get merged,
+// but the 'export default' site, includes both meanings.
+
+// Here we are testing whether the 'export default' site
+// and all value-uses of 'f' are included in the references to the function.
+
+goTo.marker();
+let ranges = test.ranges();
+verify.referencesCountIs(ranges.length);
+
+for (let expectedReference of ranges) {
+    verify.referencesAtPositionContains(expectedReference);
+}