From 1be5d659a51d10b05bee50bd5d7cd06070198c4c Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 21 Jan 2021 15:28:42 -0800 Subject: [PATCH] fix(language-service): fully de-duplicate reference and rename results (#40523) Rather than de-duplicating results as we build them, a final de-duplication can be done at the end. This way, there's no forgetting to de-duplicate results at some level. Prior to this commit, results from template locations that mapped to multiple different typescript locations would not be de-duplicated (e.g. an input binding that is bound to two separate directives). PR Close #40523 --- .../language-service/ivy/language_service.ts | 20 ++++++++++++++----- .../ivy/references_and_rename.ts | 20 +++++++++---------- .../ivy/test/references_and_rename_spec.ts | 10 +++------- 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/packages/language-service/ivy/language_service.ts b/packages/language-service/ivy/language_service.ts index 79f731f83a..320c98e7a5 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -27,6 +27,7 @@ import {CompletionBuilder, CompletionNodeContext} from './completions'; import {DefinitionBuilder} from './definitions'; import {QuickInfoBuilder} from './quick_info'; import {ReferencesBuilder, RenameBuilder} from './references_and_rename'; +import {createLocationKey} from './references_and_rename_utils'; import {getSignatureHelp} from './signature_help'; import {getTargetAtPosition, TargetContext, TargetNodeKind} from './template_target'; import {findTightestNode, getClassDeclFromDecoratorProp, getPropertyAssignmentFromValue} from './ts_utils'; @@ -168,8 +169,9 @@ export class LanguageService { getReferencesAtPosition(fileName: string, position: number): ts.ReferenceEntry[]|undefined { return this.withCompilerAndPerfTracing(PerfPhase.LsReferencesAndRenames, (compiler) => { - return new ReferencesBuilder(this.programDriver, this.tsLS, compiler) - .getReferencesAtPosition(fileName, position); + const results = new ReferencesBuilder(this.programDriver, this.tsLS, compiler) + .getReferencesAtPosition(fileName, position); + return results === undefined ? undefined : getUniqueLocations(results); }); } @@ -191,9 +193,9 @@ export class LanguageService { findRenameLocations(fileName: string, position: number): readonly ts.RenameLocation[]|undefined { return this.withCompilerAndPerfTracing(PerfPhase.LsReferencesAndRenames, (compiler) => { - return new RenameBuilder(this.programDriver, this.tsLS, compiler) - .findRenameLocations(fileName, position) ?? - undefined; + const results = new RenameBuilder(this.programDriver, this.tsLS, compiler) + .findRenameLocations(fileName, position); + return results === null ? undefined : getUniqueLocations(results); }); } @@ -565,3 +567,11 @@ function findTightestNodeAtPosition(program: ts.Program, fileName: string, posit return findTightestNode(sourceFile, position); } + +function getUniqueLocations(locations: readonly T[]): T[] { + const uniqueLocations: Map = new Map(); + for (const location of locations) { + uniqueLocations.set(createLocationKey(location), location); + } + return Array.from(uniqueLocations.values()); +} \ No newline at end of file diff --git a/packages/language-service/ivy/references_and_rename.ts b/packages/language-service/ivy/references_and_rename.ts index c99fe80157..d4e16baa2c 100644 --- a/packages/language-service/ivy/references_and_rename.ts +++ b/packages/language-service/ivy/references_and_rename.ts @@ -59,18 +59,18 @@ export class ReferencesBuilder { return undefined; } - const entries: Map = new Map(); + const entries: ts.ReferenceEntry[] = []; for (const ref of refs) { if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) { const entry = convertToTemplateDocumentSpan(ref, this.ttc, this.driver.getProgram()); if (entry !== null) { - entries.set(createLocationKey(entry), entry); + entries.push(entry); } } else { - entries.set(createLocationKey(ref), ref); + entries.push(ref); } } - return Array.from(entries.values()); + return entries; } } @@ -249,7 +249,7 @@ export class RenameBuilder { if (entry === null) { return null; } - entries.set(createLocationKey(entry), entry); + entries.push(entry); } else { if (!isDirectRenameContext(renameRequest)) { // Discard any non-template results for non-direct renames. We should only rename @@ -263,10 +263,10 @@ export class RenameBuilder { if (refNode === null || refNode.getText() !== expectedRenameText) { return null; } - entries.set(createLocationKey(location), location); + entries.push(location); } } - return Array.from(entries.values()); + return entries; }); } @@ -351,9 +351,9 @@ export class RenameBuilder { * required for the rename operation, but cannot be found by the native TS LS). */ function getExpectedRenameTextAndInitalRenameEntries(renameRequest: RenameRequest): - {expectedRenameText: string, entries: Map}|null { + {expectedRenameText: string, entries: ts.RenameLocation[]}|null { let expectedRenameText: string; - const entries = new Map(); + const entries: ts.RenameLocation[] = []; if (renameRequest.type === RequestKind.DirectFromTypeScript) { expectedRenameText = renameRequest.requestNode.getText(); } else if (renameRequest.type === RequestKind.DirectFromTemplate) { @@ -370,7 +370,7 @@ function getExpectedRenameTextAndInitalRenameEntries(renameRequest: RenameReques fileName: renameRequest.pipeNameExpr.getSourceFile().fileName, textSpan: {start: pipeNameExpr.getStart() + 1, length: pipeNameExpr.getText().length - 2}, }; - entries.set(createLocationKey(entry), entry); + entries.push(entry); } else { // TODO(atscott): Implement other types of special renames return null; diff --git a/packages/language-service/ivy/test/references_and_rename_spec.ts b/packages/language-service/ivy/test/references_and_rename_spec.ts index c12b9142cd..64f4d2be22 100644 --- a/packages/language-service/ivy/test/references_and_rename_spec.ts +++ b/packages/language-service/ivy/test/references_and_rename_spec.ts @@ -967,8 +967,7 @@ describe('find references and rename locations', () => { file.moveCursorToText('[mod¦el]'); }); - // TODO(atscott): Does not work because we don't fully de-duplicate - xit('should find references', () => { + it('should find references', () => { const refs = getReferencesAtPosition(file)!; expect(refs.length).toEqual(3); assertFileNames(refs, ['string-model.ts', 'app.ts', 'other-dir.ts']); @@ -1257,10 +1256,7 @@ describe('find references and rename locations', () => { file.moveCursorToText('[(mod¦el)]'); const refs = getReferencesAtPosition(file)!; - // Note that this includes the 'model` twice from the template. As with other potential - // duplicates (like if another plugin returns the same span), we expect the LS clients to filter - // these out themselves. - expect(refs.length).toEqual(4); + expect(refs.length).toEqual(3); assertFileNames(refs, ['dir.ts', 'app.ts']); assertTextSpans(refs, ['model', 'modelChange']); }); @@ -1347,7 +1343,7 @@ describe('find references and rename locations', () => { it('gets references to all matching directives', () => { const refs = getReferencesAtPosition(file)!; - expect(refs.length).toBe(8); + expect(refs.length).toBe(7); assertTextSpans(refs, ['
', 'Dir', 'Dir2']); assertFileNames(refs, ['app.ts', 'dir.ts', 'dir2.ts']); });