8000 fix(compiler-cli): track poisoned scopes with a flag (#39967) · angular/angular@178cc51 · GitHub
[go: up one dir, main page]
Skip to content

Commit 178cc51

Browse files
alxhubmhevery
alxhub
authored and
mhevery
committed
fix(compiler-cli): track poisoned scopes with a flag (#39967)
To avoid overwhelming a user with secondary diagnostics that derive from a "root cause" error, the compiler has the notion of a "poisoned" NgModule. An NgModule becomes poisoned when its declaration contains semantic errors: declarations which are not components or pipes, imports which are not other NgModules, etc. An NgModule also becomes poisoned if it imports or exports another poisoned NgModule. Previously, the compiler tracked this poisoned status as an alternate state for each scope. Either a correct scope could be produced, or the entire scope would be set to a sentinel error value. This meant that the compiler would not track any information about a scope that was determined to be in error. This method presents several issues: 1. The compiler is unable to support the language service and return results when a component or its module scope is poisoned. This is fine for compilation, since diagnostics will be produced showing the error(s), but the language service needs to still work for incorrect code. 2. `getComponentScopes()` does not return components with a poisoned scope, which interferes with resource tracking of incremental builds. If the component isn't included in that list, then the NgModule for it will not have its dependencies properly tracked, and this can cause future incremental build steps to produce incorrect results. This commit changes the tracking of poisoned module scopes to use a flag on the scope itself, rather than a sentinel value that replaces the scope. This means that the scope itself will still be tracked, even if it contains semantic errors. PR Close #39967
1 parent 0aa35ec commit 178cc51

File tree

21 files changed

+156
-92
lines changed

21 files changed

+156
-92
lines changed

‎packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ export class DecorationAnalyzer {
9797
this.scopeRegistry, this.scopeRegistry, new TemplateMapping(), this.isCore,
9898
this.resourceManager, this.rootDirs, !!this.compilerOptions.preserveWhitespaces,
9999
/* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat,
100+
/* usePoisonedData */ false,
100101
/* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
101102
this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, NOOP_DEPENDENCY_TRACKER,
102103
this.injectableRegistry, !!this.compilerOptions.annotateForClosureCompiler),

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

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ export interface ComponentAnalysisData {
7070
* require an Angular factory definition at runtime.
7171
*/
7272
viewProvidersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
73+
74+
isPoisoned: boolean;
7375
}
7476

7577
export type ComponentResolutionData = Pick<R3ComponentMetadata, ComponentMetadataResolvedFields>;
@@ -86,7 +88,7 @@ export class ComponentDecoratorHandler implements
8688
private templateMapping: TemplateMapping, private isCore: boolean,
8789
private resourceLoader: ResourceLoader, private rootDirs: ReadonlyArray<string>,
8890
private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean,
89-
private enableI18nLegacyMessageIdFormat: boolean,
91+
private enableI18nLegacyMessageIdFormat: boolean, private usePoisonedData: boolean,
9092
private i18nNormalizeLineEndingsInICUs: boolean|undefined,
9193
private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer,
9294
private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder,
@@ -359,6 +361,7 @@ export class ComponentDecoratorHandler implements
359361
template,
360362
providersRequiringFactory,
361< 9E88 /code>363
viewProvidersRequiringFactory,
364+
isPoisoned: diagnostics !== undefined && diagnostics.length > 0,
362365
},
363366
diagnostics,
364367
};
@@ -383,6 +386,7 @@ export class ComponentDecoratorHandler implements
383386
isComponent: true,
384387
baseClass: analysis.baseClass,
385388
...analysis.typeCheckMeta,
389+
isPoisoned: analysis.isPoisoned,
386390
});
387391

