From 1d8c5d88cdfc88010cf4571a5f61fc1b4ff8c7c1 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 26 Aug 2020 11:56:38 +0100 Subject: [PATCH] refactor(compiler): `element.sourceSpan` should span the `outerHTML` (#38581) Previously, the `sourceSpan` and `startSourceSpan` were the same object, which meant that you had the following situation: ``` element =
some content
sourceSpan =
startSourceSpan =
endSourceSpan =
``` This made `sourceSpan` redundant and meant that if you wanted a span for the whole element including its content and closing tag, it had to be computed. Now `sourceSpan` is separated from `startSourceSpan` resulting in: ``` element =
some content
sourceSpan =
some content
startSourceSpan =
endSourceSpan =
``` PR Close #38581 --- .../src/ngtsc/indexer/src/template.ts | 2 +- .../src/ngtsc/typecheck/src/dom.ts | 2 +- .../compiler-cli/test/extract_i18n_spec.ts | 4 ++-- .../test/ngtsc/template_mapping_spec.ts | 4 ++-- packages/compiler/src/i18n/i18n_parser.ts | 2 +- packages/compiler/src/ml_parser/parser.ts | 5 +++- .../compiler/src/render3/view/template.ts | 16 +++++++------ .../test/i18n/extractor_merger_spec.ts | 17 +++++++++---- .../test/ml_parser/html_parser_spec.ts | 15 ++++++------ .../test/render3/r3_ast_spans_spec.ts | 24 +++++++++---------- .../template_parser/template_parser_spec.ts | 11 +++++---- 11 files changed, 59 insertions(+), 43 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/indexer/src/template.ts b/packages/compiler-cli/src/ngtsc/indexer/src/template.ts index b6b08c29d4..5303112c87 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/src/template.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/src/template.ts @@ -246,7 +246,7 @@ class TemplateVisitor extends TmplAstRecursiveVisitor { name = node.name; kind = IdentifierKind.Element; } - const {sourceSpan} = node; + const sourceSpan = node.startSourceSpan; // An element's or template's source span can be of the form ``, ``, or // ``. Only the selector is interesting to the indexer, so the source is // searched for the first occurrence of the element (selector) name. diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts index c6b39be34d..a946a2539d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts @@ -93,7 +93,7 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker { } const diag = makeTemplateDiagnostic( - id, mapping, element.sourceSpan, ts.DiagnosticCategory.Error, + id, mapping, element.startSourceSpan, ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.SCHEMA_INVALID_ELEMENT), errorMsg); this._diagnostics.push(diag); } diff --git a/packages/compiler-cli/test/extract_i18n_spec.ts b/packages/compiler-cli/test/extract_i18n_spec.ts index e06f4f8108..4e77cced28 100644 --- a/packages/compiler-cli/test/extract_i18n_spec.ts +++ b/packages/compiler-cli/test/extract_i18n_spec.ts @@ -42,7 +42,7 @@ const EXPECTED_XMB = ` src/icu.html:4,6 foo { count, plural, =1 {...} other {...}}{ count, plural, =1 {...} other {...}} - src/placeholders.html:1Name: <b><b>{{ + src/placeholders.html:1,3Name: <b><b>{{ name // i18n(ph="name") }}{{ name // i18n(ph="name") @@ -182,7 +182,7 @@ const EXPECTED_XLIFF2 = ` with placeholders - src/placeholders.html:1 + src/placeholders.html:1,3 Name: ', + source: '', generated: 'i0.ɵɵprojection(1)', sourceUrl: '../test.ts' }); @@ -351,7 +351,7 @@ runInEachFileSystem((os) => { expect(mappings).toContain( {source: '
', generated: 'i0.ɵɵelementStart(2, "div")', sourceUrl: '../test.ts'}); expect(mappings).toContain({ - source: '', + source: '', generated: 'i0.ɵɵprojection(3, 1)', sourceUrl: '../test.ts' }); diff --git a/packages/compiler/src/i18n/i18n_parser.ts b/packages/compiler/src/i18n/i18n_parser.ts index d6d93f32f8..1747e64463 100644 --- a/packages/compiler/src/i18n/i18n_parser.ts +++ b/packages/compiler/src/i18n/i18n_parser.ts @@ -83,7 +83,7 @@ class _I18nVisitor implements html.Visitor { const isVoid: boolean = getHtmlTagDefinition(el.name).isVoid; const startPhName = context.placeholderRegistry.getStartTagPlaceholderName(el.name, attrs, isVoid); - context.placeholderToContent[startPhName] = el.sourceSpan.toString(); + context.placeholderToContent[startPhName] = el.startSourceSpan.toString(); let closePhName = ''; diff --git a/packages/compiler/src/ml_parser/parser.ts b/packages/compiler/src/ml_parser/parser.ts index 3d3131989c..5c5a3f9805 100644 --- a/packages/compiler/src/ml_parser/parser.ts +++ b/packages/compiler/src/ml_parser/parser.ts @@ -258,7 +258,9 @@ class _TreeBuilder { } const end = this._peek.sourceSpan.start; const span = new ParseSourceSpan(startTagToken.sourceSpan.start, end); - const el = new html.Element(fullName, attrs, [], span, span, undefined); + // Create a separate `startSpan` because `span` will be modified when there is an `end` span. + const startSpan = new ParseSourceSpan(startTagToken.sourceSpan.start, end); + const el = new html.Element(fullName, attrs, [], span, startSpan, undefined); this._pushElement(el); if (selfClosing) { // Elements that are self-closed have their `endSourceSpan` set to the full span, as the @@ -301,6 +303,7 @@ class _TreeBuilder { // removed from the element stack at this point are closed implicitly, so they won't get // an end source span (as there is no explicit closing element). el.endSourceSpan = endSourceSpan; + el.sourceSpan.end = endSourceSpan.end || el.sourceSpan.end; this._elementStack.splice(stackIndex, this._elementStack.length - stackIndex); return true; diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 818d0c5119..b0599fc717 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -544,7 +544,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver private addNamespaceInstruction(nsInstruction: o.ExternalReference, element: t.Element) { this._namespace = nsInstruction; - this.creationInstruction(element.sourceSpan, nsInstruction); + this.creationInstruction(element.startSourceSpan, nsInstruction); } /** @@ -671,15 +671,16 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver trimTrailingNulls(parameters)); } else { this.creationInstruction( - element.sourceSpan, isNgContainer ? R3.elementContainerStart : R3.elementStart, + element.startSourceSpan, isNgContainer ? R3.elementContainerStart : R3.elementStart, trimTrailingNulls(parameters)); if (isNonBindableMode) { - this.creationInstruction(element.sourceSpan, R3.disableBindings); + this.creationInstruction(element.startSourceSpan, R3.disableBindings); } if (i18nAttrs.length > 0) { - this.i18nAttributesInstruction(elementIndex, i18nAttrs, element.sourceSpan); + this.i18nAttributesInstruction( + elementIndex, i18nAttrs, element.startSourceSpan ?? element.sourceSpan); } // Generate Listeners (outputs) @@ -695,7 +696,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // Note: it's important to keep i18n/i18nStart instructions after i18nAttributes and // listeners, to make sure i18nAttributes instruction targets current element at runtime. if (isI18nRootElement) { - this.i18nStart(element.sourceSpan, element.i18n!, createSelfClosingI18nInstruction); + this.i18nStart(element.startSourceSpan, element.i18n!, createSelfClosingI18nInstruction); } } @@ -827,7 +828,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver if (!createSelfClosingInstruction) { // Finish element construction mode. - const span = element.endSourceSpan || element.sourceSpan; + const span = element.endSourceSpan ?? element.sourceSpan; if (isI18nRootElement) { this.i18nEnd(span, createSelfClosingI18nInstruction); } @@ -919,7 +920,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // elements, in case of inline templates, corresponding instructions will be generated in the // nested template function. if (i18nAttrs.length > 0) { - this.i18nAttributesInstruction(templateIndex, i18nAttrs, template.sourceSpan); + this.i18nAttributesInstruction( + templateIndex, i18nAttrs, template.startSourceSpan ?? template.sourceSpan); } // Add the input bindings diff --git a/packages/compiler/test/i18n/extractor_merger_spec.ts b/packages/compiler/test/i18n/extractor_merger_spec.ts index eb7c4a8567..e607d42e4f 100644 --- a/packages/compiler/test/i18n/extractor_merger_spec.ts +++ b/packages/compiler/test/i18n/extractor_merger_spec.ts @@ -322,19 +322,28 @@ import {serializeNodes as serializeHtmlNodes} from '../ml_parser/util/util'; describe('elements', () => { it('should report nested translatable elements', () => { expect(extractErrors(`

`)).toEqual([ - ['Could not mark an element as translatable inside a translatable section', ''], + [ + 'Could not mark an element as translatable inside a translatable section', + '' + ], ]); }); it('should report translatable elements in implicit elements', () => { expect(extractErrors(`

`, ['p'])).toEqual([ - ['Could not mark an element as translatable inside a translatable section', ''], + [ + 'Could not mark an element as translatable inside a translatable section', + '' + ], ]); }); it('should report translatable elements in translatable blocks', () => { expect(extractErrors(``)).toEqual([ - ['Could not mark an element as translatable inside a translatable section', ''], + [ + 'Could not mark an element as translatable inside a translatable section', + '' + ], ]); }); }); @@ -370,7 +379,7 @@ import {serializeNodes as serializeHtmlNodes} from '../ml_parser/util/util'; it('should report when start and end of a block are not at the same level', () => { expect(extractErrors(`

`)).toEqual([ ['I18N blocks should not cross element boundaries', '

'], ]); expect(extractErrors(`

`)).toEqual([ diff --git a/packages/compiler/test/ml_parser/html_parser_spec.ts b/packages/compiler/test/ml_parser/html_parser_spec.ts index b3eb04fbe3..6dd3f9e97e 100644 --- a/packages/compiler/test/ml_parser/html_parser_spec.ts +++ b/packages/compiler/test/ml_parser/html_parser_spec.ts @@ -653,7 +653,8 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe '
\na\n
', 'TestComp'))) .toEqual([ [ - html.Element, 'div', 0, '
', + html.Element, 'div', 0, + '
\na\n
', '
', '
' ], [html.Attribute, '[prop]', 'v1', '[prop]="v1"'], @@ -676,14 +677,14 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe it('should not set the end source span for void elements', () => { expect(humanizeDomSourceSpans(parser.parse('

', 'TestComp'))).toEqual([ - [html.Element, 'div', 0, '
', '
', '
'], + [html.Element, 'div', 0, '

', '
', '
'], [html.Element, 'br', 1, '
', '
', null], ]); }); it('should not set the end source span for multiple void elements', () => { expect(humanizeDomSourceSpans(parser.parse('

', 'TestComp'))).toEqual([ - [html.Element, 'div', 0, '
', '
', '
'], + [html.Element, 'div', 0, '

', '
', '
'], [html.Element, 'br', 1, '
', '
', null], [html.Element, 'hr', 1, '
', '
', null], ]); @@ -703,19 +704,19 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe it('should set the end source span for self-closing elements', () => { expect(humanizeDomSourceSpans(parser.parse('

', 'TestComp'))).toEqual([ - [html.Element, 'div', 0, '
', '
', '
'], + [html.Element, 'div', 0, '

', '
', '
'], [html.Element, 'br', 1, '
', '
', '
'], ]); }); it('should not set the end source span for elements that are implicitly closed', () => { expect(humanizeDomSourceSpans(parser.parse('

', 'TestComp'))).toEqual([ - [html.Element, 'div', 0, '
', '
', '
'], + [html.Element, 'div', 0, '

', '
', '
'], [html.Element, 'p', 1, '

', '

', null], ]); expect(humanizeDomSourceSpans(parser.parse('

  • A
  • B
    • ', 'TestComp'))) .toEqual([ - [html.Element, 'div', 0, '
    ', '
    ', '
    '], + [html.Element, 'div', 0, '
  • A
  • B
    • ', '
    ', '
    '], [html.Element, 'li', 1, '
  • ', '
  • ', null], [html.Text, 'A', 2, 'A'], [html.Element, 'li', 1, '
  • ', '
  • ', null], @@ -728,7 +729,7 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe '
    {count, plural, =0 {msg}}
    ', 'TestComp', {tokenizeExpansionForms: true}))) .toEqual([ - [html.Element, 'div', 0, '
    ', '
    ', '
    '], + [html.Element, 'div', 0, '
    {count, plural, =0 {msg}}
    ', '
    ', '
    '], [html.Expansion, 'count', 'plural', 1, '{count, plural, =0 {msg}}'], [html.ExpansionCase, '=0', 2, '=0 {msg}'], ]); diff --git a/packages/compiler/test/render3/r3_ast_spans_spec.ts b/packages/compiler/test/render3/r3_ast_spans_spec.ts index 38f6d93d42..6fc6b3003b 100644 --- a/packages/compiler/test/render3/r3_ast_spans_spec.ts +++ b/packages/compiler/test/render3/r3_ast_spans_spec.ts @@ -173,7 +173,7 @@ describe('R3 AST source spans', () => { describe('templates', () => { it('is correct for * directives', () => { expectFromHtml('
    ').toEqual([ - ['Template', '0:11', '0:11', '11:17'], + ['Template', '0:17', '0:11', '11:17'], ['TextAttribute', '5:10', ''], ['Element', '0:17', '0:11', '11:17'], ]); @@ -181,48 +181,48 @@ describe('R3 AST source spans', () => { it('is correct for ', () => { expectFromHtml('').toEqual([ - ['Template', '0:13', '0:13', '13:27'], + ['Template', '0:27', '0:13', '13:27'], ]); }); it('is correct for reference via #...', () => { expectFromHtml('').toEqual([ - ['Template', '0:16', '0:16', '16:30'], + ['Template', '0:30', '0:16', '16:30'], ['Reference', '13:15', ''], ]); }); it('is correct for reference with name', () => { expectFromHtml('').toEqual([ - ['Template', '0:20', '0:20', '20:34'], + ['Template', '0:34', '0:20', '20:34'], ['Reference', '13:19', '17:18'], ]); }); it('is correct for reference via ref-...', () => { expectFromHtml('').toEqual([ - ['Template', '0:19', '0:19', '19:33'], + ['Template', '0:33', '0:19', '19:33'], ['Reference', '13:18', ''], ]); }); it('is correct for variables via let-...', () => { expectFromHtml('').toEqual([ - ['Template', '0:23', '0:23', '23:37'], + ['Template', '0:37', '0:23', '23:37'], ['Variable', '13:22', '20:21'], ]); }); it('is correct for attributes', () => { expectFromHtml('').toEqual([ - ['Template', '0:21', '0:21', '21:35'], + ['Template', '0:35', '0:21', '21:35'], ['TextAttribute', '13:20', '17:19'], ]); }); it('is correct for bound attributes', () => { expectFromHtml('').toEqual([ - ['Template', '0:23', '0:23', '23:37'], + ['Template', '0:37', '0:23', '23:37'], ['BoundAttribute', '13:22', '19:21'], ]); }); @@ -236,7 +236,7 @@ describe('R3 AST source spans', () => { //
    //     expectFromHtml('
    ').toEqual([ - ['Template', '0:32', '0:32', '32:38'], + ['Template', '0:38', '0:32', '32:38'], ['TextAttribute', '5:31', ''], ['BoundAttribute', '5:31', '25:30'], // *ngFor="let item of items" -> items ['Variable', '13:22', ''], // let item @@ -250,7 +250,7 @@ describe('R3 AST source spans', () => { //
    // expectFromHtml('
    ').toEqual([ - ['Template', '0:28', '0:28', '28:34'], + ['Template', '0:34', '0:28', '28:34'], ['BoundAttribute', '5:27', '13:17'], // ngFor="item of items" -> item ['BoundAttribute', '5:27', '21:26'], // ngFor="item of items" -> items ['Element', '0:34', '0:28', '28:34'], @@ -259,7 +259,7 @@ describe('R3 AST source spans', () => { it('is correct for variables via let ...', () => { expectFromHtml('
    ').toEqual([ - ['Template', '0:21', '0:21', '21:27'], + ['Template', '0:27', '0:21', '21:27'], ['TextAttribute', '5:20', ''], ['Variable', '12:19', '18:19'], // let a=b -> b ['Element', '0:27', '0:21', '21:27'], @@ -268,7 +268,7 @@ describe('R3 AST source spans', () => { it('is correct for variables via as ...', () => { expectFromHtml('
    ').toEqual([ - ['Template', '0:27', '0:27', '27:33'], + ['Template', '0:33', '0:27', '27:33'], ['BoundAttribute', '5:26', '12:16'], // ngIf="expr as local" -> expr ['Variable', '6:25', '6:10'], // ngIf="expr as local -> ngIf ['Element', '0:33', '0:27', '27:33'], diff --git a/packages/compiler/test/template_parser/template_parser_spec.ts b/packages/compiler/test/template_parser/template_parser_spec.ts index 17f75b0957..d43238c102 100644 --- a/packages/compiler/test/template_parser/template_parser_spec.ts +++ b/packages/compiler/test/template_parser/template_parser_spec.ts @@ -2046,7 +2046,7 @@ Property binding a not used by any directive on an embedded template. Make sure it('should support embedded template', () => { expect(humanizeTplAstSourceSpans(parse('', []))).toEqual([ - [EmbeddedTemplateAst, ''] + [EmbeddedTemplateAst, ''] ]); }); @@ -2058,14 +2058,14 @@ Property binding a not used by any directive on an embedded template. Make sure it('should support references', () => { expect(humanizeTplAstSourceSpans(parse('
    ', []))).toEqual([ - [ElementAst, 'div', '
    '], [ReferenceAst, 'a', null, '#a'] + [ElementAst, 'div', '
    '], [ReferenceAst, 'a', null, '#a'] ]); }); it('should support variables', () => { expect(humanizeTplAstSourceSpans(parse('', []))) .toEqual([ - [EmbeddedTemplateAst, ''], + [EmbeddedTemplateAst, ''], [VariableAst, 'a', 'b', 'let-a="b"'], ]); }); @@ -2128,7 +2128,7 @@ Property binding a not used by any directive on an embedded template. Make sure expect(humanizeTplAstSourceSpans( parse('', [tagSel, attrSel]))) .toEqual([ - [ElementAst, ':svg:svg', ''], + [ElementAst, ':svg:svg', ''], [ElementAst, ':svg:circle', ''], [DirectiveAst, tagSel, ''], [ElementAst, ':svg:use', ''], @@ -2144,7 +2144,8 @@ Property binding a not used by any directive on an embedded template. Make sure inputs: ['aProp'] }).toSummary(); expect(humanizeTplAstSourceSpans(parse('
    ', [dirA]))).toEqual([ - [ElementAst, 'div', '
    '], [DirectiveAst, dirA, '
    '], + [ElementAst, 'div', '
    '], + [DirectiveAst, dirA, '
    '], [BoundDirectivePropertyAst, 'aProp', 'foo', '[aProp]="foo"'] ]); });