fix(compiler-cli): add compiler option to disable control flow conten…

…t projection diagnostic (angular#53311) These changes add an option to the `extendedDiagnostics` field that allows the check from angular#53190 to be disabled. This is a follow-up based on a recent discussion. PR Close angular#53311
tbondwilkinson · Dec 6, 2023 · 9f38477 · 9f38477
1 parent 56e349f
commit 9f38477
Show file tree

Hide file tree

Showing 10 changed files with 130 additions and 16 deletions.
diff --git a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md
@@ -6,6 +6,8 @@
 
 // @public
 export enum ExtendedTemplateDiagnosticName {
+    // (undocumented)
+    CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = "controlFlowPreventingContentProjection",
     // (undocumented)
     INTERPOLATED_SIGNAL_NOT_INVOKED = "interpolatedSignalNotInvoked",
     // (undocumented)

diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts
@@ -31,7 +31,7 @@ import {StandaloneComponentScopeReader} from '../../scope/src/standalone';
 import {aliasTransformFactory, CompilationMode, declarationTransformFactory, DecoratorHandler, DtsTransformRegistry, ivyTransformFactory, TraitCompiler} from '../../transform';
 import {TemplateTypeCheckerImpl} from '../../typecheck';
 import {OptimizeFor, TemplateTypeChecker, TypeCheckingConfig} from '../../typecheck/api';
-import {ALL_DIAGNOSTIC_FACTORIES, ExtendedTemplateCheckerImpl} from '../../typecheck/extended';
+import {ALL_DIAGNOSTIC_FACTORIES, ExtendedTemplateCheckerImpl, SUPPORTED_DIAGNOSTIC_NAMES} from '../../typecheck/extended';
 import {ExtendedTemplateChecker} from '../../typecheck/extended/api';
 import {getSourceFileOrNull, isDtsPath, toUnredirectedSourceFile} from '../../util/src/typescript';
 import {Xi18nContext} from '../../xi18n';
@@ -782,6 +782,8 @@ export class NgCompiler {
         // (providing the full TemplateTypeChecker API) and if strict mode is not enabled. In strict
         // mode, the user is in full control of type inference.
         suggestionsForSuboptimalTypeInference: this.enableTemplateTypeChecker && !strictTemplates,
+        controlFlowPreventingContentProjection:
+            this.options.extendedDiagnostics?.defaultCategory || DiagnosticCategoryLabel.Warning,
       };
     } else {
       typeCheckingConfig = {
@@ -810,6 +812,8 @@ export class NgCompiler {
         // In "basic" template type-checking mode, no warnings are produced since most things are
         // not checked anyways.
         suggestionsForSuboptimalTypeInference: false,
+        controlFlowPreventingContentProjection:
+            this.options.extendedDiagnostics?.defaultCategory || DiagnosticCategoryLabel.Warning,
       };
     }
 
@@ -848,6 +852,11 @@ export class NgCompiler {
     if (this.options.strictLiteralTypes !== undefined) {
       typeCheckingConfig.strictLiteralTypes = this.options.strictLiteralTypes;
     }
+    if (this.options.extendedDiagnostics?.checks?.controlFlowPreventingContentProjection !==
+        undefined) {
+      typeCheckingConfig.controlFlowPreventingContentProjection =
+          this.options.extendedDiagnostics.checks.controlFlowPreventingContentProjection;
+    }
 
     return typeCheckingConfig;
   }
@@ -1285,18 +1294,16 @@ ${allowedCategoryLabels.join('\n')}
     });
   }
 
