From 5a0ff41f75fd6d58d73baf11315c012131d9ec82 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Thu, 18 Jul 2024 17:28:09 -0700 Subject: [PATCH] refactor(compiler): ensure context is always provided for `WhitespaceVisitor` (#56507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When disabling `i18nPreserveSignificantWhitespaceForLegacyExtraction` I was looking at a test case with ICU messages containing leading and trailing whitespace: ```angular
{apples, plural, =other {I have many apples.}}
``` This would historically generate two messages: ```javascript const MSG_TMP = goog.getMsg('{apples, plural, =other {I have many apples.}}'); const MSG_FOO = goog.getMsg(' {$ICU} ', { 'ICU': MSG_TMP }); ``` But I found that I was getting just one message: ```javascript const MSG_TMP = goog.getMsg(' {apples, plural, =other {I have many apples.}} '); ``` This is arguably an improvement, but changed the messages and message IDs, which isn't desirable with this option. I eventually traced this back to the `isIcu` initialization in [`i18n_parser.ts`](/packages/compiler/src/i18n/i18n_parser.ts): ```typescript const context: I18nMessageVisitorContext = { isIcu: nodes.length == 1 && nodes[0] instanceof html.Expansion, // ... }; ``` [`_I18nVisitor.prototype.visitExpansion`](/packages/compiler/src/i18n/i18n_parser.ts) uses this to decide whether or not to generate a sub-message for a given ICU expansion: ```typescript if (context.isIcu || context.icuDepth > 0) { // Returns an ICU node when: // - the message (vs a part of the message) is an ICU message, or // - the ICU message is nested. const expPh = context.placeholderRegistry.getUniquePlaceholder(`VAR_${icu.type}`); i18nIcu.expressionPlaceholder = expPh; context.placeholderToContent[expPh] = { text: icu.switchValue, sourceSpan: icu.switchValueSourceSpan, }; return context.visitNodeFn(icu, i18nIcu); } // Else returns a placeholder // ICU placeholders should not be replaced with their original content but with the their // translations. // TODO(vicb): add a html.Node -> i18n.Message cache to avoid having to re-create the msg const phName = context.placeholderRegistry.getPlaceholderName('ICU', icu.sourceSpan.toString()); context.placeholderToMessage[phName] = this.toI18nMessage([icu], '', '', '', undefined); const node = new i18n.IcuPlaceholder(i18nIcu, phName, icu.sourceSpan); return context.visitNodeFn(icu, node); ``` Note that `isIcu` is the key condition between these two cases and depends on whether or not the ICU expansion has any siblings. The introduction of `WhitespaceVisitor` to `I18nMetaVisitor` trims insignificant whitespace, including empty text nodes not adjacent to an ICU expansion (from [`WhitespaceVisitor.prototype.visitText`](/packages/compiler/src/ml_parser/html_whitespaces.ts)): ```typescript const isNotBlank = text.value.match(NO_WS_REGEXP); const hasExpansionSibling = context && (context.prev instanceof html.Expansion || context.next instanceof html.Expansion); if (isNotBlank || hasExpansionSibling) { // Transform node by trimming it... return trimmedNode; } return null; // Drop node which is empty and has no ICU expansion sibling. ``` `hasExpansionSibling` was intended to retain empty text nodes leading or trailing an ICU expansion, however `context` was `undefined`, so this check failed and the leading / trailing text nodes were dropped. This resulted in trimming the ICU text by dropping the leading / trailing whitespace nodes. Having only a single ICU expansion with no leading / trailing text nodes caused `_I18nVisitor` to initialize `isIcu` incorrectly and caused it to generate one message instead of two. `WhitespaceVisitor` is supposed to get this context from `visitAllWithSiblings`. So the fix here is to make sure `WhitespaceVisitor` is always visited via this function which provides the required context. I updated all usage sites to make sure this context is use consistently and implemented the `WhitespaceVisitor.prototype.visit` method to throw when the context is missing to make sure we don't encounter a similar mistake in the future. Unfortunately this broke one compliance test. Specifically the [`icu_logic/icu_only.js`](/home/douglasparker/Source/ng/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/icu_logic/icu_only.js) test which changed from generating: ```javascript function MyComponent_Template(rf, ctx) { if (rf & 1) { i0.ɵɵi18n(0, 0); } // ... } ``` To now generating: ```javascript function MyComponent_Template(rf, ctx) { if (rf & 1) { i0.ɵɵtext(0, " "); i0.ɵɵi18n(1, 0); i0.ɵɵtext(2, "\n"); } // ... } ``` This test uses the default value `preserveWhitespaces: false` (`i18nPreserveSignificantWhitespaceForLegacyExtraction` should not affect compiled JS output, we already retain significant whitespace there). So what this indicates to me is that ICU logic is already broken because it's not preserving significant whitespace in this case. My change is probably a bug fix, but one which would affect the compiled runtime, which is not in scope here. The root cause is because using `visitAllWithSiblings` everywhere means the context is retained correctly in this case and the whitespace is leading/trailing an ICU message, therefore it is retained per the logic of `WhitespaceVisitor.prototype.visitText` I mentioned eariler. To address this, I left one usage of `WhitespaceVisitor` using `html.visitAll` instead of `visitAllWithSiblings` to retain this bug. I has to lossen the assertion I put in `WhitespaceVisitor.prototype.visit` to make this possible, but it should still throw by default when misused, which is the important part. PR Close #56507 --- packages/compiler/src/i18n/message_bundle.ts | 5 ++--- .../src/ml_parser/html_whitespaces.ts | 19 ++++++++++++++++--- .../compiler/src/render3/view/i18n/meta.ts | 4 ++-- .../compiler/src/render3/view/template.ts | 13 ++++++++++++- packages/compiler/test/render3/view/util.ts | 4 ++-- 5 files changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/compiler/src/i18n/message_bundle.ts b/packages/compiler/src/i18n/message_bundle.ts index f3ed1faedc4..534eefb1dae 100644 --- a/packages/compiler/src/i18n/message_bundle.ts +++ b/packages/compiler/src/i18n/message_bundle.ts @@ -6,10 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import * as html from '../ml_parser/ast'; import {InterpolationConfig} from '../ml_parser/defaults'; import {HtmlParser} from '../ml_parser/html_parser'; -import {WhitespaceVisitor} from '../ml_parser/html_whitespaces'; +import {WhitespaceVisitor, visitAllWithSiblings} from '../ml_parser/html_whitespaces'; import {ParseError} from '../parse_util'; import {extractMessages} from './extractor_merger'; @@ -49,7 +48,7 @@ export class MessageBundle { // affected message IDs. const rootNodes = this._preserveWhitespace ? htmlParserResult.rootNodes - : html.visitAll( + : visitAllWithSiblings( new WhitespaceVisitor(/* preserveSignificantWhitespace */ false), htmlParserResult.rootNodes, ); diff --git a/packages/compiler/src/ml_parser/html_whitespaces.ts b/packages/compiler/src/ml_parser/html_whitespaces.ts index 3e6af5dc9e2..6813aff8d4b 100644 --- a/packages/compiler/src/ml_parser/html_whitespaces.ts +++ b/packages/compiler/src/ml_parser/html_whitespaces.ts @@ -63,6 +63,7 @@ export class WhitespaceVisitor implements html.Visitor { constructor( private readonly preserveSignificantWhitespace: boolean, private readonly originalNodeMap?: Map, + private readonly requireContext = true, ) {} visitElement(element: html.Element, context: any): any { @@ -71,7 +72,7 @@ export class WhitespaceVisitor implements html.Visitor { // but still visit all attributes to eliminate one used as a market to preserve WS const newElement = new html.Element( element.name, - html.visitAll(this, element.attrs), + visitAllWithSiblings(this, element.attrs), element.children, element.sourceSpan, element.startSourceSpan, @@ -202,6 +203,18 @@ export class WhitespaceVisitor implements html.Visitor { visitLetDeclaration(decl: html.LetDeclaration, context: any) { return decl; } + + visit(_node: html.Node, context: any) { + // `visitAllWithSiblings` provides context necessary for ICU messages to be handled correctly. + // Prefer that over calling `html.visitAll` directly on this visitor. + if (this.requireContext && !context) { + throw new Error( + `WhitespaceVisitor requires context. Visit via \`visitAllWithSiblings\` to get this context.`, + ); + } + + return false; + } } function trimLeadingWhitespace( @@ -261,7 +274,7 @@ export function removeWhitespaces( preserveSignificantWhitespace: boolean, ): ParseTreeResult { return new ParseTreeResult( - html.visitAll( + visitAllWithSiblings( new WhitespaceVisitor(preserveSignificantWhitespace), htmlAstWithErrors.rootNodes, ), @@ -274,7 +287,7 @@ interface SiblingVisitorContext { next: html.Node | undefined; } -function visitAllWithSiblings(visitor: WhitespaceVisitor, nodes: html.Node[]): any[] { +export function visitAllWithSiblings(visitor: WhitespaceVisitor, nodes: html.Node[]): any[] { const result: any[] = []; nodes.forEach((ast, i) => { diff --git a/packages/compiler/src/render3/view/i18n/meta.ts b/packages/compiler/src/render3/view/i18n/meta.ts index e1786d1668a..1c55eb12175 100644 --- a/packages/compiler/src/render3/view/i18n/meta.ts +++ b/packages/compiler/src/render3/view/i18n/meta.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {WhitespaceVisitor} from '../../../ml_parser/html_whitespaces'; +import {WhitespaceVisitor, visitAllWithSiblings} from '../../../ml_parser/html_whitespaces'; import {computeDecimalDigest, computeDigest, decimalDigest} from '../../../i18n/digest'; import * as i18n from '../../../i18n/i18n_ast'; import {createI18nMessageFactory, VisitNodeFn} from '../../../i18n/i18n_parser'; @@ -129,7 +129,7 @@ export class I18nMetaVisitor implements html.Visitor { const originalNodeMap = new Map(); const trimmedNodes = this.preserveSignificantWhitespace ? element.children - : html.visitAll( + : visitAllWithSiblings( new WhitespaceVisitor(false /* preserveSignificantWhitespace */, originalNodeMap), element.children, ); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 555396acdec..6fb920b10a3 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -237,8 +237,19 @@ export function parseTemplate( // and `$localize` calls which should retain significant whitespace in order to render the // correct output. We let this diverge from the message IDs generated earlier which might not // have preserved significant whitespace. + // + // This should use `visitAllWithSiblings` to set `WhitespaceVisitor` context correctly, however + // there is an existing bug where significant whitespace is not properly retained in the JS + // output of leading/trailing whitespace for ICU messages due to the existing lack of context\ + // in `WhitespaceVisitor`. Using `visitAllWithSiblings` here would fix that bug and retain the + // whitespace, however it would also change the runtime representation which we don't want to do + // right now. rootNodes = html.visitAll( - new WhitespaceVisitor(/* preserveSignificantWhitespace */ true), + new WhitespaceVisitor( + /* preserveSignificantWhitespace */ true, + /* originalNodeMap */ undefined, + /* requireContext */ false, + ), rootNodes, ); diff --git a/packages/compiler/test/render3/view/util.ts b/packages/compiler/test/render3/view/util.ts index 9281b7144da..041f084d6e5 100644 --- a/packages/compiler/test/render3/view/util.ts +++ b/packages/compiler/test/render3/view/util.ts @@ -12,7 +12,7 @@ import {Parser} from '../../../src/expression_parser/parser'; import * as html from '../../../src/ml_parser/ast'; import {DEFAULT_INTERPOLATION_CONFIG, InterpolationConfig} from '../../../src/ml_parser/defaults'; import {HtmlParser} from '../../../src/ml_parser/html_parser'; -import {WhitespaceVisitor} from '../../../src/ml_parser/html_whitespaces'; +import {WhitespaceVisitor, visitAllWithSiblings} from '../../../src/ml_parser/html_whitespaces'; import {ParseTreeResult} from '../../../src/ml_parser/parser'; import * as a from '../../../src/render3/r3_ast'; import {htmlAstToRender3Ast, Render3ParseResult} from '../../../src/render3/r3_template_transform'; @@ -164,7 +164,7 @@ export function parseR3( let htmlNodes = processI18nMeta(parseResult).rootNodes; if (!options.preserveWhitespaces) { - htmlNodes = html.visitAll( + htmlNodes = visitAllWithSiblings( new WhitespaceVisitor(true /* preserveSignificantWhitespace */), htmlNodes, );