Don't go past import in cross-project renaming (#48758)

* WIP * fix cross-project renaming logic * only use configure if prefix opt is defined * refactor skipAlias into stopAtAlias * fix stopAtAlias * update another stopAtAlias location
microsoft · May 3, 2022 · 8f56f6b · 8f56f6b
1 parent 3b8b207
commit 8f56f6b
Show file tree

Hide file tree

Showing 9 changed files with 90 additions and 18 deletions.
diff --git a/src/harness/client.ts b/src/harness/client.ts
@@ -38,6 +38,7 @@ namespace ts.server {
         private lineMaps = new Map<string, number[]>();
         private messages: string[] = [];
         private lastRenameEntry: RenameEntry | undefined;
+        private preferences: UserPreferences | undefined;
 
         constructor(private host: SessionClientHost) {
         }
@@ -124,6 +125,7 @@ namespace ts.server {
 
         /*@internal*/
         configure(preferences: UserPreferences) {
+            this.preferences = preferences;
             const args: protocol.ConfigureRequestArguments = { preferences };
             const request = this.processRequest(CommandNames.Configure, args);
             this.processResponse(request, /*expectEmptyBody*/ true);
@@ -488,13 +490,25 @@ namespace ts.server {
             return notImplemented();
         }
 
-        findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean): RenameLocation[] {
+        findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean, providePrefixAndSuffixTextForRename?: boolean): RenameLocation[] {
             if (!this.lastRenameEntry ||
                 this.lastRenameEntry.inputs.fileName !== fileName ||
                 this.lastRenameEntry.inputs.position !== position ||
                 this.lastRenameEntry.inputs.findInStrings !== findInStrings ||
                 this.lastRenameEntry.inputs.findInComments !== findInComments) {
-                this.getRenameInfo(fileName, position, { allowRenameOfImportPath: true }, findInStrings, findInComments);
+                if (providePrefixAndSuffixTextForRename !== undefined) {
+                    // User preferences have to be set through the `Configure` command
+                    this.configure({ providePrefixAndSuffixTextForRename });
+                    // Options argument is not used, so don't pass in options
+                    this.getRenameInfo(fileName, position, /*options*/{}, findInStrings, findInComments);
+                    // Restore previous user preferences
+                    if (this.preferences) {
+                        this.configure(this.preferences);
+                    }
+                }
+                else {
+                    this.getRenameInfo(fileName, position, /*options*/{}, findInStrings, findInComments);
+                }
             }
 
             return this.lastRenameEntry!.locations;

diff --git a/src/server/session.ts b/src/server/session.ts
@@ -317,6 +317,7 @@ namespace ts.server {
             projects,
             defaultProject,
             initialLocation,
+            /*isForRename*/ true,
             (project, location, tryAddToTodo) => {
                 const projectOutputs = project.getLanguageService().findRenameLocations(location.fileName, location.pos, findInStrings, findInComments, providePrefixAndSuffixTextForRename);
                 if (projectOutputs) {
@@ -332,8 +333,8 @@ namespace ts.server {
         return outputs;
     }
 
-    function getDefinitionLocation(defaultProject: Project, initialLocation: DocumentPosition): DocumentPosition | undefined {
-        const infos = defaultProject.getLanguageService().getDefinitionAtPosition(initialLocation.fileName, initialLocation.pos);
+    function getDefinitionLocation(defaultProject: Project, initialLocation: DocumentPosition, isForRename: boolean): DocumentPosition | undefined {
+        const infos = defaultProject.getLanguageService().getDefinitionAtPosition(initialLocation.fileName, initialLocation.pos, /*searchOtherFilesOnly*/ false, isForRename);
         const info = infos && firstOrUndefined(infos);
         return info && !info.isLocal ? { fileName: info.fileName, pos: info.textSpan.start } : undefined;
     }
@@ -350,6 +351,7 @@ namespace ts.server {
             projects,
             defaultProject,
             initialLocation,
+            /*isForRename*/ false,
             (project, location, getMappedLocation) => {
                 logger.info(`Finding references to ${location.fileName} position ${location.pos} in project ${project.getProjectName()}`);
                 const projectOutputs = project.getLanguageService().findReferences(location.fileName, location.pos);
@@ -417,7 +419,8 @@ namespace ts.server {
         projects: Projects,
         defaultProject: Project,
         initialLocation: TLocation,
-        cb: CombineProjectOutputCallback<TLocation>
+        isForRename: boolean,
+        cb: CombineProjectOutputCallback<TLocation>,
     ): void {
         const projectService = defaultProject.projectService;
         let toDo: ProjectAndLocation<TLocation>[] | undefined;
@@ -430,7 +433,7 @@ namespace ts.server {
 
         // After initial references are collected, go over every other project and see if it has a reference for the symbol definition.
         if (initialLocation) {
-            const defaultDefinition = getDefinitionLocation(defaultProject, initialLocation);
+            const defaultDefinition = getDefinitionLocation(defaultProject, initialLocation, isForRename);
             if (defaultDefinition) {
                 const getGeneratedDefinition = memoize(() => defaultProject.isSourceOfProjectReferenceRedirect(defaultDefinition.fileName) ?
                     defaultDefinition :
@@ -1243,7 +1246,7 @@ namespace ts.server {
                 definitions?.forEach(d => definitionSet.add(d));
                 const noDtsProject = project.getNoDtsResolutionProject([file]);
                 const ls = noDtsProject.getLanguageService();
-                const jsDefinitions = ls.getDefinitionAtPosition(file, position, /*searchOtherFilesOnly*/ true)
+                const jsDefinitions = ls.getDefinitionAtPosition(file, position, /*searchOtherFilesOnly*/ true, /*stopAtAlias*/ false)
                     ?.filter(d => toNormalizedPath(d.fileName) !== file);
                 if (some(jsDefinitions)) {
                     for (const jsDefinition of jsDefinitions) {
@@ -1330,7 +1333,7 @@ namespace ts.server {
                 if ((isStringLiteralLike(initialNode) || isIdentifier(initialNode)) && isAccessExpression(initialNode.parent)) {
                     return forEachNameInAccessChainWalkingLeft(initialNode, nameInChain => {
                         if (nameInChain === initialNode) return undefined;
-                        const candidates = ls.getDefinitionAtPosition(file, nameInChain.getStart(), /*searchOtherFilesOnly*/ true)
+                        const candidates = ls.getDefinitionAtPosition(file, nameInChain.getStart(), /*searchOtherFilesOnly*/ true, /*stopAtAlias*/ false)
                             ?.filter(d => toNormalizedPath(d.fileName) !== file && d.isAmbient)
                             .map(d => ({
                                 fileName: d.fileName,

diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts
@@ -1,6 +1,6 @@
 /* @internal */
 namespace ts.GoToDefinition {
-    export function getDefinitionAtPosition(program: Program, sourceFile: SourceFile, position: number, searchOtherFilesOnly?: boolean): readonly DefinitionInfo[] | undefined {
+    export function getDefinitionAtPosition(program: Program, sourceFile: SourceFile, position: number, searchOtherFilesOnly?: boolean, stopAtAlias?: boolean): readonly DefinitionInfo[] | undefined {
         const resolvedRef = getReferenceAtPosition(sourceFile, position, program);
         const fileReferenceDefinition = resolvedRef && [getDefinitionInfoForFileReference(resolvedRef.reference.fileName, resolvedRef.fileName, resolvedRef.unverified)] || emptyArray;
         if (resolvedRef?.file) {
@@ -28,7 +28,7 @@ namespace ts.GoToDefinition {
 
         if (isStaticModifier(node) && isClassStaticBlockDeclaration(node.parent)) {
             const classDecl = node.parent.parent;
-            const { symbol, failedAliasResolution } = getSymbol(classDecl, typeChecker);
+            const { symbol, failedAliasResolution } = getSymbol(classDecl, typeChecker, stopAtAlias);
 
             const staticBlocks = filter(classDecl.members, isClassStaticBlockDeclaration);
             const containerName = symbol ? typeChecker.symbolToString(symbol, classDecl) : "";
@@ -40,15 +40,15 @@ namespace ts.GoToDefinition {
             });
         }
 
-        let { symbol, failedAliasResolution } = getSymbol(node, typeChecker);
+        let { symbol, failedAliasResolution } = getSymbol(node, typeChecker, stopAtAlias);
         let fallbackNode = node;
 
         if (searchOtherFilesOnly && failedAliasResolution) {
             // We couldn't resolve the specific import, try on the module specifier.
             const importDeclaration = forEach([node, ...symbol?.declarations || emptyArray], n => findAncestor(n, isAnyImportOrBareOrAccessedRequire));
             const moduleSpecifier = importDeclaration && tryGetModuleSpecifierFromDeclaration(importDeclaration);
             if (moduleSpecifier) {
-                ({ symbol, failedAliasResolution } = getSymbol(moduleSpecifier, typeChecker));
+                ({ symbol, failedAliasResolution } = getSymbol(moduleSpecifier, typeChecker, stopAtAlias));
                 fallbackNode = moduleSpecifier;
             }
         }
@@ -235,7 +235,7 @@ namespace ts.GoToDefinition {
             return definitionFromType(typeChecker.getTypeAtLocation(node.parent), typeChecker, node.parent, /*failedAliasResolution*/ false);
         }
 
-        const { symbol, failedAliasResolution } = getSymbol(node, typeChecker);
+        const { symbol, failedAliasResolution } = getSymbol(node, typeChecker, /*stopAtAlias*/ false);
         if (!symbol) return undefined;
 
         const typeAtLocation = typeChecker.getTypeOfSymbolAtLocation(symbol, node);
@@ -292,14 +292,14 @@ namespace ts.GoToDefinition {
         return mapDefined(checker.getIndexInfosAtLocation(node), info => info.declaration && createDefinitionFromSignatureDeclaration(checker, info.declaration));
     }
 
-    function getSymbol(node: Node, checker: TypeChecker) {
+    function getSymbol(node: Node, checker: TypeChecker, stopAtAlias: boolean | undefined) {
         const symbol = checker.getSymbolAtLocation(node);
         // If this is an alias, and the request came at the declaration location
         // get the aliased symbol instead. This allows for goto def on an import e.g.
         //   import {A, B} from "mod";
         // to jump to the implementation directly.
         let failedAliasResolution = false;
-        if (symbol?.declarations && symbol.flags & SymbolFlags.Alias && shouldSkipAlias(node, symbol.declarations[0])) {
+        if (symbol?.declarations && symbol.flags & SymbolFlags.Alias && !stopAtAlias && shouldSkipAlias(node, symbol.declarations[0])) {
             const aliased = checker.getAliasedSymbol(symbol);
             if (aliased.declarations) {
                 return { symbol: aliased };

diff --git a/src/services/services.ts b/src/services/services.ts
@@ -1768,9 +1768,9 @@ namespace ts {
         }
 
         /// Goto definition
-        function getDefinitionAtPosition(fileName: string, position: number, searchOtherFilesOnly?: boolean): readonly DefinitionInfo[] | undefined {
+        function getDefinitionAtPosition(fileName: string, position: number, searchOtherFilesOnly?: boolean, stopAtAlias?: boolean): readonly DefinitionInfo[] | undefined {
             synchronizeHostData();
-            return GoToDefinition.getDefinitionAtPosition(program, getValidSourceFile(fileName), position, searchOtherFilesOnly);
+            return GoToDefinition.getDefinitionAtPosition(program, getValidSourceFile(fileName), position, searchOtherFilesOnly, stopAtAlias);
         }
 
         function getDefinitionAndBoundSpan(fileName: string, position: number): DefinitionInfoAndBoundSpan | undefined {

diff --git a/src/services/types.ts b/src/services/types.ts
@@ -474,7 +474,10 @@ namespace ts {
 
         /*@internal*/
         // eslint-disable-next-line @typescript-eslint/unified-signatures
-        getDefinitionAtPosition(fileName: string, position: number, searchOtherFilesOnly: boolean): readonly DefinitionInfo[] | undefined;
+        getDefinitionAtPosition(fileName: string, position: number, searchOtherFilesOnly: false, stopAtAlias: boolean): readonly DefinitionInfo[] | undefined;
+        /*@internal*/
+        // eslint-disable-next-line @typescript-eslint/unified-signatures
+        getDefinitionAtPosition(fileName: string, position: number, searchOtherFilesOnly: boolean, stopAtAlias: false): readonly DefinitionInfo[] | undefined;
         getDefinitionAtPosition(fileName: string, position: number): readonly DefinitionInfo[] | undefined;
         getDefinitionAndBoundSpan(fileName: string, position: number): DefinitionInfoAndBoundSpan | undefined;
         getTypeDefinitionAtPosition(fileName: string, position: number): readonly DefinitionInfo[] | undefined;

diff --git a/tests/baselines/reference/renameNamedImport.baseline b/tests/baselines/reference/renameNamedImport.baseline
@@ -0,0 +1,4 @@
+/*====== /src/index.ts ======*/
+
+import { [|RENAME|] } from '../lib/index';
+RENAME;
diff --git a/tests/baselines/reference/renameNamespaceImport.baseline b/tests/baselines/reference/renameNamespaceImport.baseline
@@ -0,0 +1,4 @@
+/*====== /src/index.ts ======*/
+
+import * as [|RENAME|] from '../lib/index';
+RENAME.someExportedVariable;
diff --git a/tests/cases/fourslash/server/renameNamedImport.ts b/tests/cases/fourslash/server/renameNamedImport.ts
@@ -0,0 +1,22 @@
+/// <reference path='../fourslash.ts' />
+
+// @Filename: /lib/tsconfig.json
+//// {}
+
+// @Filename: /lib/index.ts
+//// const unrelatedLocalVariable = 123;
+//// export const someExportedVariable = unrelatedLocalVariable;
+
+// @Filename: /src/tsconfig.json
+//// {}
+
+// @Filename: /src/index.ts
+//// import { /*i*/someExportedVariable } from '../lib/index';
+//// someExportedVariable;
+
+// @Filename: /tsconfig.json
+//// {}
+
+goTo.file("/lib/index.ts");
+goTo.file("/src/index.ts");
+verify.baselineRename("i", { providePrefixAndSuffixTextForRename: true });
diff --git a/tests/cases/fourslash/server/renameNamespaceImport.ts b/tests/cases/fourslash/server/renameNamespaceImport.ts
@@ -0,0 +1,22 @@
+/// <reference path='../fourslash.ts' />
+
+// @Filename: /lib/tsconfig.json
+//// {}
+
+// @Filename: /lib/index.ts
+//// const unrelatedLocalVariable = 123;
+//// export const someExportedVariable = unrelatedLocalVariable;
+
+// @Filename: /src/tsconfig.json
+//// {}
+
+// @Filename: /src/index.ts
+//// import * as /*i*/lib from '../lib/index';
+//// lib.someExportedVariable;
+
+// @Filename: /tsconfig.json
+//// {}
+
+goTo.file("/lib/index.ts");
+goTo.file("/src/index.ts");
+verify.baselineRename("i", {});