From 93839c33c33d23d6de6552a0e54a14813fff9c95 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 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). --- .../language-service/ivy/language_service.ts | 13 +++++++++-- .../ivy/references_and_rename.ts | 22 +++++++++---------- .../ivy/test/references_and_rename_spec.ts | 10 +++------ 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/packages/language-service/ivy/language_service.ts b/packages/language-service/ivy/language_service.ts index bab7cd30a79e1..17a2e5238bee0 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -19,6 +19,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 {getTargetAtPosition, TargetContext, TargetNodeKind} from './template_target'; import {getTemplateInfoAtPosition, isTypeScriptFile} from './utils'; @@ -110,7 +111,7 @@ export class LanguageService { const results = new ReferencesBuilder(this.strategy, this.tsLS, compiler) .getReferencesAtPosition(fileName, position); this.compilerFactory.registerLastKnownProgram(); - return results; + return results === undefined ? undefined : getUniqueLocations(results); } getRenameInfo(fileName: string, position: number): ts.RenameInfo { @@ -133,7 +134,7 @@ export class LanguageService { const results = new RenameBuilder(this.strategy, this.tsLS, compiler) .findRenameLocations(fileName, position); this.compilerFactory.registerLastKnownProgram(); - return results ?? undefined; + return results === null ? undefined : getUniqueLocations(results); } private getCompletionBuilder(fileName: string, position: number): @@ -322,3 +323,11 @@ function nodeContextFromTarget(target: TargetContext): CompletionNodeContext { return CompletionNodeContext.None; } } + +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 8b4fcda51369c..0e09370f6dd05 100644 --- a/packages/language-service/ivy/references_and_rename.ts +++ b/packages/language-service/ivy/references_and_rename.ts @@ -11,7 +11,7 @@ import {absoluteFrom} from '@angular/compiler-cli/src/ngtsc/file_system'; import {MetaType, PipeMeta} from '@angular/compiler-cli/src/ngtsc/metadata'; import {SymbolKind, TypeCheckingProgramStrategy} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; import * as ts from 'typescript'; -import {convertToTemplateDocumentSpan, createLocationKey, FilePosition, getParentClassMeta, getRenameTextAndSpanAtPosition, getTargetDetailsAtTemplatePosition, TemplateLocationDetails} from './references_and_rename_utils'; +import {convertToTemplateDocumentSpan, FilePosition, getParentClassMeta, getRenameTextAndSpanAtPosition, getTargetDetailsAtTemplatePosition, TemplateLocationDetails} from './references_and_rename_utils'; import {collectMemberMethods, findTightestNode} from './ts_utils'; import {getTemplateInfoAtPosition, TemplateInfo} from './utils'; @@ -57,18 +57,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.strategy.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; } } @@ -254,7 +254,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 @@ -268,10 +268,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; } private getTsNodeAtPosition(filePath: string, position: number): ts.Node|null { @@ -355,9 +355,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) { @@ -374,7 +374,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 6b4723683a039..0b33116b7f2e6 100644 --- a/packages/language-service/ivy/test/references_and_rename_spec.ts +++ b/packages/language-service/ivy/test/references_and_rename_spec.ts @@ -935,8 +935,7 @@ describe('find references and rename locations', () => { env = createModuleWithDeclarations([appFile, stringModelTestFile, otherDirFile]); }); - // TODO(atscott): Does not work because we don't fully de-duplicate - xit('should find references', () => { + it('should find references', () => { const refs = getReferencesAtPosition(_('/app.ts'), cursor)!; expect(refs.length).toEqual(3); assertFileNames(refs, ['string-model.ts', 'app.ts', 'other-dir.ts']); @@ -1215,10 +1214,7 @@ describe('find references and rename locations', () => { env = createModuleWithDeclarations([appFile, dirFile]); const refs = getReferencesAtPosition(_('/app.ts'), cursor)!; - // 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']); }); @@ -1304,7 +1300,7 @@ describe('find references and rename locations', () => { it('gets references to all matching directives', () => { const refs = getReferencesAtPosition(_('/app.ts'), cursor)!; - expect(refs.length).toBe(8); + expect(refs.length).toBe(7); assertTextSpans(refs, ['
', 'Dir', 'Dir2']); assertFileNames(refs, ['app.ts', 'dir.ts', 'dir2.ts']); });