fix(compiler-cli): add diagnostic for control flow that prevents content projection (#53190)

crisbeto · pkozlowski-opensource · commit 4c1d69e2880f · 2023-11-28T11:18:43.000+01:00
This is a follow-up to the fix from #52414. It adds a diagnostic that will tell users when a control flow is preventing its direct descendants from being projected into a specific component slot. PR Close #53190
diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md
@@ -25,6 +25,7 @@ export enum ErrorCode {
     // (undocumented)
     CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002,
     CONFLICTING_INPUT_TRANSFORM = 2020,
+    CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 8011,
     // (undocumented)
     DECORATOR_ARG_NOT_LITERAL = 1001,
     // (undocumented)
diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
@@ -286,6 +286,21 @@ export enum ErrorCode {
    */
   INACCESSIBLE_DEFERRED_TRIGGER_ELEMENT = 8010,
 
+  /**
+   * A control flow node is projected at the root of a component and is preventing its direct
+   * descendants from being projected, because it has more than one root node.
+   *
+   * ```
+   * <comp>
+   *  @if (expr) {
+   *    <div projectsIntoSlot></div>
+   *    Text preventing the div from being projected
+   *  }
+   * </comp>
+   * ```
+   */
+  CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 8011,
+
   /**
    * A two way binding in a template has an incorrect syntax,
    * parentheses outside brackets. For example:
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {BindingPipe, PropertyRead, PropertyWrite, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler';
+import {BindingPipe, PropertyRead, PropertyWrite, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler';
 import ts from 'typescript';
 
 import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '../../diagnostics';
@@ -100,6 +100,14 @@ export interface OutOfBandDiagnosticRecorder {
       templateId: TemplateId,
       trigger: TmplAstHoverDeferredTrigger|TmplAstInteractionDeferredTrigger|
       TmplAstViewportDeferredTrigger): void;
+
+  /**
+   * Reports cases where control flow nodes prevent content projection.
+   */
+  controlFlowPreventingContentProjection(
+      templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string,
+      slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock,
+      preservesWhitespaces: boolean): void;
 }
 
 export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@@ -340,6 +348,34 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
         ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.INACCESSIBLE_DEFERRED_TRIGGER_ELEMENT),
         message));
   }
