Skip to content

Commit

Permalink
fix(compiler-cli): add compiler option to disable control flow conten…
Browse files Browse the repository at this point in the history 
…t projection diagnostic (#53311)

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.

PR Close #53311
  • Loading branch information
@crisbeto @dylhunn
crisbeto authored and dylhunn committed Dec 6, 2023
1 parent 8195be1 commit e620b3a
Show file tree
Hide file tree
Showing 10 changed files with 130 additions and 16 deletions.
2 changes: 2 additions & 0 deletions goldens/public-api/compiler-cli/extended_template_diagnostic_name.md
View file
Expand Up @@ -6,6 +6,8 @@

// @public
export enum ExtendedTemplateDiagnosticName {
// (undocumented)
CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = "controlFlowPreventingContentProjection",
// (undocumented)
INTERPOLATED_SIGNAL_NOT_INVOKED = "interpolatedSignalNotInvoked",
// (undocumented)
Expand Down
17 changes: 12 additions & 5 deletions packages/compiler-cli/src/ngtsc/core/src/compiler.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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,
};
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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(),
});
}
Expand Down
3 changes: 2 additions & 1 deletion packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
View file
Expand Up @@ -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',
}
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
View file
Expand Up @@ -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.
*
Expand Down
6 changes: 6 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts
View file
Expand Up @@ -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)
]);
23 changes: 15 additions & 8 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
View file
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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 = [
Expand All @@ -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')));
}
}

Expand Down
14 changes: 12 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View file
Expand Up @@ -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;
Expand All @@ -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);
});
}
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
View file
Expand Up @@ -787,6 +787,7 @@ describe('type check blocks', () => {
enableTemplateTypeChecker: false,
useInlineTypeConstructors: true,
suggestionsForSuboptimalTypeInference: false,
controlFlowPreventingContentProjection: 'warning',
};

describe('config.applyTemplateContextGuards', () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
View file
Expand Up @@ -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.
Expand Down Expand Up @@ -323,6 +324,7 @@ export function tcb(
checkTypeOfPipes: true,
checkTemplateBodies: true,
alwaysCheckSchemaInTemplateBodies: true,
controlFlowPreventingContentProjection: 'warning',
strictSafeNavigationTypes: true,
useContextGenericType: true,
strictLiteralTypes: true,
Expand Down
73 changes: 73 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View file
Expand Up @@ -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);
});
});
});
});

0 comments on commit e620b3a

Please sign in to comment.