From b64ead5cb8000abbef2555e666ddf038d16ec832 Mon Sep 17 00:00:00 2001
From: ayazhafiz <ayaz.hafiz.1@gmail.com>
Date: Mon, 27 Jan 2020 08:13:37 -0800
Subject: [PATCH] fix(language-service): prune duplicate returned definitions
 (#34995)

Sometimes, a request for definitions will return multiple of the same
definition. This can happen in at least the cases of

- two-way bindings (one of the same definition for the property and
  event binding)
- multiple template binding expressions in the same attribute
  - something like "*ngFor="let i of items; trackBy: test" has two
    template bindings, resulting in two template binding ASTs at the
    same location (the attribute span). The language service then parses
    both of these bindings individually, resulting in two independent
    but identical definitions. For more context, see https://github.com/angular/angular/pull/34847#discussion_r371006680.

This commit prunes duplicate definitions by signing definitions with
their location, and checking if that location signature has been seen in
a previous definition returned to the client.

PR Close #34995
---
 packages/language-service/src/definitions.ts  | 25 ++++++++++++++-----
 .../language-service/test/definitions_spec.ts | 21 +++++++---------
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/packages/language-service/src/definitions.ts b/packages/language-service/src/definitions.ts
index 51dbe1ce5d..17c14744d5 100644
--- a/packages/language-service/src/definitions.ts
+++ b/packages/language-service/src/definitions.ts
@@ -39,6 +39,7 @@ export function getDefinitionAndBoundSpan(
     return;
   }
 
+  const seen = new Set<string>();
   const definitions: ts.DefinitionInfo[] = [];
   for (const symbolInfo of symbols) {
     const {symbol} = symbolInfo;
@@ -53,16 +54,28 @@ export function getDefinitionAndBoundSpan(
     const containerKind =
         container ? container.kind as ts.ScriptElementKind : ts.ScriptElementKind.unknown;
     const containerName = container ? container.name : '';
-    definitions.push(...locations.map((location) => {
-      return {
+
+    for (const {fileName, span} of locations) {
+      const textSpan = ngSpanToTsTextSpan(span);
+      // In cases like two-way bindings, a request for the definitions of an expression may return
+      // two of the same definition:
+      //    [(ngModel)]="prop"
+      //                 ^^^^  -- one definition for the property binding, one for the event binding
+      // To prune duplicate definitions, tag definitions with unique location signatures and ignore
+      // definitions whose locations have already been seen.
+      const signature = `${textSpan.start}:${textSpan.length}@${fileName}`;
+      if (seen.has(signature)) continue;
+
+      definitions.push({
         kind: kind as ts.ScriptElementKind,
         name,
         containerKind,
         containerName,
-        textSpan: ngSpanToTsTextSpan(location.span),
-        fileName: location.fileName,
-      };
-    }));
+        textSpan: ngSpanToTsTextSpan(span),
+        fileName: fileName,
+      });
+      seen.add(signature);
+    }
   }
 
   return {
diff --git a/packages/language-service/test/definitions_spec.ts b/packages/language-service/test/definitions_spec.ts
index 9b0455dd68..6f989d03c9 100644
--- a/packages/language-service/test/definitions_spec.ts
+++ b/packages/language-service/test/definitions_spec.ts
@@ -60,19 +60,16 @@ describe('definitions', () => {
     expect(textSpan).toEqual(marker);
     expect(definitions).toBeDefined();
 
-    // There are exactly two, indentical definitions here, corresponding to the "name" on the
-    // property and event bindings of the two-way binding. The two-way binding is effectively
-    // syntactic sugar for `[ngModel]="name" (ngModel)="name=$event"`.
-    expect(definitions !.length).toBe(2);
-    for (const def of definitions !) {
-      expect(def.fileName).toBe(PARSING_CASES);
-      expect(def.name).toBe('title');
-      expect(def.kind).toBe('property');
+    expect(definitions !.length).toBe(1);
+    const def = definitions ![0];
 
-      const fileContent = mockHost.readFile(def.fileName);
-      expect(fileContent !.substring(def.textSpan.start, def.textSpan.start + def.textSpan.length))
-          .toEqual(`title = 'Some title';`);
-    }
+    expect(def.fileName).toBe(PARSING_CASES);
+    expect(def.name).toBe('title');
+    expect(def.kind).toBe('property');
+
+    const fileContent = mockHost.readFile(def.fileName);
+    expect(fileContent !.substring(def.textSpan.start, def.textSpan.start + def.textSpan.length))
+        .toEqual(`title = 'Some title';`);
   });
 
   it('should be able to find a method from a call', () => {