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

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

@@ -59,18 +59,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.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<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) {
@@ -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;

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