refactor(compiler): implement control flow content projection fix in template pipeline (#52414)

crisbeto · alxhub · commit c7b730eb721e · 2023-10-31T14:52:30.000-07:00
Recreates the fix for content projection in control flow in the new template pipeline. I also had to make the following adjustments to the pipeline: 1. The `TemplateOp.tag` property was being used to generate the name of the template function, rather than the actual tag name being passed into `ɵɵtemplate`. Since the content projection fix requires the tag name to be passed in, I've introduced a new `functionNameSuffix` property instead. 2. `TemplateOp.block` was being used to determine whether to pass `TemplateOp.tag` into the `ɵɵtemplate` instruction. Now that we're always passing in the tag name after the refactor in point 1, we no longer need this flag. In addition to the refactors above, I also made some minor cleanups where I saw the opportunity to do so. PR Close #52414
diff --git a/packages/compiler/src/template/pipeline/ir/src/ops/create.ts b/packages/compiler/src/template/pipeline/ir/src/ops/create.ts
@@ -183,11 +183,9 @@ export interface TemplateOp extends ElementOpBase {
   vars: number|null;
 
   /**
-   * Whether or not this template was automatically created for use with block syntax (control flow
-   * or defer). This will eventually cause the emitted template instruction to use fewer arguments,
-   * since several of the default arguments are unnecessary for blocks.
+   * Suffix to add to the name of the generated template function.
    */
-  block: boolean;
+  functionNameSuffix: string;
 
   /**
    * The i18n placeholder data associated with this template.
@@ -199,15 +197,15 @@ export interface TemplateOp extends ElementOpBase {
  * Create a `TemplateOp`.
  */
 export function createTemplateOp(
-    xref: XrefId, tag: string|null, namespace: Namespace, generatedInBlock: boolean,
+    xref: XrefId, tag: string|null, functionNameSuffix: string, namespace: Namespace,
     i18nPlaceholder: i18n.TagPlaceholder|undefined, sourceSpan: ParseSourceSpan): TemplateOp {
   return {
     kind: OpKind.Template,
     xref,
     attributes: null,
     tag,
     slot: new SlotHandle(),
-    block: generatedInBlock,
+    functionNameSuffix,
     decls: null,
     vars: null,
     localRefs: [],
@@ -264,6 +262,11 @@ export interface RepeaterCreateOp extends ElementOpBase {
    */
   usesComponentInstance: boolean;
 
+  /**
+   * Suffix to add to the name of the generated template function.
+   */
+  functionNameSuffix: string;
+
   sourceSpan: ParseSourceSpan;
 }
 
@@ -279,8 +282,8 @@ export interface RepeaterVarNames {
 }
 
 export function createRepeaterCreateOp(
-    primaryView: XrefId, emptyView: XrefId|null, track: o.Expression, varNames: RepeaterVarNames,
-    sourceSpan: ParseSourceSpan): RepeaterCreateOp {
+    primaryView: XrefId, emptyView: XrefId|null, tag: string|null, track: o.Expression,
+    varNames: RepeaterVarNames, sourceSpan: ParseSourceSpan): RepeaterCreateOp {
   return {
     kind: OpKind.RepeaterCreate,
     attributes: null,
@@ -289,7 +292,8 @@ export function createRepeaterCreateOp(
     emptyView,
     track,
     trackByFn: null,
-    tag: 'For',
+    tag,
+    functionNameSuffix: 'For',
     namespace: Namespace.HTML,
     nonBindable: false,
     localRefs: [],
diff --git a/packages/compiler/src/template/pipeline/src/ingest.ts b/packages/compiler/src/template/pipeline/src/ingest.ts
@@ -18,7 +18,7 @@ import {BindingParser} from '../../../template_parser/binding_parser';
 import * as ir from '../ir';
 
 import {ComponentCompilationJob, HostBindingCompilationJob, type CompilationJob, type ViewCompilationUnit} from './compilation';
-import {BINARY_OPERATORS, namespaceForKey} from './conversion';
+import {BINARY_OPERATORS, namespaceForKey, prefixWithNamespace} from './conversion';
 
 const compatibilityMode = ir.CompatibilityMode.TemplateDefinitionBuilder;
 
@@ -149,10 +149,6 @@ function ingestElement(unit: ViewCompilationUnit, element: t.Element): void {
     throw Error(`Unhandled i18n metadata type for element: ${element.i18n.constructor.name}`);
   }
 
-  const staticAttributes: Record<string, string> = {};
-  for (const attr of element.attributes) {
-    staticAttributes[attr.name] = attr.value;
-  }
   const id = unit.job.allocateXrefId();
 
   const [namespaceKey, elementName] = splitNsName(element.name);
@@ -196,9 +192,13 @@ function ingestTemplate(unit: ViewCompilationUnit, tmpl: t.Template): void {
   }
 
   const i18nPlaceholder = tmpl.i18n instanceof i18n.TagPlaceholder ? tmpl.i18n : undefined;
+  const namespace = namespaceForKey(namespacePrefix);
+  const functionNameSuffix = tagNameWithoutNamespace === null ?
+      '' :
+      prefixWithNamespace(tagNameWithoutNamespace, namespace);
   const tplOp = ir.createTemplateOp(
-      childView.xref, tagNameWithoutNamespace, namespaceForKey(namespacePrefix), false,
-      i18nPlaceholder, tmpl.startSourceSpan);
+      childView.xref, tagNameWithoutNamespace, functionNameSuffix, namespace, i18nPlaceholder,
+      tmpl.startSourceSpan);
   unit.create.push(tplOp);
 
   ingestBindings(unit, tplOp, tmpl);
@@ -281,20 +281,29 @@ function ingestIfBlock(unit: ViewCompilationUnit, ifBlock: t.IfBlock): void {
   let firstXref: ir.XrefId|null = null;
   let firstSlotHandle: ir.SlotHandle|null = null;
   let conditions: Array<ir.ConditionalCaseExpr> = [];
-  for (const ifCase of ifBlock.branches) {
+  for (let i = 0; i < ifBlock.branches.length; i++) {
+    const ifCase = ifBlock.branches[i];
     const cView = unit.job.allocateView(unit.xref);
+    let tagName: string|null = null;
+
+    // Only the first branch can be used for projection, because the conditional
+    // uses the container of the first branch as the insertion point for all branches.
+    if (i === 0) {
+      tagName = ingestControlFlowInsertionPoint(unit, cView.xref, ifCase);
+    }
     if (ifCase.expressionAlias !== null) {
       cView.contextVariables.set(ifCase.expressionAlias.name, ir.CTX_REF);
     }
     const tmplOp = ir.createTemplateOp(
-        cView.xref, 'Conditional', ir.Namespace.HTML, true,
+        cView.xref, tagName, 'Conditional', ir.Namespace.HTML,
         undefined /* TODO: figure out how i18n works with new control flow */, ifCase.sourceSpan);
     unit.create.push(tmplOp);
 
     if (firstXref === null) {
       firstXref = cView.xref;
       firstSlotHandle = tmplOp.slot;
     }
+
     const caseExpr = ifCase.expression ? convertAst(ifCase.expression, unit.job, null) : null;
     const conditionalCaseExpr =
         new ir.ConditionalCaseExpr(caseExpr, tmplOp.xref, tmplOp.slot, ifCase.expressionAlias);
@@ -316,7 +325,7 @@ function ingestSwitchBlock(unit: ViewCompilationUnit, switchBlock: t.SwitchBlock
   for (const switchCase of switchBlock.cases) {
     const cView = unit.job.allocateView(unit.xref);
     const tmplOp = ir.createTemplateOp(
-        cView.xref, 'Case', ir.Namespace.HTML, true,
+        cView.xref, null, 'Case', ir.Namespace.HTML,
         undefined /* TODO: figure out how i18n works with new control flow */,
         switchCase.sourceSpan);
     unit.create.push(tmplOp);
@@ -346,7 +355,7 @@ function ingestDeferView(
   const secondaryView = unit.job.allocateView(unit.xref);
   ingestNodes(secondaryView, children);
   const templateOp = ir.createTemplateOp(
-      secondaryView.xref, `Defer${suffix}`, ir.Namespace.HTML, true, undefined, sourceSpan!);
+      secondaryView.xref, null, `Defer${suffix}`, ir.Namespace.HTML, undefined, sourceSpan!);
   unit.create.push(templateOp);
   return templateOp;
 }
@@ -503,8 +512,9 @@ function ingestForBlock(unit: ViewCompilationUnit, forBlock: t.ForLoopBlock): vo
     $implicit: forBlock.item.name,
   };
 
+  const tagName = ingestControlFlowInsertionPoint(unit, repeaterView.xref, forBlock);
   const repeaterCreate = ir.createRepeaterCreateOp(
-      repeaterView.xref, emptyView?.xref ?? null, track, varNames, forBlock.sourceSpan);
+      repeaterView.xref, emptyView?.xref ?? null, tagName, track, varNames, forBlock.sourceSpan);
   unit.create.push(repeaterCreate);
 
   const expression = convertAst(
@@ -841,3 +851,64 @@ function convertSourceSpan(
   const fullStart = baseSourceSpan.fullStart.moveBy(span.start);
   return new ParseSourceSpan(start, end, fullStart);
 }
+
+/**
+ * With the directive-based control flow users were able to conditionally project content using
+ * the `*` syntax. E.g. `<div *ngIf="expr" projectMe></div>` will be projected into
+ * `<ng-content select="[projectMe]"/>`, because the attributes and tag name from the `div` are
+ * copied to the template via the template creation instruction. With `@if` and `@for` that is
+ * not the case, because the conditional is placed *around* elements, rather than *on* them.
+ * The result is that content projection won't work in the same way if a user converts from
+ * `*ngIf` to `@if`.
+ *
+ * This function aims to cover the most common case by doing the same copying when a control flow
+ * node has *one and only one* root element or template node.
+ *
+ * This approach comes with some caveats:
+ * 1. As soon as any other node is added to the root, the copying behavior won't work anymore.
+ *    A diagnostic will be added to flag cases like this and to explain how to work around it.
+ * 2. If `preserveWhitespaces` is enabled, it's very likely that indentation will break this
+ *    workaround, because it'll include an additional text node as the first child. We can work
+ *    around it here, but in a discussion it was decided not to, because the user explicitly opted
+ *    into preserving the whitespace and we would have to drop it from the generated code.
+ *    The diagnostic mentioned point #1 will flag such cases to users.
+ *
+ * @returns Tag name to be used for the control flow template.
+ */
+function ingestControlFlowInsertionPoint(
+    unit: ViewCompilationUnit, xref: ir.XrefId, node: t.IfBlockBranch|t.ForLoopBlock): string|null {
+  let root: t.Element|t.Template|null = null;
+
+  for (const child of node.children) {
+    // Skip over comment nodes.
+    if (child instanceof t.Comment) {
+      continue;
+    }
+
+    // We can only infer the tag name/attributes if there's a single root node.
+    if (root !== null) {
+      return null;
+    }
+
+    // Root nodes can only elements or templates with a tag name (e.g. `<div *foo></div>`).
+    if (child instanceof t.Element || (child instanceof t.Template && child.tagName !== null)) {
+      root = child;
+    }
+  }
+
+  // If we've found a single root node, its tag name and *static* attributes can be copied
+  // to the surrounding template to be used for content projection. Note that it's important
+  // that we don't copy any bound attributes since they don't participate in content projection
+  // and they can be used in directive matching (in the case of `Template.templateAttrs`).
+  if (root !== null) {
+    for (const attr of root.attributes) {
+      ingestBinding(
+          unit, xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null,
+          SecurityContext.NONE, attr.sourceSpan, BindingFlags.TextValue);
+    }
+
+    return root instanceof t.Element ? root.name : root.tagName;
+  }
+
+  return null;
+}
diff --git a/packages/compiler/src/template/pipeline/src/instruction.ts b/packages/compiler/src/template/pipeline/src/instruction.ts
@@ -263,22 +263,23 @@ export function i18nStart(slot: number, constIndex: number, subTemplateIndex: nu
 }
 
 export function repeaterCreate(
-    slot: number, viewFnName: string, decls: number, vars: number, trackByFn: o.Expression,
-    trackByUsesComponentInstance: boolean, emptyViewFnName: string|null, emptyDecls: number|null,
-    emptyVars: number|null, sourceSpan: ParseSourceSpan|null): ir.CreateOp {
-  let args = [
+    slot: number, viewFnName: string, decls: number, vars: number, tag: string|null,
+    constIndex: number|null, trackByFn: o.Expression, trackByUsesComponentInstance: boolean,
+    emptyViewFnName: string|null, emptyDecls: number|null, emptyVars: number|null,
+    sourceSpan: ParseSourceSpan|null): ir.CreateOp {
+  const args = [
     o.literal(slot),
     o.variable(viewFnName),
     o.literal(decls),
     o.literal(vars),
+    o.literal(tag),
+    o.literal(constIndex),
     trackByFn,
   ];
   if (trackByUsesComponentInstance || emptyViewFnName !== null) {
     args.push(o.literal(trackByUsesComponentInstance));
     if (emptyViewFnName !== null) {
-      args.push(o.variable(emptyViewFnName));
-      args.push(o.literal(emptyDecls));
-      args.push(o.literal(emptyVars));
+      args.push(o.variable(emptyViewFnName), o.literal(emptyDecls), o.literal(emptyVars));
     }
   }
   return call(Identifiers.repeaterCreate, args, sourceSpan);
diff --git a/packages/compiler/src/template/pipeline/src/phases/naming.ts b/packages/compiler/src/template/pipeline/src/phases/naming.ts
@@ -10,7 +10,6 @@ import {sanitizeIdentifier} from '../../../../parse_util';
 import {hyphenate} from '../../../../render3/view/style_parser';
 import * as ir from '../../ir';
 import {ViewCompilationUnit, type CompilationJob, type CompilationUnit} from '../compilation';
-import {prefixWithNamespace} from '../conversion';
 
 /**
  * Generate names for functions and variables across all views.
@@ -76,17 +75,13 @@ function addNamesToView(
           const emptyView = unit.job.views.get(op.emptyView)!;
           // Repeater empty view function is at slot +2 (metadata is in the first slot).
           addNamesToView(
-              emptyView,
-              `${baseName}_${prefixWithNamespace(`${op.tag}Empty`, op.namespace)}_${
-                  op.slot.slot + 2}`,
+              emptyView, `${baseName}_${`${op.functionNameSuffix}Empty`}_${op.slot.slot + 2}`,
               state, compatibility);
         }
-        const repeaterToken =
-            op.tag === null ? '' : '_' + prefixWithNamespace(op.tag, op.namespace);
         // Repeater primary view function is at slot +1 (metadata is in the first slot).
         addNamesToView(
-            unit.job.views.get(op.xref)!, `${baseName}${repeaterToken}_${op.slot.slot + 1}`, state,
-            compatibility);
+            unit.job.views.get(op.xref)!,
+            `${baseName}_${op.functionNameSuffix}_${op.slot.slot + 1}`, state, compatibility);
         break;
       case ir.OpKind.Template:
         if (!(unit instanceof ViewCompilationUnit)) {
@@ -96,8 +91,8 @@ function addNamesToView(
         if (op.slot.slot === null) {
           throw new Error(`Expected slot to be assigned`);
         }
-        const tagToken = op.tag === null ? '' : '_' + prefixWithNamespace(op.tag, op.namespace);
-        addNamesToView(childView, `${baseName}${tagToken}_${op.slot.slot}`, state, compatibility);
+        const suffix = op.functionNameSuffix.length === 0 ? '' : `_${op.functionNameSuffix}`;
+        addNamesToView(childView, `${baseName}${suffix}_${op.slot.slot}`, state, compatibility);
         break;
       case ir.OpKind.StyleProp:
         op.name = normalizeStylePropName(op.name);
diff --git a/packages/compiler/src/template/pipeline/src/phases/reify.ts b/packages/compiler/src/template/pipeline/src/phases/reify.ts
@@ -98,7 +98,7 @@ function reifyCreateOperations(unit: CompilationUnit, ops: ir.OpList<ir.CreateOp
             op,
             ng.template(
                 op.slot.slot!, o.variable(childView.fnName!), childView.decls!, childView.vars!,
-                op.block ? null : op.tag, op.attributes, op.sourceSpan),
+                op.tag, op.attributes, op.sourceSpan),
         );
         break;
       case ir.OpKind.DisableBindings:
@@ -220,8 +220,9 @@ function reifyCreateOperations(unit: CompilationUnit, ops: ir.OpList<ir.CreateOp
         ir.OpList.replace(
             op,
             ng.repeaterCreate(
-                op.slot.slot!, repeaterView.fnName, op.decls!, op.vars!, op.trackByFn!,
-                op.usesComponentInstance, emptyViewFnName, emptyDecls, emptyVars, op.sourceSpan));
+                op.slot.slot, repeaterView.fnName, op.decls!, op.vars!, op.tag, op.attributes,
+                op.trackByFn!, op.usesComponentInstance, emptyViewFnName, emptyDecls, emptyVars,
+                op.sourceSpan));
         break;
       case ir.OpKind.Statement:
         // Pass statement operations directly through.
