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

gabritto · 2022-04-18T23:45:41Z

The bug seemed to be caused by the cross-project part of "find all references": in session.ts, we try to find references in each project in the solution, other than the original project where the request was made. To accomplish this, we call getDefinitionAtLocation, and then try to find references to the resulting definition in other projects.
This meant that, in a situation like:

// lib/lib.ts
const unrelatedLocalVariable = 123;
export const someExportedVariable = unrelatedLocalVariable;

// src/index.ts
import * as lib from '../lib/lib.js';

when we tried to rename lib in import * as lib from ..., we found the original definition (which points to the module/file lib/lib.ts), and then tried to rename that.

The main thing about the bug, in my perspective, is that we should not try to rename the original definition and references to it, and should instead just rename the alias, in this case the namespace import itself (lib).
This PR accomplishes that by having getDefinitionAtLocation stop at aliases, instead of skipping them. The idea is that now we will find cross-project references to the alias that is being renamed, not to the original thing that was aliased.

P.S.: the other thing that seemed weird to me is that, when we try to get the definition of the lib namespace in import * as lib ..., we get a reference to the lib/lib.ts file, at position 0 (references are represented as a file + position). This PR doesn't fix that, so when invoking find all references on import * as |lib| ..., we still get one entry pointing to the lib file at position 0. But I thought that was a minor problem and something to be fixed in a different PR, if that's the case.

tests/cases/fourslash/server/renameNamedImport.ts

gabritto · 2022-04-19T19:08:59Z

src/services/types.ts

@@ -474,7 +474,7 @@ 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: boolean, skipAlias: boolean): readonly DefinitionInfo[] | undefined;


I'm not sure this is the best way to add this new parameter, open to feedback on this

Small nit but I think I would make it stopAtAlias so you can just make it optional instead of defaulting to true. But wait until the end of the PR review to do this, because the implication of my other feedback may be that not all aliases should be stopped at / skipped.

gabritto · 2022-04-19T19:10:39Z

src/server/session.ts

@@ -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);


that's where the logic changed: if we're renaming, we don't skip past aliases when getting the definition, because we want to rename the alias itself (I think), and not the original thing (e.g. we don't want to rename the original variable, just its import)

gabritto · 2022-04-19T20:04:44Z

src/services/goToDefinition.ts

        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, skipAlias));


@andrewbranch I'd like you to take a look at this case and the interaction between searchOtherFilesOnly and the new skipAlias

skipAlias should always be true when serachOtherFilesOnly is true.

tests/cases/fourslash/server/renameNamedImport.ts

tests/cases/fourslash/server/renameNamespaceImport.ts

amcasey · 2022-04-20T17:02:38Z

I wish I'd known you were working on the rename logic in session.ts because you probably want to build on top of #48619. It almost certainly doesn't address this problem, and may even make it worse, but it should be much easier to reason about and fix.

gabritto · 2022-04-20T17:29:47Z

I wish I'd known you were working on the rename logic in session.ts because you probably want to build on top of #48619. It almost certainly doesn't address this problem, and may even make it worse, but it should be much easier to reason about and fix.

Does that mean I should wait for #48619 to be merged and reimplement this on top of that?

amcasey · 2022-04-20T17:48:42Z

I wish I'd known you were working on the rename logic in session.ts because you probably want to build on top of #48619. It almost certainly doesn't address this problem, and may even make it worse, but it should be much easier to reason about and fix.

Does that mean I should wait for #48619 to be merged and reimplement this on top of that?

I was tentatively planning to hold #48619 until 4.8 since it's a bit risky, but I don't think that's essential. If your change needs to get in for 4.7, I don't mind merging it into my branch later - I just got the impression that there were a lot of open questions and I thought I'd jump in and suggest that any rewrite happen on top for #48619.

Thoughts on sequencing, @andrewbranch?

andrewbranch · 2022-04-21T21:00:59Z

I suspect this can go in for 4.7, and the I’m guessing the merge conflict will likely just be some of the calls you passed the new parameter to in session.ts get deleted altogether. I’ll plan to take a close look at this tomorrow. I want to investigate why shouldSkipAlias in goToDefinition.ts wasn’t already working here, as I think it’s supposed to handle exactly this case. I’m guessing it’s due to the complexities of triggering in another project, but that’s so confusing to me that I want to understand it before approving.

andrewbranch · 2022-04-22T23:37:17Z

I want to investigate why shouldSkipAlias in goToDefinition.ts wasn’t already working here, as I think it’s supposed to handle exactly this case.

