From 86b561807e779dcdd70ed9eacd0d2186b99879d5 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 1 Dec 2023 13:53:22 +0100 Subject: [PATCH] fix(compiler-cli): add compiler option to disable control flow content projection diagnostic These changes add an option to the `extendedDiagnostics` field that allows the check from #53190 to be disabled. This is a follow-up based on a recent discussion. --- .../extended_template_diagnostic_name.md | 2 + .../src/ngtsc/core/src/compiler.ts | 17 +++-- .../src/extended_template_diagnostic_name.ts | 3 +- .../src/ngtsc/typecheck/api/api.ts | 5 ++ .../src/ngtsc/typecheck/extended/index.ts | 6 ++ .../src/ngtsc/typecheck/src/oob.ts | 23 ++++-- .../ngtsc/typecheck/src/type_check_block.ts | 14 +++- .../typecheck/test/type_check_block_spec.ts | 1 + .../src/ngtsc/typecheck/testing/index.ts | 2 + .../test/ngtsc/template_typecheck_spec.ts | 73 +++++++++++++++++++ 10 files changed, 130 insertions(+), 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 index 65db985eb5a2..125b562667c3 100644 --- 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 index f3e755fa7d53..3e07bb8929f2 100644 --- 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,10 +1294,8 @@ ${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, @@ -1296,7 +1303,7 @@ ${allowedCategoryLabels.join('\n')} 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 index b4446eeb2546..677d6b56bad1 100644 --- 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 index 11e82931d560..89eb27ce7ad5 100644 --- 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 index 87d0c43fa992..93b24e02895e 100644 --- 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([ + 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 index 8adee446fb9b..774367b1721c 100644 --- 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 index 531f5a6c3b73..e095ab9f5c7e 100644 --- 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); }); } @@ -1932,7 +1940,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 index 04acceb7582c..712930a80dc7 100644 --- 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 index f72cde49c7ed..d79b4fa191c2 100644 --- 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 = { 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 index aa4383013882..570c0a013d5b 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -5318,6 +5318,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: ' ', + standalone: true, + }) + class Comp {} + + @Component({ + standalone: true, + imports: [Comp], + template: \` + + @if (true) { +
+ breaks projection + } + + \`, + }) + 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: ' ', + standalone: true, + }) + class Comp {} + + @Component({ + standalone: true, + imports: [Comp], + template: \` + + @if (true) { +
+ breaks projection + } + + \`, + }) + class TestCmp {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); }); }); });