Skip to content

Commit 79e5d5d

Browse files
leonsenftatscott
leonsenft
authored and
atscott
committed
refactor(compiler-cli): validate @content block names for conflicts
Ensures `@content` blocks on foreign components have unique names and do not conflict with static attributes or input property bindings. Specifically, this commit introduces two new template diagnostics: 1. `CONFLICTING_CONTENT_DECLARATION` (8028): Raised when multiple `@content` blocks with the same name are defined under the same foreign component. 2. `CONFLICTING_CONTENT_AND_PROPERTY` (8029): Raised when a `@content` block's name matches an attribute or input property binding on the parent foreign component. Both diagnostics include related information pointing to the location of the conflicting declaration or property.
1 parent 3374424 commit 79e5d5d

4 files changed

Lines changed: 352 additions & 49 deletions

File tree

‎goldens/public-api/compiler-cli/error_code.api.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ export enum ErrorCode {
2828
CONFIG_FLAT_MODULE_NO_INDEX = 4001,
2929
// (undocumented)
3030
CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002,
31+
CONFLICTING_CONTENT_AND_PROPERTY = 8029,
32+
CONFLICTING_CONTENT_DECLARATION = 8028,
3133
CONFLICTING_HOST_DIRECTIVE_BINDING = -8024,
3234
CONFLICTING_INPUT_TRANSFORM = 2020,
3335
CONFLICTING_LET_DECLARATION = 8017,

‎packages/compiler-cli/src/ngtsc/annotations/component/src/foreign_component.ts‎

Lines changed: 175 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,11 @@ import {ForeignComponentMeta} from '../../../metadata';
3636
import {makeTemplateDiagnostic} from '../../../typecheck/diagnostics';
3737
import {SourceMapping} from '../../../typecheck/api';
3838

39+
/**
40+
* The intrinsic property name used to project children into a foreign component.
41+
*/
42+
const CHILDREN = 'children';
43+
3944
/**
4045
* Analyzes the template for invalid use of features relating to foreign components.
4146
*
@@ -55,6 +60,10 @@ export function analyzeForeignComponentFeatures(
5560
class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
5661
private currentParent: TmplAstNode | null = null;
5762
readonly diagnostics: ts.Diagnostic[] = [];
63+
// Tracks the named @content blocks defined for each foreign component element.
64+
// This is used to detect duplicate @content declarations under the same parent
65+
// during the recursive AST traversal.
66+
private readonly seenContentBlocks = new Map<TmplAstElement, Map<string, TmplAstContentBlock>>();
5867

5968
constructor(
6069
private readonly foreignMatcher: SelectorlessMatcher<ForeignComponentMeta> | null,
@@ -77,42 +86,7 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
7786

7887
override visitElement(element: TmplAstElement): void {
7988
if (this.elementIsForeignComponent(element.name)) {
80-
if (element.outputs.length > 0) {
81-
this.diagnostics.push(
82-
makeTemplateDiagnostic(
83-
'' as TypeCheckId,
84-
this.sourceMapping,
85-
element.sourceSpan,
86-
ts.DiagnosticCategory.Error,
87-
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_UNSUPPORTED_BINDING),
88-
'Foreign components do not support event bindings.',
89-
),
90-
);
91-
}
92-
if (element.references.length > 0) {
93-
this.diagnostics.push(
94-
makeTemplateDiagnostic(
95-
'' as TypeCheckId,
96-
this.sourceMapping,
97-
element.sourceSpan,
98-
ts.DiagnosticCategory.Error,
99-
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_UNSUPPORTED_BINDING),
100-
'Foreign components do not support references.',
101-
),
102-
);
103-
}
104-
if (element.inputs.some((input) => input.type !== BindingType.Property)) {
105-
this.diagnostics.push(
106-
makeTemplateDiagnostic(
107-
'' as TypeCheckId,
108-
this.sourceMapping,
109-
element.sourceSpan,
110-
ts.DiagnosticCategory.Error,
111-
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_UNSUPPORTED_BINDING),
112-
'Foreign components only support static attributes and property bindings.',
113-
),
114-
);
115-
}
89+
this.validateForeignComponent(element);
11690
}
11791

11892
const prevParent = this.currentParent;
@@ -121,6 +95,84 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
12195
this.currentParent = prevParent;
12296
}
12397

98+
private validateForeignComponent(element: TmplAstElement): void {
99+
if (element.outputs.length > 0) {
100+
this.diagnostics.push(
101+
makeTemplateDiagnostic(
102+
'' as TypeCheckId,
103+
this.sourceMapping,
104+
element.sourceSpan,
105+
ts.DiagnosticCategory.Error,
106+
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_UNSUPPORTED_BINDING),
107+
'Foreign components do not support event bindings.',
108+
),
109+
);
110+
}
111+
if (element.references.length > 0) {
112+
this.diagnostics.push(
113+
makeTemplateDiagnostic(
114+
'' as TypeCheckId,
115+
this.sourceMapping,
116+
element.sourceSpan,
117+
ts.DiagnosticCategory.Error,
118+
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_UNSUPPORTED_BINDING),
119+
'Foreign components do not support references.',
120+
),
121+
);
122+
}
123+
if (element.inputs.some((input) => input.type !== BindingType.Property)) {
124+
this.diagnostics.push(
125+
makeTemplateDiagnostic(
126+
'' as TypeCheckId,
127+
this.sourceMapping,
128+
element.sourceSpan,
129+
ts.DiagnosticCategory.Error,
130+
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_UNSUPPORTED_BINDING),
131+
'Foreign components only support static attributes and property bindings.',
132+
),
133+
);
134+
}
135+
136+
// A foreign component maps implicit child nodes to a 'children' property.
137+
// If the user also explicitly binds to '[children]' or sets a static 'children' attribute,
138+
// this is a conflict.
139+
const childrenInput = element.inputs.find(
140+
(input) => input.type === BindingType.Property && input.name === CHILDREN,
141+
);
142+
const childrenAttr = element.attributes.find((attr) => attr.name === CHILDREN);
143+
const conflictingSource = childrenInput ?? childrenAttr;
144+
if (conflictingSource === undefined) {
145+
return;
146+
}
147+
148+
// Explicit `@content` blocks (TmplAstContentBlock) are mapped to properties by their name, so
149+
// they do not conflict with the default 'children' property. We only care about child nodes
150+
// that are not content blocks, as those are implicitly passed to the 'children' property.
151+
const firstChild = element.children.find((child) => !(child instanceof TmplAstContentBlock));
152+
if (firstChild === undefined) {
153+
return;
154+
}
155+
156+
this.diagnostics.push(
157+
makeTemplateDiagnostic(
158+
'' as TypeCheckId,
159+
this.sourceMapping,
160+
conflictingSource.sourceSpan,
161+
ts.DiagnosticCategory.Error,
162+
ngErrorCode(ErrorCode.CONFLICTING_CONTENT_AND_PROPERTY),
163+
`A foreign component cannot have both a '${CHILDREN}' property and child nodes.`,
164+
[
165+
{
166+
text: 'Child nodes are defined here.',
167+
start: firstChild.sourceSpan.start.offset,
168+
end: firstChild.sourceSpan.end.offset,
169+
sourceFile: this.sourceMapping.node.getSourceFile(),
170+
},
171+
],
172+
),
173+
);
174+
}
175+
124176
override visitTemplate(template: TmplAstTemplate): void {
125177
const prevParent = this.currentParent;
126178
this.currentParent = template;
@@ -207,19 +259,7 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
207259

208260
override visitContentBlock(block: TmplAstContentBlock): void {
209261
if (this.parentNodeIsForeignComponent()) {
210-
if (block.name === 'children') {
211-
this.diagnostics.push(
212-
makeTemplateDiagnostic(
213-
'' as TypeCheckId,
214-
this.sourceMapping,
215-
block.sourceSpan,
216-
ts.DiagnosticCategory.Error,
217-
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_CONTENT_UNNECESSARY_FOR_CHILDREN),
218-
'Defining a @content (children) block is unnecessary. ' +
219-
'Pass children as direct nested content of the foreign component instead.',
220-
),
221-
);
222-
}
262+
this.validateContentBlock(block);
223263
} else {
224264
this.diagnostics.push(
225265
makeTemplateDiagnostic(
@@ -232,9 +272,95 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
232272
),
233273
);
234274
}
275+
235276
const prevParent = this.currentParent;
236277
this.currentParent = block;
237278
super.visitContentBlock(block);
238279
this.currentParent = prevParent;
239280
}
281+
282+
private validateContentBlock(block: TmplAstContentBlock): void {
283+
const parent = this.currentParent as TmplAstElement;
284+
285+
// Retrieve or initialize the map of @content blocks seen so far for this parent.
286+
// Since the visitor is recursive, we must track declarations per-parent to
287+
// only report duplicates within the scope of the same foreign component.
288+
let seen = this.seenContentBlocks.get(parent);
289+
if (seen === undefined) {
290+
seen = new Map<string, TmplAstContentBlock>();
291+
this.seenContentBlocks.set(parent, seen);
292+
}
293+
294+
if (seen.has(block.name)) {
295+
const firstDecl = seen.get(block.name)!;
296+
this.diagnostics.push(
297+
makeTemplateDiagnostic(
298+
'' as TypeCheckId,
299+
this.sourceMapping,
300+
block.sourceSpan,
301+
ts.DiagnosticCategory.Error,
302+
ngErrorCode(ErrorCode.CONFLICTING_CONTENT_DECLARATION),
303+
`A @content block with the name '${block.name}' has already been defined for this ` +
304+
'component.',
305+
[
306+
{
307+
text: `The @content block '${block.name}' was first defined here.`,
308+
start: firstDecl.sourceSpan.start.offset,
309+
end: firstDecl.sourceSpan.end.offset,
310+
sourceFile: this.sourceMapping.node.getSourceFile(),
311+
},
312+
],
313+
),
314+
);
315+
} else {
316+
seen.set(block.name, block);
317+
}
318+
319+
// A @content block projects content into a property of the foreign component.
320+
// If the parent element also binds to this property (either via a property binding
321+
// or a static attribute), it creates a conflict as both try to write to the same prop.
322+
const conflictInput = parent.inputs.find(
323+
(input) => input.type === BindingType.Property && input.name === block.name,
324+
);
325+
const conflictAttr = parent.attributes.find((attr) => attr.name === block.name);
326+
const conflict = conflictInput ?? conflictAttr;
327+
328+
if (conflict !== undefined) {
329+
this.diagnostics.push(
330+
makeTemplateDiagnostic(
331+
'' as TypeCheckId,
332+
this.sourceMapping,
333+
block.sourceSpan,
334+
ts.DiagnosticCategory.Error,
335+
ngErrorCode(ErrorCode.CONFLICTING_CONTENT_AND_PROPERTY),
336+
`A @content block with the name '${block.name}' conflicts with a property on the ` +
337+
'parent component.',
338+
[
339+
{
340+
text: `The property '${block.name}' is defined here.`,
341+
start: conflict.sourceSpan.start.offset,
342+
end: conflict.sourceSpan.end.offset,
343+
sourceFile: this.sourceMapping.node.getSourceFile(),
344+
},
345+
],
346+
),
347+
);
348+
}
349+
350+
// Explicitly defining a `@content(children)` block is unnecessary because child nodes are
351+
// implicitly passed to the `children` property.
352+
if (block.name === CHILDREN) {
353+
this.diagnostics.push(
354+
makeTemplateDiagnostic(
355+
'' as TypeCheckId,
356+
this.sourceMapping,
357+
block.sourceSpan,
358+
ts.DiagnosticCategory.Error,
359+
ngErrorCode(ErrorCode.FOREIGN_COMPONENT_CONTENT_UNNECESSARY_FOR_CHILDREN),
360+
`Defining a @content (${CHILDREN}) block is unnecessary. ` +
361+
'Pass children as direct nested content of the foreign component instead.',
362+
),
363+
);
364+
}
365+
}
240366
}

‎packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,16 @@ export enum ErrorCode {
475475
*/
476476
FOREIGN_COMPONENT_CONTENT_UNNECESSARY_FOR_CHILDREN = 8027,
477477

478+
/**
479+
* Raised when multiple `@content` blocks with the same name are defined for a foreign component.
480+
*/
481+
CONFLICTING_CONTENT_DECLARATION = 8028,
482+
483+
/**
484+
* Raised when a `@content` block name conflicts with an input binding on the parent foreign component.
485+
*/
486+
CONFLICTING_CONTENT_AND_PROPERTY = 8029,
487+
478488
/**
479489
* A two way binding in a template has an incorrect syntax,
480490
* parentheses outside brackets. For example:

0 commit comments

Comments
 (0)