From cf6c24cd02e2ac37e53f69d4d558a22811520795 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 27 Mar 2017 11:46:38 -0700 Subject: [PATCH] Respond to minor PR comments --- src/services/findAllReferences.ts | 33 +++++++++---------- src/services/importTracker.ts | 25 +++++++------- src/services/rename.ts | 14 +++++--- ...indAllRefsDefaultImportThroughNamespace.ts | 12 +++++-- 4 files changed, 48 insertions(+), 36 deletions(-) diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index a0c93a4b53d..581774ae219 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -520,17 +520,18 @@ namespace ts.FindAllReferences.Core { } if (indirectUsers.length) { - const indirectSearch = (() => { - switch (exportInfo.exportKind) { - case ExportKind.Named: - return state.createSearch(exportLocation, exportSymbol, ImportExport.Export); - case ExportKind.Default: - // Search for a property access to '.default'. This can't be renamed. - return state.isForRename ? undefined : state.createSearch(exportLocation, exportSymbol, ImportExport.Export, { text: "default" }); - case ExportKind.ExportEquals: - return undefined; - } - })(); + let indirectSearch: Search | undefined; + switch (exportInfo.exportKind) { + case ExportKind.Named: + indirectSearch = state.createSearch(exportLocation, exportSymbol, ImportExport.Export); + break; + case ExportKind.Default: + // Search for a property access to '.default'. This can't be renamed. + indirectSearch = state.isForRename ? undefined : state.createSearch(exportLocation, exportSymbol, ImportExport.Export, { text: "default" }); + break; + case ExportKind.ExportEquals: + break; + } if (indirectSearch) { for (const indirectUser of indirectUsers) { state.cancellationToken.throwIfCancellationRequested(); @@ -549,15 +550,11 @@ namespace ts.FindAllReferences.Core { /** Search for all occurences of an identifier in a source file (and filter out the ones that match). */ function searchForName(sourceFile: SourceFile, search: Search, state: State): void { - if (sourceFileHasName(sourceFile, search.escapedText)) { + if (getNameTable(sourceFile).get(search.escapedText) !== undefined) { getReferencesInSourceFile(sourceFile, search, state); } } - function sourceFileHasName(sourceFile: SourceFile, escapedName: string): boolean { - return getNameTable(sourceFile).get(escapedName) !== undefined; - } - function getPropertySymbolOfDestructuringAssignment(location: Node, checker: TypeChecker): Symbol | undefined { return isArrayLiteralOrObjectLiteralDestructuringPattern(location.parent.parent) && checker.getPropertySymbolOfDestructuringAssignment(location); @@ -613,7 +610,9 @@ namespace ts.FindAllReferences.Core { return undefined; } - // The only symbols with parents *not* globally acessible are exports of external modules, and only then if they do not have `export as namespace`. + // If the symbol has a parent, it's globally visible. + // Unless that parent is an external module, then we should only search in the module (and recurse on the export later). + // But if the parent is a module that has `export as namespace`, then the symbol *is* globally visible. if (parent && !((parent.flags & SymbolFlags.Module) && isExternalModuleSymbol(parent) && !parent.globalExports)) { return undefined; } diff --git a/src/services/importTracker.ts b/src/services/importTracker.ts index edbc8cf1a19..d9fe2cd19d3 100644 --- a/src/services/importTracker.ts +++ b/src/services/importTracker.ts @@ -36,7 +36,7 @@ namespace ts.FindAllReferences { type ImporterOrCallExpression = Importer | CallExpression; /** Returns import statements that directly reference the exporting module, and a list of files that may access the module through a namespace. */ - function getImportersForExport(sourceFiles: SourceFile[], allDirectImports: ImporterOrCallExpression[][], { exportingModuleSymbol, exportKind }: ExportInfo, checker: TypeChecker): { directImports: Importer[], indirectUsers: SourceFile[] } { + function getImportersForExport(sourceFiles: SourceFile[], allDirectImports: Map, { exportingModuleSymbol, exportKind }: ExportInfo, checker: TypeChecker): { directImports: Importer[], indirectUsers: SourceFile[] } { const markSeenDirectImport = nodeSeenTracker(); const markSeenIndirectUser = nodeSeenTracker(); const directImports: Importer[] = []; @@ -148,7 +148,7 @@ namespace ts.FindAllReferences { } function getDirectImports(moduleSymbol: Symbol): ImporterOrCallExpression[] | undefined { - return allDirectImports[getSymbolId(moduleSymbol)]; + return allDirectImports.get(getSymbolId(moduleSymbol).toString()); } } @@ -173,7 +173,7 @@ namespace ts.FindAllReferences { function handleImport(decl: Importer): void { if (decl.kind === SyntaxKind.ImportEqualsDeclaration) { - if (isProperImportEquals(decl)) { + if (isExternalModuleImportEquals(decl)) { handleNamespaceImportLike(decl.name); } return; @@ -274,17 +274,17 @@ namespace ts.FindAllReferences { } /** Returns a map from a module symbol Id to all import statements that directly reference the module. */ - function getDirectImportsMap(sourceFiles: SourceFile[], checker: TypeChecker): ImporterOrCallExpression[][] { - const map: ImporterOrCallExpression[][] = []; + function getDirectImportsMap(sourceFiles: SourceFile[], checker: TypeChecker): Map { + const map = createMap(); for (const sourceFile of sourceFiles) { forEachImport(sourceFile, (importDecl, moduleSpecifier) => { const moduleSymbol = checker.getSymbolAtLocation(moduleSpecifier); if (moduleSymbol) { - const id = getSymbolId(moduleSymbol); - let imports = map[id]; + const id = getSymbolId(moduleSymbol).toString(); + let imports = map.get(id); if (!imports) { - imports = map[id] = []; + map.set(id, imports = []); } imports.push(importDecl); } @@ -371,8 +371,7 @@ namespace ts.FindAllReferences { * @param comingFromExport If we are doing a search for all exports, don't bother looking backwards for the imported symbol, since that's the reason we're here. */ export function getImportOrExportSymbol(node: Node, symbol: Symbol, checker: TypeChecker, comingFromExport: boolean): ImportedSymbol | ExportedSymbol | undefined { - const ex = getExport(); - return ex || comingFromExport ? ex : getImport(); + return comingFromExport ? getExport() : getExport() || getImport(); function getExport(): ExportedSymbol | ImportedSymbol | undefined { const { parent } = node; @@ -459,7 +458,9 @@ namespace ts.FindAllReferences { const { parent } = node; switch (parent.kind) { case SyntaxKind.ImportEqualsDeclaration: - return (parent as ImportEqualsDeclaration).name === node ? { isNamedImport: false } : undefined; + return (parent as ImportEqualsDeclaration).name === node && isExternalModuleImportEquals(parent as ImportEqualsDeclaration) + ? { isNamedImport: false } + : undefined; case SyntaxKind.ImportSpecifier: // For a rename import `{ foo as bar }`, don't search for the imported symbol. Just find local uses of `bar`. return (parent as ImportSpecifier).propertyName ? undefined : { isNamedImport: true }; @@ -522,7 +523,7 @@ namespace ts.FindAllReferences { return node.kind === SyntaxKind.ModuleDeclaration && (node as ModuleDeclaration).name.kind === SyntaxKind.StringLiteral; } - function isProperImportEquals({ moduleReference }: ImportEqualsDeclaration): boolean { + function isExternalModuleImportEquals({ moduleReference }: ImportEqualsDeclaration): boolean { return moduleReference.kind === SyntaxKind.ExternalModuleReference && moduleReference.expression.kind === SyntaxKind.StringLiteral } } diff --git a/src/services/rename.ts b/src/services/rename.ts index bee530340bd..27c47179052 100644 --- a/src/services/rename.ts +++ b/src/services/rename.ts @@ -31,6 +31,13 @@ namespace ts.Rename { return getRenameInfoError(Diagnostics.You_cannot_rename_elements_that_are_defined_in_the_standard_TypeScript_library); } + // Cannot rename `default` as in `import { default as foo } from "./someModule"; + if (node.kind === SyntaxKind.Identifier && + (node as Identifier).originalKeywordKind === SyntaxKind.DefaultKeyword && + symbol.parent.flags & ts.SymbolFlags.Module) { + return undefined; + } + const displayName = stripQuotes(getDeclaredName(typeChecker, symbol, node)); const kind = SymbolDisplay.getSymbolKind(typeChecker, symbol, node); return kind ? getRenameInfoSuccess(displayName, typeChecker.getFullyQualifiedName(symbol), kind, SymbolDisplay.getSymbolModifiers(symbol), node, sourceFile) : undefined; @@ -82,11 +89,8 @@ namespace ts.Rename { } function nodeIsEligibleForRename(node: Node): boolean { - if (node.kind === SyntaxKind.Identifier) { - // Cannot rename `default` as in `import { default as foo } from "./someModule"; - return (node as Identifier).originalKeywordKind !== SyntaxKind.DefaultKeyword; - } - return node.kind === SyntaxKind.StringLiteral || + return node.kind === ts.SyntaxKind.Identifier || + node.kind === SyntaxKind.StringLiteral || isLiteralNameOfPropertyDeclarationOrIndexAccess(node) || isThis(node); } diff --git a/tests/cases/fourslash/findAllRefsDefaultImportThroughNamespace.ts b/tests/cases/fourslash/findAllRefsDefaultImportThroughNamespace.ts index b282af52ec1..e5f77631324 100644 --- a/tests/cases/fourslash/findAllRefsDefaultImportThroughNamespace.ts +++ b/tests/cases/fourslash/findAllRefsDefaultImportThroughNamespace.ts @@ -9,12 +9,20 @@ // @Filename: /c.ts ////import { a } from "./b"; ////a.[|default|](); +//// +////declare const x: { [|{| "isWriteAccess": true, "isDefinition": true |}default|]: number }; +////x.[|default|]; -verify.singleReferenceGroup("function f(): void"); +const [r0, r1, r2, r3] = test.ranges(); -const [r0, r1] = test.ranges(); +verify.singleReferenceGroup("function f(): void", [r0, r1]); +verify.singleReferenceGroup("(property) default: number", [r2, r3]); verify.rangesAreRenameLocations([r0]); +// Can't rename a default import. goTo.rangeStart(r1); verify.renameInfoFailed(); + +// Can rename a default property. +verify.rangesAreRenameLocations([r2, r3]);