refactor(compiler): ensure context is always provided for WhitespaceVisitor (#56507)

When disabling `i18nPreserveSignificantWhitespaceForLegacyExtraction` I was looking at a test case with ICU messages containing leading and trailing whitespace:

```angular
<div i18n>
  {apples, plural, =other {I have many apples.}}
</div>
```

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

@@ -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,
         );

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<html.Node, html.Node>,
+    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) => {

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<html.Node, html.Node>();
           const trimmedNodes = this.preserveSignificantWhitespace
             ? element.children
-            : html.visitAll(
+            : visitAllWithSiblings(
                 new WhitespaceVisitor(false /* preserveSignificantWhitespace */, originalNodeMap),
                 element.children,
               );

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,
     );
 

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,
     );