Skip to content

Commit a7872ca

Browse files
crisbetoatscott
crisbeto
authored and
atscott
committed
refactor(compiler-cli): introduce template semantics checker (#54714)
Introduces a new `TemplateSemanticsChecker` that will be used to flag semantic errors in the user's template. Currently we do some of this in the type check block, but the problem is that it doesn't have access to the template type checker which prevents us from properly checking cases like #54670. This pass is also distinct from the extended template checks, because we don't want users to be able to turn the checks off and we want them to run even if `strictTemplates` are disabled. PR Close #54714
1 parent 1031478 commit a7872ca

File tree

12 files changed

+140
-58
lines changed

12 files changed

+140
-58
lines changed

‎packages/compiler-cli/src/ngtsc/annotations/component/BUILD.bazel‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,10 @@ ts_library(
2424
"//packages/compiler-cli/src/ngtsc/perf",
2525
"//packages/compiler-cli/src/ngtsc/reflection",
2626
"//packages/compiler-cli/src/ngtsc/scope",
27-
#"//packages/compiler-cli/src/ngtsc/shims:api",
2827
"//packages/compiler-cli/src/ngtsc/transform",
2928
"//packages/compiler-cli/src/ngtsc/typecheck/api",
30-
#"//packages/compiler-cli/src/ngtsc/typecheck/diagnostics",
3129
"//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
30+
"//packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api",
3231
"//packages/compiler-cli/src/ngtsc/util",
3332
"//packages/compiler-cli/src/ngtsc/xi18n",
3433
"@npm//@types/node",

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {getDiagnosticNode, makeUnknownComponentDeferredImportDiagnostic} from '.
2525
import {AnalysisOutput, CompilationMode, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../../transform';
2626
import {TypeCheckableDirectiveMeta, TypeCheckContext} from '../../../typecheck/api';
2727
import {ExtendedTemplateChecker} from '../../../typecheck/extended/api';
28+
import {TemplateSemanticsChecker} from '../../../typecheck/template_semantics/api/api';
2829
import {getSourceFile} from '../../../util/src/typescript';
2930
import {Xi18nContext} from '../../../xi18n';
3031
import {combineResolvers, compileDeclareFactory, compileInputTransformFields, compileNgFactoryDefField, compileResults, extractClassDebugInfo, extractClassMetadata, extractSchemas, findAngularDecorator, forwardRefResolver, getDirectiveDiagnostics, getProviderDiagnostics, InjectableClassRegistry, isExpressionForwardReference, readBaseClass, ReferencesRegistry, removeIdentifierReferences, resolveEncapsulationEnumValueLocally, resolveEnumValue, resolveImportedFile, resolveLiteral, resolveProvidersRequiringFactory, ResourceLoader, toFactoryMetadata, tryUnwrapForwardRef, validateHostDirectives, wrapFunctionExpressionsInParens} from '../../common';
@@ -673,6 +674,12 @@ export class ComponentDecoratorHandler implements
673674
return extendedTemplateChecker.getDiagnosticsForComponent(component);
674675
}
675676

677+
templateSemanticsCheck(
678+
component: ts.ClassDeclaration,
679+
templateSemanticsChecker: TemplateSemanticsChecker): ts.Diagnostic[] {
680+
return templateSemanticsChecker.getDiagnosticsForComponent(component);
681+
}
682+
676683
resolve(
677684
node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>,
678685
symbol: ComponentSymbol): ResolveResult<ComponentResolutionData> {

‎packages/compiler-cli/src/ngtsc/core/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ ts_library(
3939
"//packages/compiler-cli/src/ngtsc/typecheck/diagnostics",
4040
"//packages/compiler-cli/src/ngtsc/typecheck/extended",
4141
"//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
42+
"//packages/compiler-cli/src/ngtsc/typecheck/template_semantics",
43+
"//packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api",
4244
"//packages/compiler-cli/src/ngtsc/util",
4345
"//packages/compiler-cli/src/ngtsc/xi18n",
4446
"@npm//@types/semver",

‎packages/compiler-cli/src/ngtsc/core/src/compiler.ts‎

Lines changed: 33 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ import {TemplateTypeCheckerImpl} from '../../typecheck';
3333
import {OptimizeFor, TemplateTypeChecker, TypeCheckingConfig} from '../../typecheck/api';
3434
import {ALL_DIAGNOSTIC_FACTORIES, ExtendedTemplateCheckerImpl, SUPPORTED_DIAGNOSTIC_NAMES} from '../../typecheck/extended';
3535
import {ExtendedTemplateChecker} from '../../typecheck/extended/api';
36+
import {TemplateSemanticsChecker} from '../../typecheck/template_semantics/api/api';
37+
import {TemplateSemanticsCheckerImpl} from '../../typecheck/template_semantics/src/template_semantics_checker';
3638
import {getSourceFileOrNull, isDtsPath, toUnredirectedSourceFile} from '../../util/src/typescript';
3739
import {Xi18nContext} from '../../xi18n';
3840
import {DiagnosticCategoryLabel, NgCompilerAdapter, NgCompilerOptions} from '../api';
@@ -59,6 +61,7 @@ interface LazyCompilationState {
5961
templateTypeChecker: TemplateTypeChecker;
6062
resourceRegistry: ResourceRegistry;
6163
extendedTemplateChecker: ExtendedTemplateChecker|null;
64+
templateSemanticsChecker: TemplateSemanticsChecker|null;
6265

6366
/**
6467
* Only available in local compilation mode when option `generateExtraImportsInLocalMode` is set.
@@ -442,11 +445,7 @@ export class NgCompiler {
442445
// by running the extended template checking code, which will attempt to
443446
// generate the same TCB.
444447
try {
445-
diagnostics.push(...this.getTemplateDiagnostics());
446-
447-
if (this.options.strictTemplates) {
448-
diagnostics.push(...this.getExtendedTemplateDiagnostics());
449-
}
448+
diagnostics.push(...this.getTemplateDiagnostics(), ...this.runAdditionalChecks());
450449
} catch (err: unknown) {
451450
if (!isFatalDiagnosticError(err)) {
452451
throw err;
@@ -472,11 +471,9 @@ export class NgCompiler {
472471
// by running the extended template checking code, which will attempt to
473472
// generate the same TCB.
474473
try {
475-
diagnostics.push(...this.getTemplateDiagnosticsForFile(file, optimizeFor));
476-
477-
if (this.options.strictTemplates) {
478-
diagnostics.push(...this.getExtendedTemplateDiagnostics(file));
479-
}
474+
diagnostics.push(
475+
...this.getTemplateDiagnosticsForFile(file, optimizeFor),
476+
...this.runAdditionalChecks(file));
480477
} catch (err: unknown) {
481478
if (!isFatalDiagnosticError(err)) {
482479
throw err;
@@ -503,8 +500,12 @@ export class NgCompiler {
503500
try {
504501
diagnostics.push(...ttc.getDiagnosticsForComponent(component));
505502

506-
const extendedTemplateChecker = compilation.extendedTemplateChecker;
507-
if (this.options.strictTemplates && extendedTemplateChecker) {
503+
const {extendedTemplateChecker, templateSemanticsChecker} = compilation;
504+
505+
if (templateSemanticsChecker !== null) {
506+
diagnostics.push(...templateSemanticsChecker.getDiagnosticsForComponent(component));
507+
}
508+
if (this.options.strictTemplates && extendedTemplateChecker !== null) {
508509
diagnostics.push(...extendedTemplateChecker.getDiagnosticsForComponent(component));
509510
}
510511
} catch (err: unknown) {
@@ -968,26 +969,23 @@ export class NgCompiler {
968969
return this.nonTemplateDiagnostics;
969970
}
970971

971-
/**
972-
* Calls the `extendedTemplateCheck` phase of the trait compiler
973-
* @param sf optional parameter to get diagnostics for a certain file
974-
* or all files in the program if `sf` is undefined
975-
* @returns generated extended template diagnostics
976-
*/
977-
private getExtendedTemplateDiagnostics(sf?: ts.SourceFile): ts.Diagnostic[] {
972+
private runAdditionalChecks(sf?: ts.SourceFile): ts.Diagnostic[] {
978973
const diagnostics: ts.Diagnostic[] = [];
979974
const compilation = this.ensureAnalyzed();
980-
const extendedTemplateChecker = compilation.extendedTemplateChecker;
981-
if (!extendedTemplateChecker) {
982-
return [];
983-
}
984-
985-
if (sf !== undefined) {
986-
return compilation.traitCompiler.extendedTemplateCheck(sf, extendedTemplateChecker);
987-
}
988-
for (const sf of this.inputProgram.getSourceFiles()) {
989-
diagnostics.push(
990-
...compilation.traitCompiler.extendedTemplateCheck(sf, extendedTemplateChecker));
975+
const {extendedTemplateChecker, templateSemanticsChecker} = compilation;
976+
const files = sf ? [sf] : this.inputProgram.getSourceFiles();
977+
978+
for (const sf of files) {
979+
if (templateSemanticsChecker !== null) {
980+
diagnostics.push(...compilation.traitCompiler.runAdditionalChecks(sf, (clazz, handler) => {
981+
return handler.templateSemanticsCheck?.(clazz, templateSemanticsChecker) || null;
982+
}));
983+
}
984+
if (this.options.strictTemplates && extendedTemplateChecker !== null) {
985+
diagnostics.push(...compilation.traitCompiler.runAdditionalChecks(sf, (clazz, handler) => {
986+
return handler.extendedTemplateCheck?.(clazz, extendedTemplateChecker) || null;
987+
}));
988+
}
991989
}
992990

993991
return diagnostics;
@@ -1233,6 +1231,10 @@ export class NgCompiler {
12331231
templateTypeChecker, checker, ALL_DIAGNOSTIC_FACTORIES, this.options) :
12341232
null;
12351233

1234+
const templateSemanticsChecker = this.constructionDiagnostics.length === 0 ?
1235+
new TemplateSemanticsCheckerImpl(templateTypeChecker) :
1236+
null;
1237+
12361238
return {
12371239
isCore,
12381240
traitCompiler,
@@ -1248,6 +1250,7 @@ export class NgCompiler {
12481250
resourceRegistry,
12491251
extendedTemplateChecker,
12501252
localCompilationExtraImportsTracker,
1253+
templateSemanticsChecker,
12511254
};
12521255
}
12531256
}

‎packages/compiler-cli/src/ngtsc/transform/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ ts_library(
2121
"//packages/compiler-cli/src/ngtsc/translator",
2222
"//packages/compiler-cli/src/ngtsc/typecheck/api",
2323
"//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
24+
"//packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api",
2425
"//packages/compiler-cli/src/ngtsc/util",
2526
"//packages/compiler-cli/src/ngtsc/xi18n",
2627
"@npm//typescript",

‎packages/compiler-cli/src/ngtsc/transform/src/api.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {ClassDeclaration, Decorator, ReflectionHost} from '../../reflection';
1616
import {ImportManager} from '../../translator';
1717
import {TypeCheckContext} from '../../typecheck/api';
1818
import {ExtendedTemplateChecker} from '../../typecheck/extended/api';
19+
import {TemplateSemanticsChecker} from '../../typecheck/template_semantics/api/api';
1920
import {Xi18nContext} from '../../xi18n';
2021

2122
/**
@@ -175,6 +176,10 @@ export interface DecoratorHandler<D, A, S extends SemanticSymbol|null, R> {
175176
(component: ts.ClassDeclaration, extendedTemplateChecker: ExtendedTemplateChecker):
176177
ts.Diagnostic[];
177178

179+
templateSemanticsCheck?
180+
(component: ts.ClassDeclaration, templateSemanticsChecker: TemplateSemanticsChecker):
181+
ts.Diagnostic[];
182+
178183
/**
179184
* Generate a description of the field which should be added to the class, including any
180185
* initialization code to be generated.

‎packages/compiler-cli/src/ngtsc/transform/src/compilation.ts‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import {IndexingContext} from '../../indexer';
1717
import {PerfEvent, PerfRecorder} from '../../perf';
1818
import {ClassDeclaration, DeclarationNode, Decorator, isNamedClassDeclaration, ReflectionHost} from '../../reflection';
1919
import {ProgramTypeCheckAdapter, TypeCheckContext} from '../../typecheck/api';
20-
import {ExtendedTemplateChecker} from '../../typecheck/extended/api';
2120
import {getSourceFile} from '../../util/src/typescript';
2221
import {Xi18nContext} from '../../xi18n';
2322

@@ -522,8 +521,12 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
522521
}
523522
}
524523

525-
extendedTemplateCheck(sf: ts.SourceFile, extendedTemplateChecker: ExtendedTemplateChecker):
526-
ts.Diagnostic[] {
524+
runAdditionalChecks(
525+
sf: ts.SourceFile,
526+
check:
527+
(clazz: ts.ClassDeclaration,
528+
handler: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>) =>
529+
ts.Diagnostic[] | null): ts.Diagnostic[] {
527530
if (this.compilationMode === CompilationMode.LOCAL) {
528531
return [];
529532
}
@@ -539,10 +542,10 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
539542
}
540543
const record = this.classes.get(clazz)!;
541544
for (const trait of record.traits) {
542-
if (trait.handler.extendedTemplateCheck === undefined) {
543-
continue;
545+
const result = check(clazz, trait.handler);
546+
if (result !== null) {
547+
diagnostics.push(...result);
544548
}
545-
diagnostics.push(...trait.handler.extendedTemplateCheck(clazz, extendedTemplateChecker));
546549
}
547550
}
548551
return diagnostics;

‎packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import {NOOP_PERF_RECORDER} from '../../perf';
1515
import {ClassDeclaration, Decorator, isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection';
1616
import {getDeclaration, makeProgram} from '../../testing';
1717
import {CompilationMode, DetectResult, DtsTransformRegistry, TraitCompiler} from '../../transform';
18-
import {ExtendedTemplateChecker} from '../../typecheck/extended/api';
1918
import {AnalysisOutput, CompileResult, DecoratorHandler, HandlerPrecedence, ResolveResult} from '../src/api';
2019

2120
const fakeSfTypeIdentifier = {
@@ -309,10 +308,6 @@ runInEachFileSystem(() => {
309308

310309
register(): void {}
311310

312-
extendedTemplateCheck() {
313-
return [];
314-
}
315-
316311
updateResources() {}
317312

318313
symbol(): null {
@@ -368,21 +363,6 @@ runInEachFileSystem(() => {
368363
expect(handler.register).toHaveBeenCalled();
369364
});
370365

371-
it('should not call extendedTemplateCheck', () => {
372-
const contents = `
373-
export class Test {}
374-
`;
375-
const handler = new TestDecoratorHandler();
376-
spyOn(handler, 'extendedTemplateCheck');
377-
const {compiler, sourceFile} = setup(contents, [handler], CompilationMode.LOCAL);
378-
379-
compiler.analyzeSync(sourceFile);
380-
compiler.resolve();
381-
compiler.extendedTemplateCheck(sourceFile, {} as ExtendedTemplateChecker);
382-
383-
expect(handler.extendedTemplateCheck).not.toHaveBeenCalled();
384-
});
385-
386366
it('should not call updateResources', () => {
387367
const contents = `
388368
export class Test {}

‎packages/compiler-cli/src/ngtsc/typecheck/template_semantics/BUILD.bazel‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
load("//tools:defaults.bzl", "ts_library")
2+
3+
ts_library(
4+
name = "template_semantics",
5+
srcs = glob(
6+
["**/*.ts"],
7+
),
8+
visibility = ["//packages/compiler-cli/src/ngtsc:__subpackages__"],
9+
deps = [
10+
"//packages/compiler",
11+
"//packages/compiler-cli/src/ngtsc/core:api",
12+
"//packages/compiler-cli/src/ngtsc/diagnostics",
13+
"//packages/compiler-cli/src/ngtsc/typecheck/api",
14+
"//packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api",
15+
"@npm//typescript",
16+
],
17+
)

‎packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api/BUILD.bazel‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
load("//tools:defaults.bzl", "ts_library")
2+
3+
ts_library(
4+
name = "api",
5+
srcs = glob(
6+
["**/*.ts"],
7+
),
8+
visibility = ["//packages/compiler-cli/src/ngtsc:__subpackages__"],
9+
deps = [
10+
"//packages/compiler",
11+
"//packages/compiler-cli/src/ngtsc/core:api",
12+
"//packages/compiler-cli/src/ngtsc/diagnostics",
13+
"//packages/compiler-cli/src/ngtsc/typecheck/api",
14+
"@npm//typescript",
15+
],
16+
)

0 commit comments

Comments
 (0)