From c33326c5382508c8b2c0a805abdb29c4c7cc0b5f Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Mon, 9 Nov 2020 09:16:19 -0800
Subject: [PATCH] refactor(compiler-cli): add keySpan to parsed events (#39609)

Though we currently have the knowledge of where the `key` for an
event binding appears during parsing, we do not propagate this
information to the output AST. This means that once we produce the
template AST, we have no way of mapping a template position to the key
span alone. The best we can currently do is map back to the
`sourceSpan`. This presents problems downstream, specifically for the
language service, where we cannot provide correct information about a
position in a template because the AST is not granular enough.

This is essentially identical to the change from #38898, but for event
bindings rather than input bindings.

PR Close #39609
---
 .../compiler/src/expression_parser/ast.ts     |  5 ++--
 packages/compiler/src/render3/r3_ast.ts       |  9 +++++--
 .../src/render3/r3_template_transform.ts      | 22 ++++++++-------
 .../src/template_parser/binding_parser.ts     | 27 ++++++++++++-------
 .../test/render3/r3_ast_spans_spec.ts         | 18 +++++++------
 .../ivy/test/definitions_spec.ts              |  4 +--
 .../ivy/test/quick_info_spec.ts               |  6 ++---
 7 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/packages/compiler/src/expression_parser/ast.ts b/packages/compiler/src/expression_parser/ast.ts
index 08589c143c..327311a1dd 100644
--- a/packages/compiler/src/expression_parser/ast.ts
+++ b/packages/compiler/src/expression_parser/ast.ts
@@ -869,7 +869,7 @@ export class ParsedProperty {
 
   constructor(
       public name: string, public expression: ASTWithSource, public type: ParsedPropertyType,
-      // TODO(atscott): `keySpan` should really be required but allows `undefined` so VE does
+      // TODO(FW-2095): `keySpan` should really be required but allows `undefined` so VE does
       // not need to be updated. Make `keySpan` required when VE is removed.
       public sourceSpan: ParseSourceSpan, readonly keySpan: ParseSourceSpan|undefined,
       public valueSpan: ParseSourceSpan|undefined) {
@@ -897,7 +897,8 @@ export class ParsedEvent {
   constructor(
       public name: string, public targetOrPhase: string, public type: ParsedEventType,
       public handler: ASTWithSource, public sourceSpan: ParseSourceSpan,
-      public handlerSpan: ParseSourceSpan) {}
+      // TODO(FW-2095): keySpan should be required but was made optional to avoid changing VE
+      public handlerSpan: ParseSourceSpan, readonly keySpan: ParseSourceSpan|undefined) {}
 }
 
 /**
diff --git a/packages/compiler/src/render3/r3_ast.ts b/packages/compiler/src/render3/r3_ast.ts
index 0d408651bb..68153667b1 100644
--- a/packages/compiler/src/render3/r3_ast.ts
+++ b/packages/compiler/src/render3/r3_ast.ts
@@ -73,14 +73,19 @@ export class BoundEvent implements Node {
   constructor(
       public name: string, public type: ParsedEventType, public handler: AST,
       public target: string|null, public phase: string|null, public sourceSpan: ParseSourceSpan,
-      public handlerSpan: ParseSourceSpan) {}
+      public handlerSpan: ParseSourceSpan, readonly keySpan: ParseSourceSpan) {}
 
   static fromParsedEvent(event: ParsedEvent) {
     const target: string|null = event.type === ParsedEventType.Regular ? event.targetOrPhase : null;
     const phase: string|null =
         event.type === ParsedEventType.Animation ? event.targetOrPhase : null;
+    if (event.keySpan === undefined) {
+      throw new Error(`Unexpected state: keySpan must be defined for bound event but was not for ${
+          event.name}: ${event.sourceSpan}`);
+    }
     return new BoundEvent(
-        event.name, event.type, event.handler, target, phase, event.sourceSpan, event.handlerSpan);
+        event.name, event.type, event.handler, target, phase, event.sourceSpan, event.handlerSpan,
+        event.keySpan);
   }
 
   visit<Result>(visitor: Visitor<Result>): Result {
diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts
index a30a3c67d3..3019bfb3b3 100644
--- a/packages/compiler/src/render3/r3_template_transform.ts
+++ b/packages/compiler/src/render3/r3_template_transform.ts
@@ -361,9 +361,10 @@ class HtmlAstToIvyAst implements html.Visitor {
       } else if (bindParts[KW_ON_IDX]) {
         const events: ParsedEvent[] = [];
         const identifier = bindParts[IDENT_KW_IDX];
+        const keySpan = createKeySpan(srcSpan, bindParts[KW_ON_IDX], identifier);
         this.bindingParser.parseEvent(
-            identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes,
-            events);
+            identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes, events,
+            keySpan);
         addEvents(events, boundEvents);
       } else if (bindParts[KW_BINDON_IDX]) {
         const identifier = bindParts[IDENT_KW_IDX];
@@ -372,7 +373,8 @@ class HtmlAstToIvyAst implements html.Visitor {
             identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
             matchableAttributes, parsedProperties, keySpan);
         this.parseAssignmentEvent(
-            identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents);
+            identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents,
+            keySpan);
       } else if (bindParts[KW_AT_IDX]) {
         const keySpan = createKeySpan(srcSpan, '', name);
         this.bindingParser.parseLiteralAttr(
@@ -399,23 +401,23 @@ class HtmlAstToIvyAst implements html.Visitor {
         // TODO(ayazhafiz): update this to handle malformed bindings.
         name.endsWith(delims.end) && name.length > delims.start.length + delims.end.length) {
       const identifier = name.substring(delims.start.length, name.length - delims.end.length);
+      const keySpan = createKeySpan(srcSpan, delims.start, identifier);
       if (delims.start === BINDING_DELIMS.BANANA_BOX.start) {
-        const keySpan = createKeySpan(srcSpan, delims.start, identifier);
         this.bindingParser.parsePropertyBinding(
             identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
             matchableAttributes, parsedProperties, keySpan);
         this.parseAssignmentEvent(
-            identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents);
+            identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents,
+            keySpan);
       } else if (delims.start === BINDING_DELIMS.PROPERTY.start) {
-        const keySpan = createKeySpan(srcSpan, delims.start, identifier);
         this.bindingParser.parsePropertyBinding(
             identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
             matchableAttributes, parsedProperties, keySpan);
       } else {
         const events: ParsedEvent[] = [];
         this.bindingParser.parseEvent(
-            identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes,
-            events);
+            identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes, events,
+            keySpan);
         addEvents(events, boundEvents);
       }
 
@@ -463,11 +465,11 @@ class HtmlAstToIvyAst implements html.Visitor {
   private parseAssignmentEvent(
       name: string, expression: string, sourceSpan: ParseSourceSpan,
       valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
-      boundEvents: t.BoundEvent[]) {
+      boundEvents: t.BoundEvent[], keySpan: ParseSourceSpan) {
     const events: ParsedEvent[] = [];
     this.bindingParser.parseEvent(
         `${name}Change`, `${expression}=$event`, sourceSpan, valueSpan || sourceSpan,
-        targetMatchableAttrs, events);
+        targetMatchableAttrs, events, keySpan);
     addEvents(events, boundEvents);
   }
 
diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts
index f810d55099..ea3636673a 100644
--- a/packages/compiler/src/template_parser/binding_parser.ts
+++ b/packages/compiler/src/template_parser/binding_parser.ts
@@ -97,8 +97,14 @@ export class BindingParser {
       Object.keys(dirMeta.hostListeners).forEach(propName => {
         const expression = dirMeta.hostListeners[propName];
         if (typeof expression === 'string') {
-          // TODO: pass a more accurate handlerSpan for this event.
-          this.parseEvent(propName, expression, sourceSpan, sourceSpan, [], targetEvents);
+          // Use the `sourceSpan` for  `keySpan` and `handlerSpan`. This isn't really accurate, but
+          // neither is the `sourceSpan`, as it represents the `sourceSpan` of the host itself
+          // rather than the source of the host binding (which doesn't exist in the template).
+          // Regardless, neither of these values are used in Ivy but are only here to satisfy the
+          // function signature. This should likely be refactored in the future so that `sourceSpan`
+          // isn't being used inaccurately.
+          this.parseEvent(
+              propName, expression, sourceSpan, sourceSpan, [], targetEvents, sourceSpan);
         } else {
           this._reportError(
               `Value of the host listener "${
@@ -408,19 +414,20 @@ export class BindingParser {
         boundProp.sourceSpan, boundProp.keySpan, boundProp.valueSpan);
   }
 
+  // TODO: keySpan should be required but was made optional to avoid changing VE parser.
   parseEvent(
       name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
-      targetMatchableAttrs: string[][], targetEvents: ParsedEvent[]) {
+      targetMatchableAttrs: string[][], targetEvents: ParsedEvent[], keySpan?: ParseSourceSpan) {
     if (name.length === 0) {
       this._reportError(`Event name is missing in binding`, sourceSpan);
     }
 
     if (isAnimationLabel(name)) {
       name = name.substr(1);
-      this._parseAnimationEvent(name, expression, sourceSpan, handlerSpan, targetEvents);
+      this._parseAnimationEvent(name, expression, sourceSpan, handlerSpan, targetEvents, keySpan);
     } else {
       this._parseRegularEvent(
-          name, expression, sourceSpan, handlerSpan, targetMatchableAttrs, targetEvents);
+          name, expression, sourceSpan, handlerSpan, targetMatchableAttrs, targetEvents, keySpan);
     }
   }
 
@@ -432,7 +439,7 @@ export class BindingParser {
 
   private _parseAnimationEvent(
       name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
-      targetEvents: ParsedEvent[]) {
+      targetEvents: ParsedEvent[], keySpan?: ParseSourceSpan) {
     const matches = splitAtPeriod(name, [name, '']);
     const eventName = matches[0];
     const phase = matches[1].toLowerCase();
@@ -442,7 +449,7 @@ export class BindingParser {
         case 'done':
           const ast = this._parseAction(expression, handlerSpan);
           targetEvents.push(new ParsedEvent(
-              eventName, phase, ParsedEventType.Animation, ast, sourceSpan, handlerSpan));
+              eventName, phase, ParsedEventType.Animation, ast, sourceSpan, handlerSpan, keySpan));
           break;
 
         default:
@@ -462,13 +469,13 @@ export class BindingParser {
 
   private _parseRegularEvent(
       name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
-      targetMatchableAttrs: string[][], targetEvents: ParsedEvent[]) {
+      targetMatchableAttrs: string[][], targetEvents: ParsedEvent[], keySpan?: ParseSourceSpan) {
     // long format: 'target: eventName'
     const [target, eventName] = splitAtColon(name, [null!, name]);
     const ast = this._parseAction(expression, handlerSpan);
     targetMatchableAttrs.push([name!, ast.source!]);
-    targetEvents.push(
-        new ParsedEvent(eventName, target, ParsedEventType.Regular, ast, sourceSpan, handlerSpan));
+    targetEvents.push(new ParsedEvent(
+        eventName, target, ParsedEventType.Regular, ast, sourceSpan, handlerSpan, keySpan));
     // Don't detect directives for event names for now,
     // so don't add the event name to the matchableAttrs
   }
diff --git a/packages/compiler/test/render3/r3_ast_spans_spec.ts b/packages/compiler/test/render3/r3_ast_spans_spec.ts
index e7e34c07ff..3b1ef79825 100644
--- a/packages/compiler/test/render3/r3_ast_spans_spec.ts
+++ b/packages/compiler/test/render3/r3_ast_spans_spec.ts
@@ -78,8 +78,10 @@ class R3AstSourceSpans implements t.Visitor<void> {
   }
 
   visitBoundEvent(event: t.BoundEvent) {
-    this.result.push(
-        ['BoundEvent', humanizeSpan(event.sourceSpan), humanizeSpan(event.handlerSpan)]);
+    this.result.push([
+      'BoundEvent', humanizeSpan(event.sourceSpan), humanizeSpan(event.keySpan),
+      humanizeSpan(event.handlerSpan)
+    ]);
   }
 
   visitText(text: t.Text) {
@@ -356,21 +358,21 @@ describe('R3 AST source spans', () => {
     it('is correct for event names case sensitive', () => {
       expectFromHtml('<div (someEvent)="v"></div>').toEqual([
         ['Element', '<div (someEvent)="v"></div>', '<div (someEvent)="v">', '</div>'],
-        ['BoundEvent', '(someEvent)="v"', 'v'],
+        ['BoundEvent', '(someEvent)="v"', 'someEvent', 'v'],
       ]);
     });
 
     it('is correct for bound events via on-', () => {
       expectFromHtml('<div on-event="v"></div>').toEqual([
         ['Element', '<div on-event="v"></div>', '<div on-event="v">', '</div>'],
-        ['BoundEvent', 'on-event="v"', 'v'],
+        ['BoundEvent', 'on-event="v"', 'event', 'v'],
       ]);
     });
 
     it('is correct for bound events via data-on-', () => {
       expectFromHtml('<div data-on-event="v"></div>').toEqual([
         ['Element', '<div data-on-event="v"></div>', '<div data-on-event="v">', '</div>'],
-        ['BoundEvent', 'data-on-event="v"', 'v'],
+        ['BoundEvent', 'data-on-event="v"', 'event', 'v'],
       ]);
     });
 
@@ -378,7 +380,7 @@ describe('R3 AST source spans', () => {
       expectFromHtml('<div [(prop)]="v"></div>').toEqual([
         ['Element', '<div [(prop)]="v"></div>', '<div [(prop)]="v">', '</div>'],
         ['BoundAttribute', '[(prop)]="v"', 'prop', 'v'],
-        ['BoundEvent', '[(prop)]="v"', 'v'],
+        ['BoundEvent', '[(prop)]="v"', 'prop', 'v'],
       ]);
     });
 
@@ -386,7 +388,7 @@ describe('R3 AST source spans', () => {
       expectFromHtml('<div bindon-prop="v"></div>').toEqual([
         ['Element', '<div bindon-prop="v"></div>', '<div bindon-prop="v">', '</div>'],
         ['BoundAttribute', 'bindon-prop="v"', 'prop', 'v'],
-        ['BoundEvent', 'bindon-prop="v"', 'v'],
+        ['BoundEvent', 'bindon-prop="v"', 'prop', 'v'],
       ]);
     });
 
@@ -394,7 +396,7 @@ describe('R3 AST source spans', () => {
       expectFromHtml('<div data-bindon-prop="v"></div>').toEqual([
         ['Element', '<div data-bindon-prop="v"></div>', '<div data-bindon-prop="v">', '</div>'],
         ['BoundAttribute', 'data-bindon-prop="v"', 'prop', 'v'],
-        ['BoundEvent', 'data-bindon-prop="v"', 'v'],
+        ['BoundEvent', 'data-bindon-prop="v"', 'prop', 'v'],
       ]);
     });
   });
diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts
index 50f0dc59ed..117f572593 100644
--- a/packages/language-service/ivy/test/definitions_spec.ts
+++ b/packages/language-service/ivy/test/definitions_spec.ts
@@ -199,7 +199,7 @@ describe('definitions', () => {
       it('should work for event providers', () => {
         const definitions = getDefinitionsAndAssertBoundSpan({
           templateOverride: `<test-comp (te¦st)="myClick($event)"></test-comp>`,
-          expectedSpanText: '(test)="myClick($event)"',
+          expectedSpanText: 'test',
         });
         expect(definitions!.length).toEqual(1);
 
@@ -218,7 +218,7 @@ describe('definitions', () => {
       it('should return the directive when the event is part of the selector', () => {
         const definitions = getDefinitionsAndAssertBoundSpan({
           templateOverride: `<div (eventSelect¦or)="title = ''"></div>`,
-          expectedSpanText: `(eventSelector)="title = ''"`,
+          expectedSpanText: `eventSelector`,
         });
         expect(definitions!.length).toEqual(2);
 
diff --git a/packages/language-service/ivy/test/quick_info_spec.ts b/packages/language-service/ivy/test/quick_info_spec.ts
index 75963e389e..b8a6b5b6d6 100644
--- a/packages/language-service/ivy/test/quick_info_spec.ts
+++ b/packages/language-service/ivy/test/quick_info_spec.ts
@@ -156,7 +156,7 @@ describe('quick info', () => {
       it('should work for event providers', () => {
         expectQuickInfo({
           templateOverride: `<test-comp (te¦st)="myClick($event)"></test-comp>`,
-          expectedSpanText: '(test)="myClick($event)"',
+          expectedSpanText: 'test',
           expectedDisplayString: '(event) TestComponent.testEvent: EventEmitter<any>'
         });
       });
@@ -164,12 +164,12 @@ describe('quick info', () => {
       it('should work for on- syntax binding', () => {
         expectQuickInfo({
           templateOverride: `<test-comp on-te¦st="myClick($event)"></test-comp>`,
-          expectedSpanText: 'on-test="myClick($event)"',
+          expectedSpanText: 'test',
           expectedDisplayString: '(event) TestComponent.testEvent: EventEmitter<any>'
         });
         expectQuickInfo({
           templateOverride: `<test-comp data-on-te¦st="myClick($event)"></test-comp>`,
-          expectedSpanText: 'data-on-test="myClick($event)"',
+          expectedSpanText: 'test',
           expectedDisplayString: '(event) TestComponent.testEvent: EventEmitter<any>'
         });
       });