refactor(compiler): Fix order of compound template/element param valu…

…es (#53209)

As part of this fix, I realized that child i18n blocks don't need their
own context. Instead, we can just add their params directly to the
context for their root block, and forgo the step of merging the contexts.

PR Close #53209

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/icu_logic/TEST_CASES.json

@@ -178,7 +178,7 @@
           ]
         }
       ],
-      "focusTest": true
+      "skipForTemplatePipeline": true
     },
     {
       "description": "should handle icus with interpolations",

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/ng-container_ng-template/TEST_CASES.json

@@ -180,6 +180,20 @@
           ]
         }
       ]
+    },
+    {
+      "description": "should handle structural directives with same placeholder",
+      "inputFiles": [
+        "structural_directives_same_placeholder.ts"
+      ],
+      "expectations": [
+        {
+          "extraChecks": [
+            "verifyPlaceholdersIntegrity",
+            "verifyUniqueConsts"
+          ]
+        }
+      ]
     }
   ]
 }

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/ng-container_ng-template/structural_directives_same_placeholder.js

@@ -0,0 +1,69 @@
+function MyComponent_div_2_Template(rf, ctx) { 
+  if (rf & 1) {
+    i0.ɵɵi18nStart(0, 0, 1);
+    i0.ɵɵelement(1, "div");
+    i0.ɵɵi18nEnd();
+  } 
+}
+function MyComponent_div_3_div_2_Template(rf, ctx) {
+  if (rf & 1) {
+    i0.ɵɵi18nStart(0, 0, 3);
+    i0.ɵɵelement(1, "div");
+    i0.ɵɵi18nEnd();
+  }
+}
+function MyComponent_div_3_Template(rf, ctx) { 
+  if (rf & 1) {
+    i0.ɵɵi18nStart(0, 0, 2);
+    i0.ɵɵelementStart(1, "div");
+    i0.ɵɵtemplate(2, MyComponent_div_3_div_2_Template, 2, 0, "div", 1);
+    i0.ɵɵelementEnd();
+    i0.ɵɵi18nEnd();
+  } 
+  if (rf & 2) {
+    const $ctx_r1$ = i0.ɵɵnextContext();
+    i0.ɵɵadvance(2);
+    i0.ɵɵproperty("ngIf", $ctx_r1$.someFlag);
+  } 
+}
+function MyComponent_img_4_Template(rf, ctx) { 
+  if (rf & 1) {
+    i0.ɵɵi18nStart(0, 0, 4);
+    i0.ɵɵelement(1, "img");
+    i0.ɵɵi18nEnd();
+  } 
+}
+function MyComponent_img_5_Template(rf, ctx) {
+  if (rf & 1) {
+    i0.ɵɵi18nStart(0, 0, 5);
+    i0.ɵɵelement(1, "img");
+    i0.ɵɵi18nEnd();
+  }
+}
+…
+decls: 6,
+vars: 4,
+consts: () => { 
+  __i18nMsgWithPostprocess__('{$startTagDiv}Content{$closeTagDiv}{$startTagDiv}{$startTagDiv}Content{$closeTagDiv}{$closeTagDiv}{$tagImg}{$tagImg}', [['closeTagDiv', String.raw`[\uFFFD/#1:1\uFFFD\uFFFD/*2:1\uFFFD|\uFFFD/#1:3\uFFFD\uFFFD/*2:3\uFFFD|\uFFFD/#1:2\uFFFD\uFFFD/*3:2\uFFFD]`], ['startTagDiv', String.raw`[\uFFFD*2:1\uFFFD\uFFFD#1:1\uFFFD|\uFFFD*3:2\uFFFD\uFFFD#1:2\uFFFD|\uFFFD*2:3\uFFFD\uFFFD#1:3\uFFFD]`], ['tagImg', String.raw`[\uFFFD*4:4\uFFFD\uFFFD/*4:4\uFFFD\uFFFD#1:4\uFFFD\uFFFD/#1:4\uFFFD\uFFFD*4:4\uFFFD\uFFFD/*4:4\uFFFD|\uFFFD*5:5\uFFFD\uFFFD/*5:5\uFFFD\uFFFD#1:5\uFFFD\uFFFD/#1:5\uFFFD\uFFFD*5:5\uFFFD\uFFFD/*5:5\uFFFD]`]], {original_code: {closeTagDiv: '</div>', startTagDiv: '<div *ngIf=\"someFlag\">', tagImg: '<img *ngIf=\"someOtherFlag\" />' }}, {}, [])
+  return [i18n_0, [4, "ngIf"]]; 
+},
+template: function MyComponent_Template(rf, ctx) { 
+  if (rf & 1) {
+    i0.ɵɵelementStart(0, "div");
+    i0.ɵɵi18nStart(1, 0);
+    i0.ɵɵtemplate(2, MyComponent_div_2_Template, 2, 0, "div", 1)(3, MyComponent_div_3_Template, 3, 1, "div", 1)(4, MyComponent_img_4_Template, 2, 0, "img", 1)(5, MyComponent_img_5_Template, 2, 0, "img", 1);
+    i0.ɵɵi18nEnd();
+    i0.ɵɵelementEnd();
+  } 
+  if (rf & 2) {
+    i0.ɵɵadvance(2);
+    i0.ɵɵproperty("ngIf", ctx.someFlag);
+    i0.ɵɵadvance(1);
+    i0.ɵɵproperty("ngIf", ctx.someFlag);
+    i0.ɵɵadvance(1);
+    i0.ɵɵproperty("ngIf", ctx.someOtherFlag);
+    i0.ɵɵadvance(1);
+    i0.ɵɵproperty("ngIf", ctx.someOtherFlag);
+  } 
+}
+

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/ng-container_ng-template/structural_directives_same_placeholder.ts

