From 21651d362d84da6355cb23de0cd145fedc10f0c3 Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Mon, 9 Nov 2020 10:25:25 -0800
Subject: [PATCH] refactor(compiler-cli): add keySpan to text attributes
 (#39613)

Similar to #39609 and #38898, though we currently have the knowledge of where the key for an
attribute 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.

PR Close #39613
---
 .../compiler/src/i18n/extractor_merger.ts     |  8 ++++++--
 packages/compiler/src/ml_parser/ast.ts        |  3 ++-
 .../src/ml_parser/icu_ast_expander.ts         | 20 ++++++++++++++-----
 packages/compiler/src/ml_parser/parser.ts     |  3 ++-
 packages/compiler/src/render3/r3_ast.ts       |  9 ++++++++-
 .../src/render3/r3_template_transform.ts      | 11 ++++++----
 .../test/render3/r3_ast_spans_spec.ts         | 20 ++++++++++---------
 .../ivy/test/definitions_spec.ts              |  2 +-
 .../ivy/test/quick_info_spec.ts               |  2 +-
 9 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/packages/compiler/src/i18n/extractor_merger.ts b/packages/compiler/src/i18n/extractor_merger.ts
index 66a8e7a1df..b0da1c372b 100644
--- a/packages/compiler/src/i18n/extractor_merger.ts
+++ b/packages/compiler/src/i18n/extractor_merger.ts
@@ -394,10 +394,14 @@ class _Visitor implements html.Visitor {
         const nodes = this._translations.get(message);
         if (nodes) {
           if (nodes.length == 0) {
-            translatedAttributes.push(new html.Attribute(attr.name, '', attr.sourceSpan));
+            translatedAttributes.push(new html.Attribute(
+                attr.name, '', attr.sourceSpan, undefined /* keySpan */, undefined /* valueSpan */,
+                undefined /* i18n */));
           } else if (nodes[0] instanceof html.Text) {
             const value = (nodes[0] as html.Text).value;
-            translatedAttributes.push(new html.Attribute(attr.name, value, attr.sourceSpan));
+            translatedAttributes.push(new html.Attribute(
+                attr.name, value, attr.sourceSpan, undefined /* keySpan */,
+                undefined /* valueSpan */, undefined /* i18n */));
           } else {
             this._reportError(
                 el,
diff --git a/packages/compiler/src/ml_parser/ast.ts b/packages/compiler/src/ml_parser/ast.ts
index ead5c11cce..c962e04a9d 100644
--- a/packages/compiler/src/ml_parser/ast.ts
+++ b/packages/compiler/src/ml_parser/ast.ts
@@ -53,7 +53,8 @@ export class ExpansionCase implements Node {
 export class Attribute extends NodeWithI18n {
   constructor(
       public name: string, public value: string, sourceSpan: ParseSourceSpan,
-      public valueSpan?: ParseSourceSpan, i18n?: I18nMeta) {
+      readonly keySpan: ParseSourceSpan|undefined, public valueSpan?: ParseSourceSpan,
+      i18n?: I18nMeta) {
     super(sourceSpan, i18n);
   }
   visit(visitor: Visitor, context: any): any {
diff --git a/packages/compiler/src/ml_parser/icu_ast_expander.ts b/packages/compiler/src/ml_parser/icu_ast_expander.ts
index 120a433001..9e08c954dd 100644
--- a/packages/compiler/src/ml_parser/icu_ast_expander.ts
+++ b/packages/compiler/src/ml_parser/icu_ast_expander.ts
@@ -102,10 +102,14 @@ function _expandPluralForm(ast: html.Expansion, errors: ParseError[]): html.Elem
     errors.push(...expansionResult.errors);
 
     return new html.Element(
-        `ng-template`, [new html.Attribute('ngPluralCase', `${c.value}`, c.valueSourceSpan)],
+        `ng-template`, [new html.Attribute(
+                           'ngPluralCase', `${c.value}`, c.valueSourceSpan, undefined /* keySpan */,
+                           undefined /* valueSpan */, undefined /* i18n */)],
         expansionResult.nodes, c.sourceSpan, c.sourceSpan, c.sourceSpan);
   });
-  const switchAttr = new html.Attribute('[ngPlural]', ast.switchValue, ast.switchValueSourceSpan);
+  const switchAttr = new html.Attribute(
+      '[ngPlural]', ast.switchValue, ast.switchValueSourceSpan, undefined /* keySpan */,
+      undefined /* valueSpan */, undefined /* i18n */);
   return new html.Element(
       'ng-container', [switchAttr], children, ast.sourceSpan, ast.sourceSpan, ast.sourceSpan);
 }
@@ -119,15 +123,21 @@ function _expandDefaultForm(ast: html.Expansion, errors: ParseError[]): html.Ele
     if (c.value === 'other') {
       // other is the default case when no values match
       return new html.Element(
-          `ng-template`, [new html.Attribute('ngSwitchDefault', '', c.valueSourceSpan)],
+          `ng-template`, [new html.Attribute(
+                             'ngSwitchDefault', '', c.valueSourceSpan, undefined /* keySpan */,
+                             undefined /* valueSpan */, undefined /* i18n */)],
           expansionResult.nodes, c.sourceSpan, c.sourceSpan, c.sourceSpan);
     }
 
     return new html.Element(
-        `ng-template`, [new html.Attribute('ngSwitchCase', `${c.value}`, c.valueSourceSpan)],
+        `ng-template`, [new html.Attribute(
+                           'ngSwitchCase', `${c.value}`, c.valueSourceSpan, undefined /* keySpan */,
+                           undefined /* valueSpan */, undefined /* i18n */)],
         expansionResult.nodes, c.sourceSpan, c.sourceSpan, c.sourceSpan);
   });
-  const switchAttr = new html.Attribute('[ngSwitch]', ast.switchValue, ast.switchValueSourceSpan);
+  const switchAttr = new html.Attribute(
+      '[ngSwitch]', ast.switchValue, ast.switchValueSourceSpan, undefined /* keySpan */,
+      undefined /* valueSpan */, undefined /* i18n */);
   return new html.Element(
       'ng-container', [switchAttr], children, ast.sourceSpan, ast.sourceSpan, ast.sourceSpan);
 }
diff --git a/packages/compiler/src/ml_parser/parser.ts b/packages/compiler/src/ml_parser/parser.ts
index 2bd172921e..df73b9b6ac 100644
--- a/packages/compiler/src/ml_parser/parser.ts
+++ b/packages/compiler/src/ml_parser/parser.ts
@@ -351,9 +351,10 @@ class _TreeBuilder {
       const quoteToken = this._advance();
       end = quoteToken.sourceSpan.end;
     }
+    const keySpan = new ParseSourceSpan(attrName.sourceSpan.start, attrName.sourceSpan.end);
     return new html.Attribute(
         fullName, value,
-        new ParseSourceSpan(attrName.sourceSpan.start, end, attrName.sourceSpan.fullStart),
+        new ParseSourceSpan(attrName.sourceSpan.start, end, attrName.sourceSpan.fullStart), keySpan,
         valueSpan);
   }
 
diff --git a/packages/compiler/src/render3/r3_ast.ts b/packages/compiler/src/render3/r3_ast.ts
index ef2d1a732a..0d408651bb 100644
--- a/packages/compiler/src/render3/r3_ast.ts
+++ b/packages/compiler/src/render3/r3_ast.ts
@@ -30,10 +30,17 @@ export class BoundText implements Node {
   }
 }
 
