Skip to content

Commit 1f6e843

Browse files
leonsenftatscott
leonsenft
authored and
atscott
committed
refactor(compiler-cli): validate @content block placement
Adds validation to verify that `@content` blocks are only used as direct children of foreign components. Specifically: - Defines a new compile diagnostic code `INVALID_CONTENT_PLACEMENT = 8026`. - Updates `ForeignComponentFeatureAnalyzer` to traverse content blocks and report `INVALID_CONTENT_PLACEMENT` diagnostics if they are placed incorrectly. - Removes the raw error thrown during ingestion in `packages/compiler/src/template/pipeline/src/ingest.ts`. - Adds integration tests in `template_typecheck_spec.ts`.
1 parent daa47b4 commit 1f6e843

5 files changed

Lines changed: 219 additions & 2 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ export enum ErrorCode {
8181
INLINE_TYPE_CTOR_REQUIRED = 8901,
8282
INTERPOLATED_SIGNAL_NOT_INVOKED = 8109,
8383
INVALID_BANANA_IN_BOX = 8101,
84+
INVALID_CONTENT_PLACEMENT = 8026,
8485
LET_USED_BEFORE_DEFINITION = 8016,
8586
LOCAL_COMPILATION_UNRESOLVED_CONST = 11001,
8687
LOCAL_COMPILATION_UNSUPPORTED_EXPRESSION = 11003,

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

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,22 @@
99
import {
1010
BindingType,
1111
SelectorlessMatcher,
12+
TmplAstContent,
13+
TmplAstContentBlock,
14+
TmplAstDeferredBlock,
15+
TmplAstDeferredBlockError,
16+
TmplAstDeferredBlockLoading,
17+
TmplAstDeferredBlockPlaceholder,
1218
TmplAstElement,
19+
TmplAstForLoopBlock,
20+
TmplAstForLoopBlockEmpty,
21+
TmplAstIfBlock,
22+
TmplAstIfBlockBranch,
23+
TmplAstNode,
1324
TmplAstRecursiveVisitor,
25+
TmplAstSwitchBlock,
26+
TmplAstSwitchBlockCaseGroup,
27+
TmplAstTemplate,
1428
tmplAstVisitAll,
1529
TypeCheckId,
1630
} from '@angular/compiler';
@@ -39,6 +53,7 @@ export function analyzeForeignComponentFeatures(
3953
}
4054

4155
class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
56+
private currentParent: TmplAstNode | null = null;
4257
readonly diagnostics: ts.Diagnostic[] = [];
4358

4459
constructor(
@@ -52,6 +67,14 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
5267
return this.foreignMatcher !== null && this.foreignMatcher.match(tagName).length > 0;
5368
}
5469

70+
private parentNodeIsForeignComponent(): boolean {
71+
return (
72+
this.currentParent !== null &&
73+
this.currentParent instanceof TmplAstElement &&
74+
this.elementIsForeignComponent(this.currentParent.name)
75+
);
76+
}
77+
5578
override visitElement(element: TmplAstElement): void {
5679
if (this.elementIsForeignComponent(element.name)) {
5780
if (element.outputs.length > 0) {
@@ -92,6 +115,112 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
92115
}
93116
}
94117

118+
const prevParent = this.currentParent;
119+
this.currentParent = element;
95120
super.visitElement(element);
121+
this.currentParent = prevParent;
122+
}
123+
124+
override visitTemplate(template: TmplAstTemplate): void {
125+
const prevParent = this.currentParent;
126+
this.currentParent = template;
127+
super.visitTemplate(template);
128+
this.currentParent = prevParent;
129+
}
130+
131+
override visitDeferredBlock(deferred: TmplAstDeferredBlock): void {
132+
const prevParent = this.currentParent;
133+
this.currentParent = deferred;
134+
super.visitDeferredBlock(deferred);
135+
this.currentParent = prevParent;
136+
}
137+
138+
override visitDeferredBlockPlaceholder(block: TmplAstDeferredBlockPlaceholder): void {
139+
const prevParent = this.currentParent;
140+
this.currentParent = block;
141+
super.visitDeferredBlockPlaceholder(block);
142+
this.currentParent = prevParent;
143+
}
144+
145+
override visitDeferredBlockError(block: TmplAstDeferredBlockError): void {
146+
const prevParent = this.currentParent;
147+
this.currentParent = block;
148+
super.visitDeferredBlockError(block);
149+
this.currentParent = prevParent;
150+
}
151+
152+
override visitDeferredBlockLoading(block: TmplAstDeferredBlockLoading): void {
153+
const prevParent = this.currentParent;
154+
this.currentParent = block;
155+
super.visitDeferredBlockLoading(block);
156+
this.currentParent = prevParent;
157+
}
158+
159+
override visitSwitchBlock(block: TmplAstSwitchBlock): void {
160+
const prevParent = this.currentParent;
161+
this.currentParent = block;
162+
super.visitSwitchBlock(block);
163+
this.currentParent = prevParent;
164+
}
165+
166+
override visitSwitchBlockCaseGroup(block: TmplAstSwitchBlockCaseGroup): void {
167+
const prevParent = this.currentParent;
168+
this.currentParent = block;
169+
super.visitSwitchBlockCaseGroup(block);
170+
this.currentParent = prevParent;
171+
}
172+
173+
override visitForLoopBlock(block: TmplAstForLoopBlock): void {
174+
const prevParent = this.currentParent;
175+
this.currentParent = block;
176+
super.visitForLoopBlock(block);
177+
this.currentParent = prevParent;
178+
}
179+
180+
override visitForLoopBlockEmpty(block: TmplAstForLoopBlockEmpty): void {
181+
const prevParent = this.currentParent;
182+
this.currentParent = block;
183+
super.visitForLoopBlockEmpty(block);
184+
this.currentParent = prevParent;
185+
}
186+
187+
override visitIfBlock(block: TmplAstIfBlock): void {
188+
const prevParent = this.currentParent;
189+
this.currentParent = block;
190+
super.visitIfBlock(block);
191+
this.currentParent = prevParent;
192+
}
193+
194+
override visitIfBlockBranch(block: TmplAstIfBlockBranch): void {
195+
const prevParent = this.currentParent;
196+
this.currentParent = block;
197+
super.visitIfBlockBranch(block);
198+
this.currentParent = prevParent;
199+
}
200+
201+
override visitContent(content: TmplAstContent): void {
202+
const prevParent = this.currentParent;
203+
this.currentParent = content;
204+
super.visitContent(content);
205+
this.currentParent = prevParent;
206+
}
207+
208+
override visitContentBlock(block: TmplAstContentBlock): void {
209+
if (!this.parentNodeIsForeignComponent()) {
210+
this.diagnostics.push(
211+
makeTemplateDiagnostic(
212+
'' as TypeCheckId,
213+
this.sourceMapping,
214+
block.sourceSpan,
215+
ts.DiagnosticCategory.Error,
216+
ngErrorCode(ErrorCode.INVALID_CONTENT_PLACEMENT),
217+
'@content blocks are only valid as direct children of foreign components.',
218+
),
219+
);
220+
}
221+
const prevParent = this.currentParent;
222+
this.currentParent = block;
223+
super.visitContentBlock(block);
224+
this.currentParent = prevParent;
96225
}
97226
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,11 @@ export enum ErrorCode {
464464
*/
465465
FOREIGN_COMPONENT_UNSUPPORTED_BINDING = 8025,
466466

467+
/**
468+
* Raised when a `@content` block is not used as a direct child of a foreign component.
469+
*/
470+
INVALID_CONTENT_PLACEMENT = 8026,
471+
467472
/**
468473
* A two way binding in a template has an incorrect syntax,
469474
* parentheses outside brackets. For example:

‎packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2320,6 +2320,90 @@ runInEachFileSystem(() => {
23202320
);
23212321
env.driveMain();
23222322
});
2323+
2324+
it('should allow @content block when used as a direct child of a foreign component', () => {
2325+
env.write(
2326+
'test.ts',
2327+
`
2328+
${foreignSetupCode}
2329+
2330+
@Component({
2331+
selector: 'test',
2332+
template: '<FancyButton> @content(icon) {} </FancyButton>',
2333+
foreignImports: [frameworkImport(FancyButton)],
2334+
})
2335+
export class TestCmp {}
2336+
`,
2337+
);
2338+
const diags = env.driveDiagnostics();
2339+
expect(diags.length).toEqual(0);
2340+
});
2341+
2342+
it('should detect @content block used outside of a foreign component', () => {
2343+
env.write(
2344+
'test.ts',
2345+
`
2346+
${foreignSetupCode}
2347+
2348+
@Component({
2349+
selector: 'test',
2350+
template: '<div> @content(icon) {} </div>',
2351+
foreignImports: [frameworkImport(FancyButton)],
2352+
})
2353+
export class TestCmp {}
2354+
`,
2355+
);
2356+
const diags = env.driveDiagnostics();
2357+
expect(diags.length).toEqual(1);
2358+
expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.INVALID_CONTENT_PLACEMENT));
2359+
expect(diags[0].messageText).toEqual(
2360+
'@content blocks are only valid as direct children of foreign components.',
2361+
);
2362+
});
2363+
2364+
it('should detect @content block nested inside a foreign component but not as a direct child', () => {
2365+
env.write(
2366+
'test.ts',
2367+
`
2368+
${foreignSetupCode}
2369+
2370+
@Component({
2371+
selector: 'test',
2372+
template: '<FancyButton> <div> @content(icon) {} </div> </FancyButton>',
2373+
foreignImports: [frameworkImport(FancyButton)],
2374+
})
2375+
export class TestCmp {}
2376+
`,
2377+
);
2378+
const diags = env.driveDiagnostics();
2379+
expect(diags.length).toEqual(1);
2380+
expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.INVALID_CONTENT_PLACEMENT));
2381+
expect(diags[0].messageText).toEqual(
2382+
'@content blocks are only valid as direct children of foreign components.',
2383+
);
2384+
});
2385+
2386+
it('should detect @content block nested inside a block (like @if) within a foreign component', () => {
2387+
env.write(
2388+
'test.ts',
2389+
`
2390+
${foreignSetupCode}
2391+
2392+
@Component({
2393+
selector: 'test',
2394+
template: '<FancyButton> @if (true) { @content (icon) {} } </FancyButton>',
2395+
foreignImports: [frameworkImport(FancyButton)],
2396+
})
2397+
export class TestCmp {}
2398+
`,
2399+
);
2400+
const diags = env.driveDiagnostics();
2401+
expect(diags.length).toEqual(1);
2402+
expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.INVALID_CONTENT_PLACEMENT));
2403+
expect(diags[0].messageText).toEqual(
2404+
'@content blocks are only valid as direct children of foreign components.',
2405+
);
2406+
});
23232407
});
23242408

23252409
it('should detect a duplicate variable declaration', () => {

‎packages/compiler/src/template/pipeline/src/ingest.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,6 @@ function ingestNodes(unit: ViewCompilationUnit, template: t.Node[]): void {
268268
ingestForBlock(unit, node);
269269
} else if (node instanceof t.LetDeclaration) {
270270
ingestLetDeclaration(unit, node);
271-
} else if (node instanceof t.ContentBlock) {
272-
throw new Error(`@content blocks are only valid as direct children of foreign components.`);
273271
} else if (node instanceof t.Component) {
274272
// TODO(crisbeto): account for selectorless nodes.
275273
} else {

0 commit comments

Comments
 (0)