From 4c8766573d306dbd948cb513b769d581723a4d97 Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Wed, 23 Sep 2020 08:36:38 -0700
Subject: [PATCH] refactor(language-service): Update hybrid visitor to use
 keySpan for bound attributes (#38955)

The keySpan in bound attributes provides more fine-grained location information and can be used
to disambiguate multiple bound attributes in a single microsyntax binding. Previously,
this case could not distinguish between the two different attributes because
the sourceSpans were identical and valueSpans would not match if the cursor
was located in a key.

PR Close #38955
---
 .../language-service/ivy/hybrid_visitor.ts    | 37 +++++++++++++++----
 .../ivy/test/hybrid_visitor_spec.ts           | 10 +++++
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/packages/language-service/ivy/hybrid_visitor.ts b/packages/language-service/ivy/hybrid_visitor.ts
index 234649533b..2f94eea3b4 100644
--- a/packages/language-service/ivy/hybrid_visitor.ts
+++ b/packages/language-service/ivy/hybrid_visitor.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ParseSourceSpan} from '@angular/compiler';
+import {AbsoluteSourceSpan, ParseSourceSpan} from '@angular/compiler';
 import * as e from '@angular/compiler/src/expression_parser/ast';  // e for expression AST
 import * as t from '@angular/compiler/src/render3/r3_ast';         // t for template AST
 
@@ -31,10 +31,13 @@ class R3Visitor implements t.Visitor {
   constructor(private readonly position: number) {}
 
   visit(node: t.Node) {
+    if (node instanceof t.BoundAttribute) {
+      node.visit(this);
+      return;
+    }
+
     const {start, end} = getSpanIncludingEndTag(node);
-    // Note both start and end are inclusive because we want to match conditions
-    // like ¦start and end¦ where ¦ is the cursor.
-    if (start <= this.position && this.position <= end) {
+    if (isWithin(this.position, {start, end})) {
       const length = end - start;
       const last: t.Node|e.AST|undefined = this.path[this.path.length - 1];
       if (last) {
@@ -97,8 +100,13 @@ class R3Visitor implements t.Visitor {
   }
 
   visitBoundAttribute(attribute: t.BoundAttribute) {
-    const visitor = new ExpressionVisitor(this.position);
-    visitor.visit(attribute.value, this.path);
+    if (isWithin(this.position, attribute.keySpan)) {
+      this.path.push(attribute);
+    } else if (attribute.valueSpan && isWithin(this.position, attribute.valueSpan)) {
+      this.path.push(attribute);
+      const visitor = new ExpressionVisitor(this.position);
+      visitor.visit(attribute.value, this.path);
+    }
   }
 
   visitBoundEvent(attribute: t.BoundEvent) {
@@ -144,10 +152,9 @@ class ExpressionVisitor extends e.RecursiveAstVisitor {
       // `ASTWithSource` and and underlying node that it wraps.
       node = node.ast;
     }
-    const {start, end} = node.sourceSpan;
     // The third condition is to account for the implicit receiver, which should
     // not be visited.
-    if (start <= this.position && this.position <= end && !(node instanceof e.ImplicitReceiver)) {
+    if (isWithin(this.position, node.sourceSpan) && !(node instanceof e.ImplicitReceiver)) {
       path.push(node);
       node.visit(this, path);
     }
@@ -178,3 +185,17 @@ function getSpanIncludingEndTag(ast: t.Node) {
   }
   return result;
 }
+
+function isWithin(position: number, span: AbsoluteSourceSpan|ParseSourceSpan): boolean {
+  let start: number, end: number;
+  if (span instanceof ParseSourceSpan) {
+    start = span.start.offset;
+    end = span.end.offset;
+  } else {
+    start = span.start;
+    end = span.end;
+  }
+  // Note both start and end are inclusive because we want to match conditions
+  // like ¦start and end¦ where ¦ is the cursor.
+  return start <= position && position <= end;
+}
diff --git a/packages/language-service/ivy/test/hybrid_visitor_spec.ts b/packages/language-service/ivy/test/hybrid_visitor_spec.ts
index 8a9d06fc6e..2bc2a1778e 100644
--- a/packages/language-service/ivy/test/hybrid_visitor_spec.ts
+++ b/packages/language-service/ivy/test/hybrid_visitor_spec.ts
@@ -518,6 +518,16 @@ describe('findNodeAtPosition for microsyntax expression', () => {
     expect((node as t.BoundAttribute).name).toBe('ngForOf');
   });
 
+  it('should locate bound attribute key for trackBy', () => {
+    const {errors, nodes, position} =
+        parse(`<div *ngFor="let item of items; trac¦kBy: trackByFn"></div>`);
+    expect(errors).toBe(null);
+    const node = findNodeAtPosition(nodes, position);
+    expect(isTemplateNode(node!)).toBe(true);
+    expect(node).toBeInstanceOf(t.BoundAttribute);
+    expect((node as t.BoundAttribute).name).toBe('ngForTrackBy');
+  });
+
   it('should locate bound attribute value', () => {
     const {errors, nodes, position} = parse(`<div *ngFor="let item of it¦ems"></div>`);
     expect(errors).toBe(null);