388392
if (!analysis.template.isInline) {
@@ -394,15 +398,19 @@ export class ComponentDecoratorHandler implements
394398

395399
index(
396400
context: IndexingContext, node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>) {
401+
if (analysis.isPoisoned && !this.usePoisonedData) {
402+
return null;
403+
}
397404
const scope = this.scopeReader.getScopeForComponent(node);
398405
const selector = analysis.meta.selector;
399406
const matcher = new SelectorMatcher<DirectiveMeta>();
400-
if (scope === 'error') {
401-
// Don't bother indexing components which had erroneous scopes.
402-
return null;
403-
}
404-
405407
if (scope !== null) {
408+
if ((scope.compilation.isPoisoned || scope.exported.isPoisoned) && !this.usePoisonedData) {
409+
// Don't bother indexing components which had erroneous scopes, unless specifically
410+
// requested.
411+
return null;
412+
}
413+
406414
for (const directive of scope.compilation.directives) {
407415
if (directive.selector !== null) {
408416
matcher.addSelectables(CssSelector.parse(directive.selector), directive);
@@ -429,9 +437,13 @@ export class ComponentDecoratorHandler implements
429437
return;
430438
}
431439

440+
if (meta.isPoisoned && !this.usePoisonedData) {
441+
return;
442+
}
443+
432444
const scope = this.typeCheckScopes.getTypeCheckScope(node);
433-
if (scope === 'error') {
434-
// Don't type-check components that had errors in their scopes.
445+
if (scope.isPoisoned && !this.usePoisonedData) {
446+
// Don't type-check components that had errors in their scopes, unless requested.
435447
return;
436448
}
437449

@@ -443,6 +455,10 @@ export class ComponentDecoratorHandler implements
443455

444456
resolve(node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>):
445457
ResolveResult<ComponentResolutionData> {
458+
if (analysis.isPoisoned && !this.usePoisonedData) {
459+
return {};
460+
}
461+
446462
const context = node.getSourceFile();
447463
// Check whether this component was registered with an NgModule. If so, it should be compiled
448464
// under that module's compilation scope.
@@ -455,7 +471,7 @@ export class ComponentDecoratorHandler implements
455471
wrapDirectivesAndPipesInClosure: false,
456472
};
457473

458-
if (scope !== null && scope !== 'error') {
474+
if (scope !== null && (!scope.compilation.isPoisoned || this.usePoisonedData)) {
459475
// Replace the empty components and directives from the analyze() step with a fully expanded
460476
// scope. This is possible now because during resolve() the whole compilation unit has been
461477
// fully analyzed.

‎packages/compiler-cli/src/ngtsc/annotations/src/directive.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ export interface DirectiveHandlerData {
4141
providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
4242
inputs: ClassPropertyMapping;
4343
outputs: ClassPropertyMapping;
44+
isPoisoned: boolean;
4445
}
4546

4647
export class DirectiveDecoratorHandler implements
@@ -106,7 +107,8 @@ export class DirectiveDecoratorHandler implements
106107
this.annotateForClosureCompiler),
107108
baseClass: readBaseClass(node, this.reflector, this.evaluator),
108109
typeCheckMeta: extractDirectiveTypeCheckMeta(node, directiveResult.inputs, this.reflector),
109-
providersRequiringFactory
110+
providersRequiringFactory,
111+
isPoisoned: false,
110112
}
111113
};
112114
}
@@ -126,6 +128,7 @@ export class DirectiveDecoratorHandler implements
126128
isComponent: false,
127129
baseClass: analysis.baseClass,
128130
...analysis.typeCheckMeta,
131+
isPoisoned: analysis.isPoisoned,
129132
});
130133

131134
this.injectableRegistry.registerInjectable(node);

‎packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ export class NgModuleDecoratorHandler implements
336336
injectorImports: [],
337337
};
338338