+/**
+ * Represents a text attribute in the template.
+ *
+ * `valueSpan` may not be present in cases where there is no value `<div a></div>`.
+ * `keySpan` may also not be present for synthetic attributes from ICU expansions.
+ */
 export class TextAttribute implements Node {
   constructor(
       public name: string, public value: string, public sourceSpan: ParseSourceSpan,
-      public valueSpan?: ParseSourceSpan, public i18n?: I18nMeta) {}
+      readonly keySpan: ParseSourceSpan|undefined, public valueSpan?: ParseSourceSpan,
+      public i18n?: I18nMeta) {}
   visit<Result>(visitor: Visitor<Result>): Result {
     return visitor.visitTextAttribute(this);
   }
diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts
index 4d2525d817..a30a3c67d3 100644
--- a/packages/compiler/src/render3/r3_template_transform.ts
+++ b/packages/compiler/src/render3/r3_template_transform.ts
@@ -166,7 +166,7 @@ class HtmlAstToIvyAst implements html.Visitor {
 
       if (!hasBinding && !isTemplateBinding) {
         // don't include the bindings as attributes as well in the AST
-        attributes.push(this.visitAttribute(attribute) as t.TextAttribute);
+        attributes.push(this.visitAttribute(attribute));
       }
     }
 
@@ -238,7 +238,8 @@ class HtmlAstToIvyAst implements html.Visitor {
 
   visitAttribute(attribute: html.Attribute): t.TextAttribute {
     return new t.TextAttribute(
-        attribute.name, attribute.value, attribute.sourceSpan, attribute.valueSpan, attribute.i18n);
+        attribute.name, attribute.value, attribute.sourceSpan, attribute.keySpan,
+        attribute.valueSpan, attribute.i18n);
   }
 
   visitText(text: html.Text): t.Node {
@@ -301,7 +302,8 @@ class HtmlAstToIvyAst implements html.Visitor {
       const i18n = i18nPropsMeta[prop.name];
       if (prop.isLiteral) {
         literal.push(new t.TextAttribute(
-            prop.name, prop.expression.source || '', prop.sourceSpan, undefined, i18n));
+            prop.name, prop.expression.source || '', prop.sourceSpan, prop.keySpan, prop.valueSpan,
+            i18n));
       } else {
         // Note that validation is skipped and property mapping is disabled
         // due to the fact that we need to make sure a given prop is not an
@@ -501,7 +503,8 @@ class NonBindableVisitor implements html.Visitor {
 
   visitAttribute(attribute: html.Attribute): t.TextAttribute {
     return new t.TextAttribute(
-        attribute.name, attribute.value, attribute.sourceSpan, undefined, attribute.i18n);
+        attribute.name, attribute.value, attribute.sourceSpan, attribute.keySpan,
+        attribute.valueSpan, attribute.i18n);
   }
 
   visitText(text: html.Text): t.Text {
diff --git a/packages/compiler/test/render3/r3_ast_spans_spec.ts b/packages/compiler/test/render3/r3_ast_spans_spec.ts
index 7cbfc704ad..e7e34c07ff 100644
--- a/packages/compiler/test/render3/r3_ast_spans_spec.ts
+++ b/packages/compiler/test/render3/r3_ast_spans_spec.ts
@@ -64,8 +64,10 @@ class R3AstSourceSpans implements t.Visitor<void> {
   }
 
   visitTextAttribute(attribute: t.TextAttribute) {
-    this.result.push(
-        ['TextAttribute', humanizeSpan(attribute.sourceSpan), humanizeSpan(attribute.valueSpan)]);
+    this.result.push([
+      'TextAttribute', humanizeSpan(attribute.sourceSpan), humanizeSpan(attribute.keySpan),
+      humanizeSpan(attribute.valueSpan)
+    ]);
   }
 
   visitBoundAttribute(attribute: t.BoundAttribute) {
@@ -132,14 +134,14 @@ describe('R3 AST source spans', () => {
     it('is correct for elements with attributes', () => {
       expectFromHtml('<div a="b"></div>').toEqual([
         ['Element', '<div a="b"></div>', '<div a="b">', '</div>'],
-        ['TextAttribute', 'a="b"', 'b'],
+        ['TextAttribute', 'a="b"', 'a', 'b'],
       ]);
     });
 
     it('is correct for elements with attributes without value', () => {
       expectFromHtml('<div a></div>').toEqual([
         ['Element', '<div a></div>', '<div a>', '</div>'],
-        ['TextAttribute', 'a', '<empty>'],
+        ['TextAttribute', 'a', 'a', '<empty>'],
       ]);
     });
   });
@@ -193,7 +195,7 @@ describe('R3 AST source spans', () => {
     it('is correct for * directives', () => {
       expectFromHtml('<div *ngIf></div>').toEqual([
         ['Template', '<div *ngIf></div>', '<div *ngIf>', '</div>'],
-        ['TextAttribute', 'ngIf', '<empty>'],
+        ['TextAttribute', 'ngIf', 'ngIf', '<empty>'],
         ['Element', '<div *ngIf></div>', '<div *ngIf>', '</div>'],
       ]);
     });
@@ -263,7 +265,7 @@ describe('R3 AST source spans', () => {
           'Template', '<ng-template k1="v1"></ng-template>', '<ng-template k1="v1">',
           '</ng-template>'
         ],
-        ['TextAttribute', 'k1="v1"', 'v1'],
+        ['TextAttribute', 'k1="v1"', 'k1', 'v1'],
       ]);
     });
 
@@ -290,7 +292,7 @@ describe('R3 AST source spans', () => {
           'Template', '<div *ngFor="let item of items"></div>', '<div *ngFor="let item of items">',
           '</div>'
         ],
-        ['TextAttribute', 'ngFor', '<empty>'],
+        ['TextAttribute', 'ngFor', 'ngFor', '<empty>'],
         ['BoundAttribute', 'of items', 'of', 'items'],
         ['Variable', 'let item ', 'item', '<empty>'],
         [
@@ -319,7 +321,7 @@ describe('R3 AST source spans', () => {
           'Template', '<div *ngFor="let item of items; trackBy: trackByFn"></div>',
           '<div *ngFor="let item of items; trackBy: trackByFn">', '</div>'
         ],
-        ['TextAttribute', 'ngFor', '<empty>'],
+        ['TextAttribute', 'ngFor', 'ngFor', '<empty>'],
         ['BoundAttribute', 'of items; ', 'of', 'items'],
         ['BoundAttribute', 'trackBy: trackByFn', 'trackBy', 'trackByFn'],
         ['Variable', 'let item ', 'item', '<empty>'],
@@ -334,7 +336,7 @@ describe('R3 AST source spans', () => {
     it('is correct for variables via let ...', () => {
       expectFromHtml('<div *ngIf="let a=b"></div>').toEqual([
         ['Template', '<div *ngIf="let a=b"></div>', '<div *ngIf="let a=b">', '</div>'],
-        ['TextAttribute', 'ngIf', '<empty>'],
+        ['TextAttribute', 'ngIf', 'ngIf', '<empty>'],
         ['Variable', 'let a=b', 'a', 'b'],
         ['Element', '<div *ngIf="let a=b"></div>', '<div *ngIf="let a=b">', '</div>'],
       ]);
diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts
index dcc52ca9f4..50f0dc59ed 100644
--- a/packages/language-service/ivy/test/definitions_spec.ts
+++ b/packages/language-service/ivy/test/definitions_spec.ts
@@ -135,7 +135,7 @@ describe('definitions', () => {
       it('should work for text inputs', () => {
         const definitions = getDefinitionsAndAssertBoundSpan({
           templateOverride: `<test-comp tcN¦ame="name"></test-comp>`,
-          expectedSpanText: 'tcName="name"',
+          expectedSpanText: 'tcName',
         });
         expect(definitions!.length).toEqual(1);
 
diff --git a/packages/language-service/ivy/test/quick_info_spec.ts b/packages/language-service/ivy/test/quick_info_spec.ts
index 2557e3e844..75963e389e 100644
--- a/packages/language-service/ivy/test/quick_info_spec.ts
+++ b/packages/language-service/ivy/test/quick_info_spec.ts
@@ -295,7 +295,7 @@ describe('quick info', () => {
     it('should find input binding on text attribute', () => {
       expectQuickInfo({
         templateOverride: `<test-comp tcN¦ame="title"></test-comp>`,
-        expectedSpanText: 'tcName="title"',
+        expectedSpanText: 'tcName',
         expectedDisplayString: '(property) TestComponent.name: string'
       });
     });