+
+  controlFlowPreventingContentProjection(
+      templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string,
+      slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock,
+      preservesWhitespaces: boolean): void {
+    const blockName = controlFlowNode instanceof TmplAstIfBlockBranch ? '@if' : '@for';
+    const lines = [
+      `Node matches the "${slotSelector}" slot of the "${
+          componentName}" component, but will not be projected into the specific slot because the surrounding ${
+          blockName} has more than one node at its root. To project the node in the right slot, you can:\n`,
+      `1. Wrap the content of the ${blockName} block in an <ng-container/> that matches the "${
+          slotSelector}" selector.`,
+      `2. Split the content of the ${blockName} block across multiple ${
+          blockName} blocks such that each one only has a single projectable node at its root.`,
+      `3. Remove all content from the ${blockName} block, except for the node being projected.`
+    ];
+
+    if (preservesWhitespaces) {
+      lines.push(
+          `Note: the host component has \`preserveWhitespaces: true\` which may ` +
+          `cause whitespace to affect content projection.`);
+    }
+
+    this._diagnostics.push(makeTemplateDiagnostic(
+        templateId, this.resolver.getSourceMapping(templateId), projectionNode.startSourceSpan,
+        ts.DiagnosticCategory.Warning,
+        ngErrorCode(ErrorCode.CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION), lines.join('\n')));
+  }
 }
 
 function makeInlineDiagnostic(
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AST, BindingPipe, BindingType, BoundTarget, Call, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler';
+import {AST, BindingPipe, BindingType, BoundTarget, Call, createCssSelectorFromNode, CssSelector, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, SelectorMatcher, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstText, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler';
 import ts from 'typescript';
 
 import {Reference} from '../../imports';
@@ -905,6 +905,100 @@ class TcbDomSchemaCheckerOp extends TcbOp {
 }
 
 
+/**
+ * A `TcbOp` that finds and flags control flow nodes that interfere with content projection.
+ *
+ * Context:
+ * `@if` and `@for` try to emulate the content projection behavior of `*ngIf` and `*ngFor`
+ * in order to reduce breakages when moving from one syntax to the other (see #52414), however the
+ * approach only works if there's only one element at the root of the control flow expression.
+ * This means that a stray sibling node (e.g. text) can prevent an element from being projected
+ * into the right slot. The purpose of the `TcbOp` is to find any places where a node at the root
+ * of a control flow expression *would have been projected* into a specific slot, if the control
+ * flow node didn't exist.
+ */
+class TcbControlFlowContentProjectionOp extends TcbOp {
+  constructor(
+      private tcb: Context, private element: TmplAstElement, private ngContentSelectors: string[],
+      private componentName: string) {
+    super();
+  }
+
+  override readonly optional = false;
+
+  override execute(): null {
+    const controlFlowToCheck = this.findPotentialControlFlowNodes();
+
+    if (controlFlowToCheck.length > 0) {
+      const matcher = new SelectorMatcher<string>();
+
+      for (const selector of this.ngContentSelectors) {
+        // `*` is a special selector for the catch-all slot.
+        if (selector !== '*') {
+          matcher.addSelectables(CssSelector.parse(selector), selector);
+        }
+      }
+
+      for (const root of controlFlowToCheck) {
+        for (const child of root.children) {
+          if (child instanceof TmplAstElement || child instanceof TmplAstTemplate) {
+            matcher.match(createCssSelectorFromNode(child), (_, originalSelector) => {
+              this.tcb.oobRecorder.controlFlowPreventingContentProjection(
+                  this.tcb.id, child, this.componentName, originalSelector, root,
+                  this.tcb.hostPreserveWhitespaces);
+            });
+          }
+        }
+      }
+    }
+
+    return null;
+  }
+
+  private findPotentialControlFlowNodes() {
+    const result: Array<TmplAstIfBlockBranch|TmplAstForLoopBlock> = [];
+
+    for (const child of this.element.children) {
+      let eligibleNode: TmplAstForLoopBlock|TmplAstIfBlockBranch|null = null;
+
+      // Only `@for` blocks and the first branch of `@if` blocks participate in content projection.
+      if (child instanceof TmplAstForLoopBlock) {
+        eligibleNode = child;
+      } else if (child instanceof TmplAstIfBlock) {
+        eligibleNode = child.branches[0];  // @if blocks are guaranteed to have at least one branch.
+      }
+
+      // Skip nodes with less than two children since it's impossible
+      // for them to run into the issue that we're checking for.
+      if (eligibleNode === null || eligibleNode.children.length < 2) {
+        continue;
+      }
+
+      // Count the number of root nodes while skipping empty text where relevant.
+      const rootNodeCount = eligibleNode.children.reduce((count, node) => {
+        // Normally `preserveWhitspaces` would have been accounted for during parsing, however
+        // in `ngtsc/annotations/component/src/resources.ts#parseExtractedTemplate` we enable
+        // `preserveWhitespaces` to preserve the accuracy of source maps diagnostics. This means
+        // that we have to account for it here since the presence of text nodes affects the
+        // content projection behavior.
+        if (!(node instanceof TmplAstText) || this.tcb.hostPreserveWhitespaces ||
+            node.value.trim().length > 0) {
+          count++;
+        }
+
+        return count;
+      }, 0);
+
+      // Content projection can only be affected if there is more than one root node.
+      if (rootNodeCount > 1) {
+        result.push(eligibleNode);
+      }
+    }
+
+    return result;
+  }
+}
+
 /**
  * Mapping between attributes names that don't correspond to their element property names.
  * Note: this mapping has to be kept in sync with the equally named mapping in the runtime.
@@ -1838,6 +1932,7 @@ class Scope {
     if (node instanceof TmplAstElement) {
       const opIndex = this.opQueue.push(new TcbElementOp(this.tcb, this, node)) - 1;
       this.elementOpMap.set(node, opIndex);
+      this.appendContentProjectionCheckOp(node);
       this.appendDirectivesAndInputsOfNode(node);
       this.appendOutputsOfNode(node);
       this.appendChildren(node);
@@ -2034,6 +2129,22 @@ class Scope {
     }
   }
 
+  private appendContentProjectionCheckOp(root: TmplAstElement): void {
+    const meta =
+        this.tcb.boundTarget.getDirectivesOfNode(root)?.find(meta => meta.isComponent) || null;
+
+    if (meta !== null && meta.ngContentSelectors !== null && meta.ngContentSelectors.length > 0) {
+      const selectors = meta.ngContentSelectors;
+
+      // We don't need to generate anything for components that don't have projection
+      // slots, or they only have one catch-all slot (represented by `*`).
+      if (selectors.length > 1 || (selectors.length === 1 && selectors[0] !== '*')) {
+        this.opQueue.push(
+            new TcbControlFlowContentProjectionOp(this.tcb, root, selectors, meta.name));
+      }
+    }
+  }
+
   private appendDeferredBlock(block: TmplAstDeferredBlock): void {
     this.appendDeferredTriggers(block, block.triggers);
     this.appendDeferredTriggers(block, block.prefetchTriggers);
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
@@ -807,4 +807,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
   missingRequiredInputs(): void {}
   illegalForLoopTrackAccess(): void {}
   inaccessibleDeferredTriggerElement(): void {}
+  controlFlowPreventingContentProjection(): void {}
 }
diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts

Original file line number	Diff line number	Diff line change
`@@ -807,4 +807,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {`
`807`	`807`	`missingRequiredInputs(): void {}`
`808`	`808`	`illegalForLoopTrackAccess(): void {}`
`809`	`809`	`inaccessibleDeferredTriggerElement(): void {}`
	`810`	`+ controlFlowPreventingContentProjection(): void {}`
`810`	`811`	`}`