From 402e2e618934c3b84555684915f87d3ee0e11d90 Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Fri, 15 Jan 2021 10:47:39 -0800
Subject: [PATCH] refactor(language-service): de-duplicate rename and reference
 results (#40454)

The initial implementation assumed that the consuming editors would
de-duplicate rename locations. In fact, vscode treats overlapping rename
locations as distinct and errors when trying to preview the renames.

This commit updates the language service to de-duplicate exact file+span
matches before returning rename and reference locations.

While vscode _does_ de-duplicate reference results, it still makes sense
to de-duplicate them on our side when possible to make tests more
understandable. If a template has 3 instances of a variable, it makes
sense to get get 3 reference results rather than 4+ with some duplicates.

PR Close #40454
---
 packages/language-service/ivy/references.ts   | 26 +++++++++-----
 .../ivy/test/references_spec.ts               | 34 ++++++++++++-------
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/packages/language-service/ivy/references.ts b/packages/language-service/ivy/references.ts
index 114385e5fd..ab92389894 100644
--- a/packages/language-service/ivy/references.ts
+++ b/packages/language-service/ivy/references.ts
@@ -166,7 +166,7 @@ export class ReferencesAndRenameBuilder {
       return undefined;
     }
 
-    const entries: ts.RenameLocation[] = [];
+    const entries: Map<string, ts.RenameLocation> = new Map();
     for (const location of locations) {
       // TODO(atscott): Determine if a file is a shim file in a more robust way and make the API
       // available in an appropriate location.
@@ -177,17 +177,17 @@ export class ReferencesAndRenameBuilder {
         if (entry === null) {
           return undefined;
         }
-        entries.push(entry);
+        entries.set(createLocationKey(entry), entry);
       } else {
         // Ensure we only allow renaming a TS result with matching text
         const refNode = this.getTsNodeAtPosition(location.fileName, location.textSpan.start);
         if (refNode === null || refNode.getText() !== originalNodeText) {
           return undefined;
         }
-        entries.push(location);
+        entries.set(createLocationKey(location), location);
       }
     }
-    return entries;
+    return Array.from(entries.values());
   }
 
   getReferencesAtPosition(filePath: string, position: number): ts.ReferenceEntry[]|undefined {
@@ -344,18 +344,18 @@ export class ReferencesAndRenameBuilder {
       return undefined;
     }
 
-    const entries: ts.ReferenceEntry[] = [];
+    const entries: Map<string, ts.ReferenceEntry> = new Map();
     for (const ref of refs) {
       if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) {
         const entry = this.convertToTemplateDocumentSpan(ref, this.ttc);
         if (entry !== null) {
-          entries.push(entry);
+          entries.set(createLocationKey(entry), entry);
         }
       } else {
-        entries.push(ref);
+        entries.set(createLocationKey(ref), ref);
       }
     }
-    return entries;
+    return Array.from(entries.values());
   }
 
   private convertToTemplateDocumentSpan<T extends ts.DocumentSpan>(
@@ -432,3 +432,13 @@ function getRenameTextAndSpanAtPosition(node: TmplAstNode|AST, position: number)
 
   return null;
 }
+
+
+/**
+ * Creates a "key" for a rename/reference location by concatenating file name, span start, and span
+ * length. This allows us to de-duplicate template results when an item may appear several times
+ * in the TCB but map back to the same template location.
+ */
+function createLocationKey(ds: ts.DocumentSpan) {
+  return ds.fileName + ds.textSpan.start + ds.textSpan.length;
+}
\ No newline at end of file
diff --git a/packages/language-service/ivy/test/references_spec.ts b/packages/language-service/ivy/test/references_spec.ts
index 39cb45be21..66959b221a 100644
--- a/packages/language-service/ivy/test/references_spec.ts
+++ b/packages/language-service/ivy/test/references_spec.ts
@@ -554,33 +554,43 @@ describe('find references and rename locations', () => {
       let cursor: number;
       beforeEach(() => {
         const cursorInfo = extractCursorInfo(`
+          <div *ngFor="let hero of heroes">
+            <span *ngIf="hero">
+              {{her¦o}}
+            </span>
+          </div>
+          `);
+        cursor = cursorInfo.cursor;
+        const appFile = {
+          name: _('/app.ts'),
+          contents: `
           import {Component} from '@angular/core';
 
-          @Component({template: '<div *ngFor="let hero of heroes">{{her¦o}}</div>'})
+          @Component({templateUrl: './template.ng.html'})
           export class AppCmp {
             heroes: string[] = [];
-          }`);
-        cursor = cursorInfo.cursor;
-        const appFile = {name: _('/app.ts'), contents: cursorInfo.text};
-        env = createModuleWithDeclarations([appFile]);
+          }`
+        };
+        const templateFile = {name: _('/template.ng.html'), contents: cursorInfo.text};
+        env = createModuleWithDeclarations([appFile], [templateFile]);
       });
 
       it('should find references', () => {
-        const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
-        expect(refs.length).toBe(2);
-        assertFileNames(refs, ['app.ts']);
+        const refs = getReferencesAtPosition(_('/template.ng.html'), cursor)!;
+        expect(refs.length).toBe(3);
+        assertFileNames(refs, ['template.ng.html']);
         assertTextSpans(refs, ['hero']);
 
-        const originalRefs = env.ngLS.getReferencesAtPosition(_('/app.ts'), cursor)!;
+        const originalRefs = env.ngLS.getReferencesAtPosition(_('/template.ng.html'), cursor)!;
         // Get the declaration by finding the reference that appears first in the template
         originalRefs.sort((a, b) => a.textSpan.start - b.textSpan.start);
         expect(originalRefs[0].isDefinition).toBe(true);
       });
 
       it('should find rename locations', () => {
-        const renameLocations = getRenameLocationsAtPosition(_('/app.ts'), cursor)!;
-        expect(renameLocations.length).toBe(2);
-        assertFileNames(renameLocations, ['app.ts']);
+        const renameLocations = getRenameLocationsAtPosition(_('/template.ng.html'), cursor)!;
+        expect(renameLocations.length).toBe(3);
+        assertFileNames(renameLocations, ['template.ng.html']);
         assertTextSpans(renameLocations, ['hero']);
       });
     });