-  const allExtendedDiagnosticNames =
-      ALL_DIAGNOSTIC_FACTORIES.map((factory) => factory.name) as string[];
   for (const [checkName, category] of Object.entries(options.extendedDiagnostics?.checks ?? {})) {
-    if (!allExtendedDiagnosticNames.includes(checkName)) {
+    if (!SUPPORTED_DIAGNOSTIC_NAMES.has(checkName)) {
       yield makeConfigDiagnostic({
         category: ts.DiagnosticCategory.Error,
         code: ErrorCode.CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CHECK,
         messageText: `
 Angular compiler option "extendedDiagnostics.checks" has an unknown check: "${checkName}".
 
 Allowed check names are:
-${allExtendedDiagnosticNames.join('\n')}
+${Array.from(SUPPORTED_DIAGNOSTIC_NAMES).join('\n')}
         `.trim(),
       });
     }

diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
@@ -24,5 +24,6 @@ export enum ExtendedTemplateDiagnosticName {
   MISSING_NGFOROF_LET = 'missingNgForOfLet',
   SUFFIX_NOT_SUPPORTED = 'suffixNotSupported',
   SKIP_HYDRATION_NOT_STATIC = 'skipHydrationNotStatic',
-  INTERPOLATED_SIGNAL_NOT_INVOKED = 'interpolatedSignalNotInvoked'
+  INTERPOLATED_SIGNAL_NOT_INVOKED = 'interpolatedSignalNotInvoked',
+  CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 'controlFlowPreventingContentProjection',
 }
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
@@ -278,6 +278,11 @@ export interface TypeCheckingConfig {
    */
   checkQueries: false;
 
+  /**
+   * Whether to check if control flow syntax will prevent a node from being projected.
+   */
+  controlFlowPreventingContentProjection: 'error'|'warning'|'suppress';
+
   /**
    * Whether to use any generic types of the context component.
    *

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts
@@ -27,3 +27,9 @@ export const ALL_DIAGNOSTIC_FACTORIES:
       textAttributeNotBindingFactory, missingNgForOfLetFactory, suffixNotSupportedFactory,
       interpolatedSignalNotInvoked
     ];
+
+
+export const SUPPORTED_DIAGNOSTIC_NAMES = new Set<string>([
+  ExtendedTemplateDiagnosticName.CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION,
+  ...ALL_DIAGNOSTIC_FACTORIES.map(factory => factory.name)
+]);
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
@@ -105,8 +105,9 @@ export interface OutOfBandDiagnosticRecorder {
    * Reports cases where control flow nodes prevent content projection.
    */
   controlFlowPreventingContentProjection(
-      templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string,
-      slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock,
+      templateId: TemplateId, category: ts.DiagnosticCategory,
+      projectionNode: TmplAstElement|TmplAstTemplate, componentName: string, slotSelector: string,
+      controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock,
       preservesWhitespaces: boolean): void;
 }
 
@@ -350,8 +351,9 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
   }
 
   controlFlowPreventingContentProjection(
-      templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string,
-      slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock,
+      templateId: TemplateId, category: ts.DiagnosticCategory,
+      projectionNode: TmplAstElement|TmplAstTemplate, componentName: string, slotSelector: string,
+      controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock,
       preservesWhitespaces: boolean): void {
     const blockName = controlFlowNode instanceof TmplAstIfBlockBranch ? '@if' : '@for';
     const lines = [
@@ -367,14 +369,19 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
 
     if (preservesWhitespaces) {
       lines.push(
-          `Note: the host component has \`preserveWhitespaces: true\` which may ` +
-          `cause whitespace to affect content projection.`);
+          'Note: the host component has `preserveWhitespaces: true` which may ' +
+          'cause whitespace to affect content projection.');
     }
 
+    lines.push(
+        '',
+        'This check can be disabled using the `extendedDiagnostics.checks.' +
+            'controlFlowPreventingContentProjection = "suppress" compiler option.`');
+
     this._diagnostics.push(makeTemplateDiagnostic(
         templateId, this.resolver.getSourceMapping(templateId), projectionNode.startSourceSpan,
-        ts.DiagnosticCategory.Warning,
-        ngErrorCode(ErrorCode.CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION), lines.join('\n')));
+        category, ngErrorCode(ErrorCode.CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION),
+        lines.join('\n')));
   }
 }
 

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
@@ -918,10 +918,18 @@ class TcbDomSchemaCheckerOp extends TcbOp {
  * flow node didn't exist.
  */
 class TcbControlFlowContentProjectionOp extends TcbOp {
+  private readonly category: ts.DiagnosticCategory;
+
   constructor(
       private tcb: Context, private element: TmplAstElement, private ngContentSelectors: string[],
       private componentName: string) {
     super();
+
+    // We only need to account for `error` and `warning` since
+    // this check won't be enabled for `suppress`.
+    this.category = tcb.env.config.controlFlowPreventingContentProjection === 'error' ?
+        ts.DiagnosticCategory.Error :
+        ts.DiagnosticCategory.Warning;
   }
 
   override readonly optional = false;
@@ -944,7 +952,7 @@ class TcbControlFlowContentProjectionOp extends TcbOp {
           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.id, this.category, child, this.componentName, originalSelector, root,
                   this.tcb.hostPreserveWhitespaces);
             });
           }
