From 9c8a1f8a71be9b8380afbdc61ee7ee60f24488b4 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Thu, 12 Aug 2021 14:31:19 +0100 Subject: [PATCH] fix(compiler): include leading whitespace in source-spans of i18n messages (#43132) Previously, the way templates were tokenized meant that we lost information about the location of interpolations if the template contained encoded HTML entities. This meant that the mapping back to the source interpolated strings could be offset incorrectly. Also, the source-span assigned to an i18n message did not include leading whitespace. This confused the output source-mappings so that the first text nodes of the message stopped at the first non-whitespace character. This commit makes use of the previous refactorings, where more fine grain information was provided in text tokens, to enable the parser to identify the location of the interpolations in the original source more accurately. Fixes #41034 PR Close #43132 --- .../GOLDEN_PARTIAL.js | 18 ++- .../legacy_enabled.js | 16 ++- .../legacy_enabled.ts | 11 +- .../i18n_message_element_whitespace.js | 10 +- ...i18n_message_element_whitespace_partial.js | 16 +-- .../test/ngtsc/template_mapping_spec.ts | 10 +- packages/compiler/src/i18n/i18n_parser.ts | 118 ++++++++---------- .../src/render3/view/i18n/localize_utils.ts | 7 +- .../compiler/test/render3/view/i18n_spec.ts | 2 +- 9 files changed, 112 insertions(+), 96 deletions(-) diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/GOLDEN_PARTIAL.js index c875e1fc24b..bfbf5afe806 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/GOLDEN_PARTIAL.js @@ -7,14 +7,28 @@ export class MyComponent { } MyComponent.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, deps: [], target: i0.ɵɵFactoryTarget.Component }); MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", ngImport: i0, template: ` -
Some Message
+
+
Some & message
+
+
Some & {{'interpolated' }} message
+
&
+
&"
+
+
`, isInline: true }); i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, decorators: [{ type: Component, args: [{ selector: 'my-component', template: ` -
Some Message
+
+
Some & message
+
+
Some & {{'interpolated' }} message
+
&
+
&"
+
+
` }] }] }); diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.js index 46587d0d12d..edaa4dac2ef 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.js @@ -1,5 +1,11 @@ -let $I18N_0$; -if (typeof ngI18nClosureMode !== "undefined" && ngI18nClosureMode) { … } -else { - $I18N_0$ = $localize `:␟ec93160d6d6a8822214060dd7938bf821c22b226␟6795333002533525253:Some Message`; -} +$localize `:␟82ec661067f503a3357ecc159b2128325e9208cd␟2908931752694090721:Some & attribute`; +… +$localize `:␟10adaf0ad7b8ba40200cd3c0e7c8d0f13280d522␟4700340487900776701:Some & message`; +… +$localize `:␟57ebd20267116c04cc1dbd7be0b73bf56484f45d␟2334195497629636162:Some & ${"\uFFFD0\uFFFD"}:INTERPOLATION: attribute`; +… +$localize `:␟28d558ca32556f1da67a333e3dada321a97212cd␟3204054277547499090:Some & ${"\uFFFD0\uFFFD"}:INTERPOLATION: message`; +… +$localize `:␟0b3dff7b9382b6217ac97c99f9b04df04381bfdd␟2406634758623728945:&`; +… +$localize `:␟25b7cbf210e59a931423097cb7f2e1b72991a687␟4156372478368653226:&"`; \ No newline at end of file diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.ts index 014baabeee6..85971f20037 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.ts +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.ts @@ -3,7 +3,14 @@ import {Component, NgModule} from '@angular/core'; @Component({ selector: 'my-component', template: ` -
Some Message
+
+
Some & message
+
+
Some & {{'interpolated' }} message
+
&
+
&"
+
+
` }) export class MyComponent { @@ -11,4 +18,4 @@ export class MyComponent { @NgModule({declarations: [MyComponent]}) export class MyModule { -} \ No newline at end of file +} diff --git a/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js b/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js index ec6e480f3d1..c396b26efec 100644 --- a/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js +++ b/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js @@ -1,19 +1,19 @@ -` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "pre-p\\n " +` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n pre-p\\n " … -"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "

\\n " +"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "

" … -}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "in-p\\n " +}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n in-p\\n " … "\uFFFD/#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "

" … -}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "post-p\\n" +}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "\\n post-p\\n" … i0.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts" "
" … i0.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts" "
" … -i0.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "

\\n " +i0.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "

" … i0.ɵɵi18nEnd() // SOURCE: "/i18n_message_element_whitespace.ts" "

" … diff --git a/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace_partial.js b/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace_partial.js index 77461f05ee3..ccdff3a1e4d 100644 --- a/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace_partial.js +++ b/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace_partial.js @@ -1,19 +1,19 @@ -$localize` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "pre-p\\n " +$localize` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n pre-p\\n " … -"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "

\\n " +"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "

" … -}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "in-p\\n " +}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n in-p\\n " … -"\uFFFD/#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "

\\n " +"\uFFFD/#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "

" … -}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "post-p\\n" +}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "\\n post-p\\n" … -.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts" "
\\n " +.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts" "
" … -.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts" "
\\n " +.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts" "
" … -.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "

\\n " +.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "

" … .ɵɵi18nEnd() // SOURCE: "/i18n_message_element_whitespace.ts" "

'" … diff --git a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts index 4eb0e9b473f..02cfcede57a 100644 --- a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts @@ -464,27 +464,27 @@ runInEachFileSystem((os) => { // $localize expressions expectMapping(mappings, { sourceUrl: '../test.ts', - source: 'pre-p\n ', + source: '\n pre-p\n ', generated: '` pre-p ${', }); expectMapping(mappings, { sourceUrl: '../test.ts', - source: '

\n ', + source: '

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

\n ', + source: '

', generated: '"\\uFFFD/#2\\uFFFD"', }); expectMapping(mappings, { sourceUrl: '../test.ts', - source: 'post-p\n', + source: '\n post-p\n', generated: '}:CLOSE_PARAGRAPH: post-p\n`', }); // ivy instructions diff --git a/packages/compiler/src/i18n/i18n_parser.ts b/packages/compiler/src/i18n/i18n_parser.ts index 67791e7c7b0..c24dd7c0a1d 100644 --- a/packages/compiler/src/i18n/i18n_parser.ts +++ b/packages/compiler/src/i18n/i18n_parser.ts @@ -7,10 +7,11 @@ */ import {Lexer as ExpressionLexer} from '../expression_parser/lexer'; -import {InterpolationPiece, Parser as ExpressionParser} from '../expression_parser/parser'; +import {Parser as ExpressionParser} from '../expression_parser/parser'; import * as html from '../ml_parser/ast'; import {getHtmlTagDefinition} from '../ml_parser/html_tags'; import {InterpolationConfig} from '../ml_parser/interpolation_config'; +import {Token, TokenType} from '../ml_parser/lexer'; import {ParseSourceSpan} from '../parse_util'; import * as i18n from './i18n_ast'; @@ -105,13 +106,18 @@ class _I18nVisitor implements html.Visitor { } visitAttribute(attribute: html.Attribute, context: I18nMessageVisitorContext): i18n.Node { - const node = this._visitTextWithInterpolation( - attribute.value, attribute.valueSpan || attribute.sourceSpan, context, attribute.i18n); + const node = attribute.valueTokens === undefined || attribute.valueTokens.length === 1 ? + new i18n.Text(attribute.value, attribute.valueSpan || attribute.sourceSpan) : + this._visitTextWithInterpolation( + attribute.valueTokens, 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, text.i18n); + const node = text.tokens.length === 1 ? + new i18n.Text(text.value, text.sourceSpan) : + this._visitTextWithInterpolation(text.tokens, text.sourceSpan, context, text.i18n); return context.visitNodeFn(text, node); } @@ -165,66 +171,54 @@ class _I18nVisitor implements html.Visitor { * @param previousI18n Any i18n metadata associated with this `text` from a previous pass. */ private _visitTextWithInterpolation( - text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext, + tokens: Token[], sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext, previousI18n: i18n.I18nMeta|undefined): i18n.Node { - const {strings, expressions} = this._expressionParser.splitInterpolation( - text, sourceSpan.start.toString(), this._interpolationConfig); - - // No expressions, return a single text. - if (expressions.length === 0) { - return new i18n.Text(text, sourceSpan); - } - // Return a sequence of `Text` and `Placeholder` nodes grouped in a `Container`. const nodes: i18n.Node[] = []; - for (let i = 0; i < strings.length - 1; i++) { - this._addText(nodes, strings[i], sourceSpan); - this._addPlaceholder(nodes, context, expressions[i], sourceSpan); + // We will only create a container if there are actually interpolations, + // so this flag tracks that. + let hasInterpolation = false; + for (const token of tokens) { + switch (token.type) { + case TokenType.INTERPOLATION: + case TokenType.ATTR_VALUE_INTERPOLATION: + hasInterpolation = true; + const expression = token.parts[1]; + const baseName = extractPlaceholderName(expression) || 'INTERPOLATION'; + const phName = context.placeholderRegistry.getPlaceholderName(baseName, expression); + context.placeholderToContent[phName] = { + text: token.parts.join(''), + sourceSpan: token.sourceSpan + }; + nodes.push(new i18n.Placeholder(expression, phName, token.sourceSpan)); + break; + default: + if (token.parts[0].length > 0) { + // This token is text or an encoded entity. + // If it is following on from a previous text node then merge it into that node + // Otherwise, if it is following an interpolation, then add a new node. + const previous = nodes[nodes.length - 1]; + if (previous instanceof i18n.Text) { + previous.value += token.parts[0]; + previous.sourceSpan = new ParseSourceSpan( + previous.sourceSpan.start, token.sourceSpan.end, previous.sourceSpan.fullStart, + previous.sourceSpan.details); + } else { + nodes.push(new i18n.Text(token.parts[0], token.sourceSpan)); + } + } + break; + } } - // The last index contains no expression - this._addText(nodes, strings[strings.length - 1], sourceSpan); - // Whitespace removal may have invalidated the interpolation source-spans. - reusePreviousSourceSpans(nodes, previousI18n); - - return new i18n.Container(nodes, sourceSpan); - } - - /** - * Create a new `Text` node from the `textPiece` and add it to the `nodes` collection. - * - * @param nodes The nodes to which the created `Text` node should be added. - * @param textPiece The text and relative span information for this `Text` node. - * @param interpolationSpan The span of the whole interpolated text. - */ - private _addText( - nodes: i18n.Node[], textPiece: InterpolationPiece, interpolationSpan: ParseSourceSpan): void { - if (textPiece.text.length > 0) { - // No need to add empty strings - const stringSpan = getOffsetSourceSpan(interpolationSpan, textPiece); - nodes.push(new i18n.Text(textPiece.text, stringSpan)); + if (hasInterpolation) { + // Whitespace removal may have invalidated the interpolation source-spans. + reusePreviousSourceSpans(nodes, previousI18n); + return new i18n.Container(nodes, sourceSpan); + } else { + return nodes[0]; } } - - /** - * Create a new `Placeholder` node from the `expression` and add it to the `nodes` collection. - * - * @param nodes The nodes to which the created `Text` node should be added. - * @param context The current context of the visitor, used to compute and store placeholders. - * @param expression The expression text and relative span information for this `Placeholder` - * node. - * @param interpolationSpan The span of the whole interpolated text. - */ - private _addPlaceholder( - nodes: i18n.Node[], context: I18nMessageVisitorContext, expression: InterpolationPiece, - interpolationSpan: ParseSourceSpan): void { - const sourceSpan = getOffsetSourceSpan(interpolationSpan, expression); - const baseName = extractPlaceholderName(expression.text) || 'INTERPOLATION'; - const phName = context.placeholderRegistry.getPlaceholderName(baseName, expression.text); - const text = this._interpolationConfig.start + expression.text + this._interpolationConfig.end; - context.placeholderToContent[phName] = {text, sourceSpan}; - nodes.push(new i18n.Placeholder(expression.text, phName, sourceSpan)); - } } /** @@ -247,7 +241,7 @@ function reusePreviousSourceSpans(nodes: i18n.Node[], previousI18n: i18n.I18nMet 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. + // after whitespace has been removed from the AST nodes. assertEquivalentNodes(previousI18n.children, nodes); // Reuse the source-spans from the first pass. @@ -282,14 +276,6 @@ function assertEquivalentNodes(previousNodes: i18n.Node[], nodes: i18n.Node[]): } } -/** - * Create a new `ParseSourceSpan` from the `sourceSpan`, offset by the `start` and `end` values. - */ -function getOffsetSourceSpan( - sourceSpan: ParseSourceSpan, {start, end}: InterpolationPiece): ParseSourceSpan { - return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end)); -} - const _CUSTOM_PH_EXP = /\/\/[\s\S]*i18n[\s\S]*\([\s\S]*ph[\s\S]*=[\s\S]*("|')([\s\S]*?)\1[\s\S]*\)/g; diff --git a/packages/compiler/src/render3/view/i18n/localize_utils.ts b/packages/compiler/src/render3/view/i18n/localize_utils.ts index 85b4178edf5..7579d3ebb88 100644 --- a/packages/compiler/src/render3/view/i18n/localize_utils.ts +++ b/packages/compiler/src/render3/view/i18n/localize_utils.ts @@ -35,7 +35,10 @@ class LocalizeSerializerVisitor implements i18n.Visitor { // Two literal pieces in a row means that there was some comment node in-between. context[context.length - 1].text += text.value; } else { - context.push(new o.LiteralPiece(text.value, text.sourceSpan)); + const sourceSpan = new ParseSourceSpan( + text.sourceSpan.fullStart, text.sourceSpan.end, text.sourceSpan.fullStart, + text.sourceSpan.details); + context.push(new o.LiteralPiece(text.value, sourceSpan)); } } @@ -90,7 +93,7 @@ function getSourceSpan(message: i18n.Message): ParseSourceSpan { const startNode = message.nodes[0]; const endNode = message.nodes[message.nodes.length - 1]; return new ParseSourceSpan( - startNode.sourceSpan.start, endNode.sourceSpan.end, startNode.sourceSpan.fullStart, + startNode.sourceSpan.fullStart, endNode.sourceSpan.end, startNode.sourceSpan.fullStart, startNode.sourceSpan.details); } diff --git a/packages/compiler/test/render3/view/i18n_spec.ts b/packages/compiler/test/render3/view/i18n_spec.ts index 170f48ca5d0..25a1a9c0d87 100644 --- a/packages/compiler/test/render3/view/i18n_spec.ts +++ b/packages/compiler/test/render3/view/i18n_spec.ts @@ -478,7 +478,7 @@ describe('serializeI18nMessageForLocalize', () => { expect(messageParts[3].text).toEqual(''); expect(messageParts[3].sourceSpan.toString()).toEqual(''); expect(messageParts[4].text).toEqual(' D'); - expect(messageParts[4].sourceSpan.toString()).toEqual('D'); + expect(messageParts[4].sourceSpan.toString()).toEqual(' D'); expect(placeHolders[0].text).toEqual('START_TAG_SPAN'); expect(placeHolders[0].sourceSpan.toString()).toEqual('');