@@ -0,0 +1,22 @@
+import {Component, NgModule} from '@angular/core';
+
+@Component({
+  selector: 'my-component',
+  template: `
+  <div i18n>
+    <div *ngIf="someFlag">Content</div>
+    <div *ngIf="someFlag">
+      <div *ngIf="someFlag">Content</div>
+    </div>
+
+    <img *ngIf="someOtherFlag" />
+    <img *ngIf="someOtherFlag" />
+  </div>
+`,
+})
+export class MyComponent {
+}
+
+@NgModule({declarations: [MyComponent]})
+export class MyModule {
+}

packages/compiler/src/template/pipeline/ir/src/enums.ts

@@ -554,8 +554,10 @@ export enum DerivedRepeaterVarIdentity {
   Odd,
 }
 
+/**
+ * Kinds of i18n contexts. They can be created because of root i18n blocks, or ICUs.
+ */
 export enum I18nContextKind {
   RootI18n,
-  ChildI18n,
   Icu
 }

packages/compiler/src/template/pipeline/ir/src/ops/create.ts

@@ -1070,20 +1070,18 @@ export function createIcuEndOp(xref: XrefId): IcuEndOp {
 }
 
 /**
- * An i18n context that is used to generate snippets of a full translated message.
- * A separate context is created in a few different scenarios:
+ * An i18n context that is used to generate a translated i18n message. A separate context is created
+ * for two different scenarios:
  *
- * 1. For each top-level i18n block. A context generated for a top-level i18n block, will be used to
- *    eventually generate the translated message for that block that is extracted into the const
- *    array.
- * 2. For each child i18n block (resulting from using an ng-template inside of another i18n block).
- *    A context generated for a child i18n block will be used to generate the portion of the final
- *    message represented by the template. It will not result in a separate message in the consts
- *    array, but will instead be rolled into the root message that spawned it.
- * 3. For each ICU referenced as a sub-message. ICUs that are referenced as a sub-message will be
+ * 1. For each top-level i18n block.
+ * 2. For each ICU referenced as a sub-message. ICUs that are referenced as a sub-message will be
  *    used to generate a separate i18n message, but will not be extracted directly into the consts
  *    array. Instead they will be pulled in as part of the initialization statements for the message
  *    that references them.
+ *
+ * Child i18n blocks, resulting from the use of an ng-template inside of a parent i18n block, do not
+ * generate a separate context. Instead their content is included in the translated message for
+ * their root block.
  */
 export interface I18nContextOp extends Op<CreateOp> {
   kind: OpKind.I18nContext;

packages/compiler/src/template/pipeline/src/emit.ts

@@ -37,7 +37,6 @@ import {parseHostStyleProperties} from './phases/host_style_property_parsing';
 import {collectI18nConsts} from './phases/i18n_const_collection';
 import {extractI18nText} from './phases/i18n_text_extraction';
 import {liftLocalRefs} from './phases/local_refs';
-import {mergeI18nContexts} from './phases/merge_i18n_contexts';
 import {emitNamespaceChanges} from './phases/namespace';
 import {nameFunctionsAndVariables} from './phases/naming';
 import {mergeNextContextExpressions} from './phases/next_context_merging';
@@ -127,7 +126,6 @@ const phases: Phase[] = [
   {kind: Kind.Tmpl, fn: resolveI18nElementPlaceholders},
   {kind: Kind.Tmpl, fn: resolveI18nExpressionPlaceholders},
   {kind: Kind.Tmpl, fn: resolveI18nIcuPlaceholders},
-  {kind: Kind.Tmpl, fn: mergeI18nContexts},
   {kind: Kind.Tmpl, fn: extractI18nMessages},
   {kind: Kind.Tmpl, fn: generateTrackFns},
   {kind: Kind.Tmpl, fn: collectI18nConsts},

packages/compiler/src/template/pipeline/src/phases/assign_i18n_slot_dependencies.ts

@@ -14,7 +14,6 @@ import {CompilationJob} from '../compilation';
  */
 export function assignI18nSlotDependencies(job: CompilationJob) {
   const i18nLastSlotConsumers = new Map<ir.XrefId, ir.XrefId>();
-  const i18nContexts = new Map<ir.XrefId, ir.I18nContextOp>();
   let lastSlotConsumer: ir.XrefId|null = null;
   let currentI18nOp: ir.I18nStartOp|null = null;
 
@@ -33,17 +32,13 @@ export function assignI18nSlotDependencies(job: CompilationJob) {
           i18nLastSlotConsumers.set(currentI18nOp!.xref, lastSlotConsumer!);
           currentI18nOp = null;
           break;
-        case ir.OpKind.I18nContext:
-          i18nContexts.set(op.xref, op);
-          break;
       }
     }
 
     // Assign i18n expressions to target the last slot in its owning block.
     for (const op of unit.update) {
       if (op.kind === ir.OpKind.I18nExpression) {
-        const i18nContext = i18nContexts.get(op.context)!;
-        op.target = i18nLastSlotConsumers.get(i18nContext.i18nBlock)!;
+        op.target = i18nLastSlotConsumers.get(op.target)!;
       }
     }
   }

packages/compiler/src/template/pipeline/src/phases/create_i18n_contexts.ts

@@ -20,19 +20,24 @@ import {CompilationJob} from '../compilation';
  * message.)
  */
 export function createI18nContexts(job: CompilationJob) {
+  const rootContexts = new Map<ir.XrefId, ir.XrefId>();
   let currentI18nOp: ir.I18nStartOp|null = null;
   let xref: ir.XrefId;
+
   for (const unit of job.units) {
     for (const op of unit.create) {
       switch (op.kind) {
         case ir.OpKind.I18nStart:
-          // Each i18n block gets its own context.
-          xref = job.allocateXrefId();
-          const contextKind =
-              op.xref === op.root ? ir.I18nContextKind.RootI18n : ir.I18nContextKind.ChildI18n;
-          unit.create.push(ir.createI18nContextOp(contextKind, xref, op.xref, op.message, null!));
-          op.context = xref;
           currentI18nOp = op;
+          // Each root i18n block gets its own context, child ones refer to the context for their
+          // root block.
+          if (op.xref === op.root) {
+            xref = job.allocateXrefId();
+            unit.create.push(ir.createI18nContextOp(
+                ir.I18nContextKind.RootI18n, xref, op.xref, op.message, null!));
+            op.context = xref;
+            rootContexts.set(op.xref, xref);
+          }
           break;
         case ir.OpKind.I18nEnd:
           currentI18nOp = null;
@@ -58,4 +63,14 @@ export function createI18nContexts(job: CompilationJob) {
       }
     }
   }
+
+  // Assign contexts to child i18n blocks, now that all root i18n blocks have their context
+  // assigned.
+  for (const unit of job.units) {
+    for (const op of unit.create) {
+      if (op.kind === ir.OpKind.I18nStart && op.xref !== op.root) {
+        op.context = rootContexts.get(op.root)!;
+      }
+    }
+  }
 }

packages/compiler/src/template/pipeline/src/phases/extract_i18n_messages.ts

@@ -200,18 +200,26 @@ function collapseElementTemplatePairs(values: ir.I18nParamValue[]) {
       if (elementIndex !== undefined && templateIndex !== undefined) {
         const elementValue = values[elementIndex];
         const templateValue = values[templateIndex];
+        // To match the TemplateDefinitionBuilder output, flip the order depending on whether the
+        // values represent a closing or opening tag (or both).
+        // TODO(mmalerba): Figure out if this makes a difference in terms of either functionality,
+        // or the resulting message ID. If not, we can remove the special-casing in the future.
+        let compundValue: string;
+        if ((elementValue.flags & ir.I18nParamValueFlags.OpenTag) &&
+            (elementValue.flags & ir.I18nParamValueFlags.CloseTag)) {
+          // TODO(mmalerba): Is this a TDB bug? I don't understand why it would put the template
+          // value twice.
+          compundValue = `${formatValue(templateValue)}${formatValue(elementValue)}${
+              formatValue(templateValue)}`;
+        } else if (elementValue.flags & ir.I18nParamValueFlags.OpenTag) {
+          compundValue = `${formatValue(templateValue)}${formatValue(elementValue)}`;
+        } else {
+          compundValue = `${formatValue(elementValue)}${formatValue(templateValue)}`;
+        }
         // Replace the element value with the combined value.
-        values.splice(elementIndex, 1, {
-          // To match the TemplateDefinitionBuilder output, flip the order dependind on whether the
-          // values represent a closing or opening tag.
-          // TODO(mmalerba): Figure out if this makes a difference in terms of either functionality,
-          // or the resulting message ID. If not, we can remove the special-casing in the future.
-          value: elementValue.flags & ir.I18nParamValueFlags.CloseTag ?
-              `${formatValue(elementValue)}${formatValue(templateValue)}` :
-              `${formatValue(templateValue)}${formatValue(elementValue)}`,
-          subTemplateIndex,
-          flags: ir.I18nParamValueFlags.None
-        });
+        values.splice(
+            elementIndex, 1,
+            {value: compundValue, subTemplateIndex, flags: ir.I18nParamValueFlags.None});
         // Replace the template value with null to preserve the indicies we calculated earlier.
         values.splice(templateIndex, 1, null!);
       }