339-
if (scope !== null && scope !== 'error') {
339+
if (scope !== null && !scope.compilation.isPoisoned) {
340340
// Using the scope information, extend the injector's imports using the modules that are
341341
// specified as module exports.
342342
const context = getSourceFile(node);
@@ -361,7 +361,8 @@ export class NgModuleDecoratorHandler implements
361361
return {diagnostics};
362362
}
363363

364-
if (scope === null || scope === 'error' || scope.reexports === null) {
364+
if (scope === null || scope.compilation.isPoisoned || scope.exported.isPoisoned ||
365+
scope.reexports === null) {
365366
return {data};
366367
} else {
367368
return {

‎packages/compiler-cli/src/ngtsc/annotations/src/typecheck_scopes.ts‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ export interface TypeCheckScope {
3333
* The schemas that are used in this scope.
3434
*/
3535
schemas: SchemaMetadata[];
36+
37+
/**
38+
* Whether the original compilation scope which produced this `TypeCheckScope` was itself poisoned
39+
* (contained semantic errors during its production).
40+
*/
41+
isPoisoned: boolean;
3642
}
3743

3844
/**
@@ -57,15 +63,18 @@ export class TypeCheckScopes {
5763
* contains an error, then 'error' is returned. If the component is not declared in any NgModule,
5864
* an empty type-check scope is returned.
5965
*/
60-
getTypeCheckScope(node: ClassDeclaration): TypeCheckScope|'error' {
66+
getTypeCheckScope(node: ClassDeclaration): TypeCheckScope {
6167
const matcher = new SelectorMatcher<DirectiveMeta>();
6268
const pipes = new Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>();
6369

6470
const scope = this.scopeReader.getScopeForComponent(node);
6571
if (scope === null) {
66-
return {matcher, pipes, schemas: []};
67-
} else if (scope === 'error') {
68-
return scope;
72+
return {
73+
matcher,
74+
pipes,
75+
schemas: [],
76+
isPoisoned: false,
77+
};
6978
}
7079

7180
if (this.scopeCache.has(scope.ngModule)) {
@@ -87,7 +96,12 @@ export class TypeCheckScopes {
8796
pipes.set(name, ref as Reference<ClassDeclaration<ts.ClassDeclaration>>);
8897
}
8998

90-
const typeCheckScope: TypeCheckScope = {matcher, pipes, schemas: scope.schemas};
99+
const typeCheckScope: TypeCheckScope = {
100+
matcher,
101+
pipes,
102+
schemas: scope.schemas,
103+
isPoisoned: scope.compilation.isPoisoned || scope.exported.isPoisoned,
104+
};
91105
this.scopeCache.set(scope.ngModule, typeCheckScope);
92106
return typeCheckScope;
93107
}

‎packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ runInEachFileSystem(() => {
7373
/* isCore */ false, new NoopResourceLoader(), /* rootDirs */[''],
7474
/* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true,
7575
/* enableI18nLegacyMessageIdFormat */ false,
76+
/* usePoisonedData */ false,
7677
/* i18nNormalizeLineEndingsInICUs */ undefined, moduleResolver, cycleAnalyzer, refEmitter,
7778
NOOP_DEFAULT_IMPORT_RECORDER, /* depTracker */ null, injectableRegistry,
7879
/* annotateForClosureCompiler */ false);

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ export class NgCompiler {
106106
private typeCheckingProgramStrategy: TypeCheckingProgramStrategy,
107107
private incrementalStrategy: IncrementalBuildStrategy,
108108
private enableTemplateTypeChecker: boolean,
109+
private usePoisonedData: boolean,
109110
oldProgram: ts.Program|null = null,
110111
private perfRecorder: PerfRecorder = NOOP_PERF_RECORDER,
111112
) {
@@ -742,7 +743,7 @@ export class NgCompiler {
742743
reflector, evaluator, metaRegistry, metaReader, scopeReader, scopeRegistry,
743744
templateMapping, isCore, this.resourceManager, this.adapter.rootDirs,
744745
this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false,
745-
this.options.enableI18nLegacyMessageIdFormat !== false,
746+
this.options.enableI18nLegacyMessageIdFormat !== false, this.usePoisonedData,
746747
this.options.i18nNormalizeLineEndingsInICUs, this.moduleResolver, this.cycleAnalyzer,
747748
refEmitter, defaultImportTracker, this.incrementalDriver.depGraph, injectableRegistry,
748749
this.closureCompilerEnabled),

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ runInEachFileSystem(() => {
5151
const program = ts.createProgram({host, options, rootNames: host.inputFiles});
5252
const compiler = new NgCompiler(
5353
host, options, program, new ReusedProgramStrategy(program, host, options, []),
54-
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
54+
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
55+
/* usePoisonedData */ false);
5556

5657
const diags = compiler.getDiagnostics(getSourceFileOrError(program, COMPONENT));
5758
expect(diags.length).toBe(1);
@@ -100,7 +101,8 @@ runInEachFileSystem(() => {
100101
const CmpC = getClass(getSourceFileOrError(program, cmpCFile), 'CmpC');
101102
const compiler = new NgCompiler(
102103
host, options, program, new ReusedProgramStrategy(program, host, options, []),
103-
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
104+
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
105+
/* usePoisonedData */ false);
104106
const components = compiler.getComponentsWithTemplateFile(templateFile);
105107
expect(components).toEqual(new Set([CmpA, CmpC]));
106108
});
@@ -129,7 +131,8 @@ runInEachFileSystem(() => {
129131
const program = ts.createProgram({host, options, rootNames: host.inputFiles});
130132
const compiler = new NgCompiler(
131133
host, options, program, new ReusedProgramStrategy(program, host, options, []),
132-
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
134+
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
135+
/* usePoisonedData */ false);
133136

134137
const deps = compiler.getResourceDependencies(getSourceFileOrError(program, COMPONENT));
135138
expect(deps.length).toBe(2);

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,12 @@ export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta {
108108
* another type, it could not statically determine the base class.
109109
*/
110110
baseClass: Reference<ClassDeclaration>|'dynamic'|null;
111+
112+
/**
113+
* Whether the directive had some issue with its declaration that means it might not have complete
114+
* and reliable metadata.
115+
*/
116+
isPoisoned: boolean;
111117
}
112118

113119
/**

‎packages/compiler-cli/src/ngtsc/metadata/src/dts.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ export class DtsMetadataReader implements MetadataReader {
9292
queries: readStringArrayType(def.type.typeArguments[5]),
9393
...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector),
9494
baseClass: readBaseClass(clazz, this.checker, this.reflector),
95+
isPoisoned: false,
9596
};
9697
}
9798

0 commit comments

Comments
 (0)
0