From 323be39297450fa5a4a70f800511722b4964dec2 Mon Sep 17 00:00:00 2001
From: Keen Yee Liau <kyliau@umich.edu>
Date: Thu, 24 Sep 2020 22:11:57 -0700
Subject: [PATCH] fix(language-service): hybrid visitor returns parent node of
 BoundAttribute (#38995)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

For the following example, the cursor is between `keySpan` and `valueSpan`
of the `BoundAttribute`.
```html
<test-cmp [foo]¦="bar"></test-cmp>
```
Our hybrid visitor will return `Element`in this case, which is the parent
node of the `BoundAttribute`.
This is because we only look at the `keySpan` and `valueSpan`, and not
the source span. The last element in the AST path is `Element`, so it gets
returned.

In this PR, I propose fixing this by adding a sentinel value `undefined`
to the AST path to signal that we've found a source span but the cursor is
neither in the key span nor the value span.

PR Close #38995
---
 .../language-service/ivy/hybrid_visitor.ts    | 37 ++++++++++++-------
 .../ivy/test/hybrid_visitor_spec.ts           |  7 ++++
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/packages/language-service/ivy/hybrid_visitor.ts b/packages/language-service/ivy/hybrid_visitor.ts
index 2f94eea3b4..1dec2486ad 100644
--- a/packages/language-service/ivy/hybrid_visitor.ts
+++ b/packages/language-service/ivy/hybrid_visitor.ts
@@ -19,7 +19,19 @@ import * as t from '@angular/compiler/src/render3/r3_ast';         // t for temp
 export function findNodeAtPosition(ast: t.Node[], position: number): t.Node|e.AST|undefined {
   const visitor = new R3Visitor(position);
   visitor.visitAll(ast);
-  return visitor.path[visitor.path.length - 1];
+  const candidate = visitor.path[visitor.path.length - 1];
+  if (!candidate) {
+    return;
+  }
+  if (isTemplateNodeWithKeyAndValue(candidate)) {
+    const {keySpan, valueSpan} = candidate;
+    // If cursor is within source span but not within key span or value span,
+    // do not return the node.
+    if (!isWithin(position, keySpan) && (valueSpan && !isWithin(position, valueSpan))) {
+      return;
+    }
+  }
+  return candidate;
 }
 
 class R3Visitor implements t.Visitor {
@@ -31,11 +43,6 @@ 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);
     if (isWithin(this.position, {start, end})) {
       const length = end - start;
@@ -100,13 +107,8 @@ class R3Visitor implements t.Visitor {
   }
 
   visitBoundAttribute(attribute: t.BoundAttribute) {
-    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);
-    }
+    const visitor = new ExpressionVisitor(this.position);
+    visitor.visit(attribute.value, this.path);
   }
 
   visitBoundEvent(attribute: t.BoundEvent) {
@@ -166,6 +168,15 @@ export function isTemplateNode(node: t.Node|e.AST): node is t.Node {
   return node.sourceSpan instanceof ParseSourceSpan;
 }
 
+interface NodeWithKeyAndValue extends t.Node {
+  keySpan: ParseSourceSpan;
+  valueSpan?: ParseSourceSpan;
+}
+
+export function isTemplateNodeWithKeyAndValue(node: t.Node|e.AST): node is NodeWithKeyAndValue {
+  return isTemplateNode(node) && node.hasOwnProperty('keySpan');
+}
+
 export function isExpressionNode(node: t.Node|e.AST): node is e.AST {
   return node instanceof e.AST;
 }
diff --git a/packages/language-service/ivy/test/hybrid_visitor_spec.ts b/packages/language-service/ivy/test/hybrid_visitor_spec.ts
index 2bc2a1778e..36504d389b 100644
--- a/packages/language-service/ivy/test/hybrid_visitor_spec.ts
+++ b/packages/language-service/ivy/test/hybrid_visitor_spec.ts
@@ -97,6 +97,13 @@ describe('findNodeAtPosition for template AST', () => {
     expect(node).toBeInstanceOf(e.PropertyRead);
   });
 
+  it('should not locate bound attribute if cursor is between key and value', () => {
+    const {errors, nodes, position} = parse(`<test-cmp [foo]¦="bar"></test-cmp>`);
+    expect(errors).toBeNull();
+    const node = findNodeAtPosition(nodes, position);
+    expect(node).toBeUndefined();
+  });
+
   it('should locate bound event key', () => {
     const {errors, nodes, position} = parse(`<test-cmp (fo¦o)="bar()"></test-cmp>`);
     expect(errors).toBe(null);