From 0462a616c367e7b0dd0a3095c68256d3c7815a5c Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Mon, 16 Nov 2020 15:02:38 +0000 Subject: [PATCH] fix(compiler): ensure that placeholders have the correct sourceSpan (#39717) When the `preserveWhitespaces` is not true, the template parser will process the parsed AST nodes to remove excess whitespace. Since the generated `goog.getMsg()` statements rely upon the AST nodes after this whitespace is removed, the i18n extraction must make a second pass. Previously this resulted in innacurrate source-spans for the i18n text and placeholder nodes that were extracted in the second pass. This commit fixes this by reusing the source-spans from the first pass when extracting the nodes in the second pass. Fixes #39671 PR Close #39717 --- .../test/ngtsc/template_mapping_spec.ts | 89 +++++++++++++++++++ packages/compiler/src/i18n/i18n_parser.ts | 46 +++++++++- 2 files changed, 132 insertions(+), 3 deletions(-) diff --git a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts index 506f58f019..017abd8190 100644 --- a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts @@ -427,6 +427,95 @@ runInEachFileSystem((os) => { }); }); + it('should correctly handle collapsed whitespace in interpolation placeholder source-mappings', + () => { + const mappings = compileAndMap( + `
pre-body {{body_value}} post-body
`); + expectMapping(mappings, { + source: '
', + generated: 'i0.ɵɵelementStart(0, "div", 0)', + sourceUrl: '../test.ts', + }); + expectMapping(mappings, { + source: '
', + generated: 'i0.ɵɵelementEnd()', + sourceUrl: '../test.ts', + }); + expectMapping(mappings, { + source: ' pre-body ', + generated: '` pre-body ${', + sourceUrl: '../test.ts', + }); + expectMapping(mappings, { + source: '{{body_value}}', + generated: '"\\uFFFD0\\uFFFD"', + sourceUrl: '../test.ts', + }); + expectMapping(mappings, { + source: ' post-body', + generated: '}:INTERPOLATION: post-body`', + sourceUrl: '../test.ts', + }); + }); + + it('should correctly handle collapsed whitespace in element placeholder source-mappings', + () => { + const mappings = + compileAndMap(`
\n pre-p\n

\n in-p\n

\n post-p\n
`); + // $localize expressions + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: 'pre-p\n ', + generated: '` pre-p ${', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: '

\n ', + generated: '"\\uFFFD#2\\uFFFD"', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: 'in-p\n ', + generated: '}:START_PARAGRAPH: in-p ${', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: '

\n ', + generated: '"\\uFFFD/#2\\uFFFD"', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: 'post-p\n', + generated: '}:CLOSE_PARAGRAPH: post-p\n`', + }); + // ivy instructions + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: '
\n ', + generated: 'i0.ɵɵelementStart(0, "div")', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: '
\n ', + generated: 'i0.ɵɵi18nStart(1, 0)', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: '

\n in-p\n

', + generated: 'i0.ɵɵelement(2, "p")', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: '
', + generated: 'i0.ɵɵi18nEnd()', + }); + expectMapping(mappings, { + sourceUrl: '../test.ts', + source: '
', + generated: 'i0.ɵɵelementEnd()', + }); + }); + it('should create tag (container) placeholder source-mappings', () => { const mappings = compileAndMap(`
Hello, World!
`); expectMapping(mappings, { diff --git a/packages/compiler/src/i18n/i18n_parser.ts b/packages/compiler/src/i18n/i18n_parser.ts index c22941f16f..4e85abfc49 100644 --- a/packages/compiler/src/i18n/i18n_parser.ts +++ b/packages/compiler/src/i18n/i18n_parser.ts @@ -105,12 +105,13 @@ class _I18nVisitor implements html.Visitor { } visitAttribute(attribute: html.Attribute, context: I18nMessageVisitorContext): i18n.Node { - const node = this._visitTextWithInterpolation(attribute.value, attribute.sourceSpan, context); + const node = this._visitTextWithInterpolation( + attribute.value, attribute.valueSpan || attribute.sourceSpan, context, attribute.i18n); return context.visitNodeFn(attribute, node); } visitText(text: html.Text, context: I18nMessageVisitorContext): i18n.Node { - const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context); + const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context, text.i18n); return context.visitNodeFn(text, node); } @@ -156,7 +157,8 @@ class _I18nVisitor implements html.Visitor { } private _visitTextWithInterpolation( - text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext): i18n.Node { + text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext, + previousI18n: i18n.I18nMeta|undefined): i18n.Node { const splitInterpolation = this._expressionParser.splitInterpolation( text, sourceSpan.start.toString(), this._interpolationConfig); @@ -197,10 +199,48 @@ class _I18nVisitor implements html.Visitor { getOffsetSourceSpan(sourceSpan, splitInterpolation.stringSpans[lastStringIdx]); nodes.push(new i18n.Text(splitInterpolation.strings[lastStringIdx], stringSpan)); } + + if (previousI18n instanceof i18n.Message) { + // The `previousI18n` is an i18n `Message`, so we are processing an `Attribute` with i18n + // metadata. The `Message` should consist only of a single `Container` that contains the + // parts (`Text` and `Placeholder`) to process. + assertSingleContainerMessage(previousI18n); + previousI18n = previousI18n.nodes[0]; + } + + if (previousI18n instanceof i18n.Container) { + // The `previousI18n` is a `Container`, which means that this is a second i18n extraction pass + // after whitespace has been removed from the AST ndoes. + assertEquivalentNodes(previousI18n.children, nodes); + + // Reuse the source-spans from the first pass. + for (let i = 0; i < nodes.length; i++) { + nodes[i].sourceSpan = previousI18n.children[i].sourceSpan; + } + } + return container; } } +function assertSingleContainerMessage(message: i18n.Message): void { + const nodes = message.nodes; + if (nodes.length !== 1 || !(nodes[0] instanceof i18n.Container)) { + throw new Error( + 'Unexpected previous i18n message - expected it to consist of only a single `Container` node.'); + } +} + +function assertEquivalentNodes(previousNodes: i18n.Node[], nodes: i18n.Node[]): void { + if (previousNodes.length !== nodes.length) { + throw new Error('The number of i18n message children changed between first and second pass.'); + } + if (previousNodes.some((node, i) => nodes[i].constructor !== node.constructor)) { + throw new Error( + 'The types of the i18n message children changed between first and second pass.'); + } +} + function getOffsetSourceSpan( sourceSpan: ParseSourceSpan, {start, end}: {start: number, end: number}): ParseSourceSpan { return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end));