Ah ok, what I had missed was that the rename location was actually on the namespace import name itself. Go To Definition has the behavior where it will stop at a NamespaceImport but only if you don’t start on the NamespaceImport (that way you can do one jump from a reference to the import, and then another jump to the module it refers to).

andrewbranch

I think this is pretty much good—I was confused by the baseline format not showing the prefixText/suffixText. It would be great if we could make renameNamedImport.baseline show up as

/*====== /src/index.ts ======*/

import { [|someExportedVariable as RENAME|] } from '../lib/index';
RENAME;

I also still think I would recommend naming the new go-to-def parameter stopAtAlias or just keep it as isForRename since it’s internal.

* Clean up FAR and RenameLocations This change had two goals: 1. Make the code easier to understand, primarily by simplifying the callback structure and minimizing side-effects 2. Improve performance by reducing repeated work, both FAR searches of individual projects and default tsconfig searches This implementation attempts to preserve the merging order found in the original code (someone less relevant in the present state of using syntactic isDefinition). * Stop enforcing search and aggregation order ...in preparation for implementing isDefinition explicitly. Also restore convention of referring to `DocumentPosition`s as "locations". * Introduce LanguageService.updateIsDefinitionOfReferencedSymbols ...to allow use of the checker when computing isDefinition across projects. * Update baselines * Tidy diff * De-dup simplified results * Baseline cross-project isDefinition results * Move de-duping upstream to fix Full output * Add server baseline test to confirm searches are not repeated * Manually merge #48758 * Update baseline for newer fix to #48963

gabritto added 4 commits February 14, 2022 14:51

WIP

9acc88f

Merge branch 'main' into gabritto/issue45659

6804b7a

fix cross-project renaming logic

3242950

only use configure if prefix opt is defined

36af929

typescript-bot assigned gabritto Apr 18, 2022

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 18, 2022

Merge branch 'main' into gabritto/issue45659

e0343f5

gabritto commented Apr 19, 2022

View reviewed changes

tests/cases/fourslash/server/renameNamedImport.ts Show resolved Hide resolved

gabritto commented Apr 19, 2022

View reviewed changes

gabritto requested review from andrewbranch and sheetalkamat April 19, 2022 20:00

gabritto commented Apr 19, 2022

View reviewed changes

andrewbranch requested changes Apr 19, 2022

View reviewed changes

tests/cases/fourslash/server/renameNamedImport.ts Show resolved Hide resolved

tests/cases/fourslash/server/renameNamespaceImport.ts Show resolved Hide resolved

andrewbranch reviewed May 2, 2022

View reviewed changes

gabritto added 3 commits May 2, 2022 15:26

refactor skipAlias into stopAtAlias

17ee2e1

fix stopAtAlias

a6f2e0f

update another stopAtAlias location

2d01675

gabritto requested a review from andrewbranch May 2, 2022 22:35

andrewbranch approved these changes May 2, 2022

View reviewed changes

gabritto merged commit 8f56f6b into main May 3, 2022

gabritto deleted the gabritto/issue45659 branch May 3, 2022 14:32

esdmr mentioned this pull request May 3, 2022

useAliasesForRenames does not prevent rename in the original file in another project #45734

Closed

amcasey added a commit to amcasey/TypeScript that referenced this pull request May 17, 2022

Manually merge microsoft#48758

d47ae8a

amcasey mentioned this pull request May 17, 2022

Clean up FAR aggregation #48619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gabritto commented Apr 18, 2022 •

edited

Loading

gabritto Apr 19, 2022

andrewbranch Apr 19, 2022

gabritto Apr 19, 2022

gabritto Apr 19, 2022

andrewbranch May 2, 2022

amcasey commented Apr 20, 2022

gabritto commented Apr 20, 2022

amcasey commented Apr 20, 2022

andrewbranch commented Apr 21, 2022

andrewbranch commented Apr 22, 2022

andrewbranch left a comment

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

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

Conversation

gabritto commented Apr 18, 2022 • edited Loading

gabritto Apr 19, 2022

Choose a reason for hiding this comment

andrewbranch Apr 19, 2022

Choose a reason for hiding this comment

gabritto Apr 19, 2022

Choose a reason for hiding this comment

gabritto Apr 19, 2022

Choose a reason for hiding this comment

andrewbranch May 2, 2022

Choose a reason for hiding this comment

amcasey commented Apr 20, 2022

gabritto commented Apr 20, 2022

amcasey commented Apr 20, 2022

andrewbranch commented Apr 21, 2022

andrewbranch commented Apr 22, 2022

andrewbranch left a comment

Choose a reason for hiding this comment

gabritto commented Apr 18, 2022 •

edited

Loading