Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(compiler-cli): add compiler option to disable control flow content projection diagnostic #53311

Closed
wants to merge 1 commit into from
Closed

fix(compiler-cli): add compiler option to disable control flow content projection diagnostic #53311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions goldens/public-api/compiler-cli/extended_template_diagnostic_name.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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,
Copy link
Member Author

@crisbeto crisbeto Dec 1, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: it feels a bit weird to be special-casing this diagnostic as an extended diagnostic that's built into the compiler, but at the moment it seem possible to implement this as an extended diagnostic.

I have a branch where the check is implemented as an actual diagnostic and it passes our own tests, however when I tried installing it in two separate apps (Material dev app and an ng new app), it didn't work. It seems like calling TemplateTypeChecker.getSymbolOfNode from an extended diagnostic doesn't work.

Since the option do disable it is in the extendedDiagnostics field, we can easily swap out the implementation later without changing the functionality.

...ALL_DIAGNOSTIC_FACTORIES.map(factory => factory.name)
]);
23 changes: 15 additions & 8 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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 @@ -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);
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<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);
});
});
});
});