@@ -1940,7 +1948,9 @@ 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);
+      if (this.tcb.env.config.controlFlowPreventingContentProjection !== 'suppress') {
+        this.appendContentProjectionCheckOp(node);
+      }
       this.appendDirectivesAndInputsOfNode(node);
       this.appendOutputsOfNode(node);
       this.appendChildren(node);

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
@@ -787,6 +787,7 @@ describe('type check blocks', () => {
       enableTemplateTypeChecker: false,
       useInlineTypeConstructors: true,
       suggestionsForSuboptimalTypeInference: false,
+      controlFlowPreventingContentProjection: 'warning',
     };
 
     describe('config.applyTemplateContextGuards', () => {

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
@@ -220,6 +220,7 @@ export const ALL_ENABLED_CONFIG: Readonly<TypeCheckingConfig> = {
   enableTemplateTypeChecker: false,
   useInlineTypeConstructors: true,
   suggestionsForSuboptimalTypeInference: false,
+  controlFlowPreventingContentProjection: 'warning',
 };
 
 // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
@@ -323,6 +324,7 @@ export function tcb(
     checkTypeOfPipes: true,
     checkTemplateBodies: true,
     alwaysCheckSchemaInTemplateBodies: true,
+    controlFlowPreventingContentProjection: 'warning',
     strictSafeNavigationTypes: true,
     useContextGenericType: true,
     strictLiteralTypes: true,

diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -5352,6 +5352,79 @@ suppress
         const diags = env.driveDiagnostics();
         expect(diags.length).toBe(0);
       });
+
+      it('should allow the content projection diagnostic to be disabled individually', () => {
+        env.tsconfig({
+          extendedDiagnostics: {
+            checks: {
+              controlFlowPreventingContentProjection: DiagnosticCategoryLabel.Suppress,
+            }
+          }
+        });
+        env.write('test.ts', `
+          import {Component} from '@angular/core';
+
+          @Component({
+            selector: 'comp',
+            template: '<ng-content/> <ng-content select="bar, [foo]"/>',
+            standalone: true,
+          })
+          class Comp {}
+
+          @Component({
+            standalone: true,
+            imports: [Comp],
+            template: \`
+              <comp>
+                @if (true) {
+                  <div foo></div>
+                  breaks projection
+                }
+              </comp>
+            \`,
+          })
+          class TestCmp {}
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(0);
+      });
+
+      it('should allow the content projection diagnostic to be disabled via `defaultCategory`',
+         () => {
+           env.tsconfig({
+             extendedDiagnostics: {
+               defaultCategory: DiagnosticCategoryLabel.Suppress,
+             }
+           });
+           env.write('test.ts', `
+              import {Component} from '@angular/core';
+
+              @Component({
+                selector: 'comp',
+                template: '<ng-content/> <ng-content select="bar, [foo]"/>',
+                standalone: true,
+              })
+              class Comp {}
+
+              @Component({
+                standalone: true,
+                imports: [Comp],
+                template: \`
+                  <comp>
+                    @if (true) {
+                      <div foo></div>
+                      breaks projection
+                    }
+                  </comp>
+                \`,
+              })
+              class TestCmp {}
+            `);
+
+           const diags = env.driveDiagnostics();
+           expect(diags.length).toBe(0);
+         });
     });
   });
 });