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).

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<T extends ts.DocumentSpan>(locations: readonly T[]): T[] {
+  const uniqueLocations: Map<string, T> = new Map();
+  for (const location of locations) {
+    uniqueLocations.set(createLocationKey(location), location);
+  }
+  return Array.from(uniqueLocations.values());
+}

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<string, ts.ReferenceEntry> = 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<string, ts.RenameLocation>}|null {
+    {expectedRenameText: string, entries: ts.RenameLocation[]}|null {
   let expectedRenameText: string;
-  const entries = new Map<string, ts.RenameLocation>();
+  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;

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, ['<div dir>', 'Dir', 'Dir2']);
         assertFileNames(refs, ['app.ts', 'dir.ts', 'dir2.ts']);
       });