honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(compiler-cli): track poisoned scopes with a flag (#39923) 

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. A test is added to the language service which verifies that
poisoned scopes can still be used in template type-checking.

PR Close #39923
This commit is contained in:
Alex Rickabaugh 2020-11-25 15:02:23 -08:00 committed by Misko Hevery
parent 6d42954327
commit 0823622202
22 changed files with 225 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts
View File

@ -97,6 +97,7 @@ export class DecorationAnalyzer {
this.scopeRegistry, this.scopeRegistry, new ResourceRegistry(), this.isCore,
this.resourceManager, this.rootDirs, !!this.compilerOptions.preserveWhitespaces,
/* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat,
/* usePoisonedData */ false,
/* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, NOOP_DEPENDENCY_TRACKER,
this.injectableRegistry, !!this.compilerOptions.annotateForClosureCompiler),

30
packages/compiler-cli/src/ngtsc/annotations/src/component.ts
View File

@ -72,6 +72,8 @@ export interface ComponentAnalysisData {
viewProvidersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
resources: ComponentResources;
isPoisoned: boolean;
}
export type ComponentResolutionData = Pick<R3ComponentMetadata, ComponentMetadataResolvedFields>;
@ -88,7 +90,7 @@ export class ComponentDecoratorHandler implements
private resourceRegistry: ResourceRegistry, private isCore: boolean,
private resourceLoader: ResourceLoader, private rootDirs: ReadonlyArray<string>,
private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean,
private enableI18nLegacyMessageIdFormat: boolean,
private enableI18nLegacyMessageIdFormat: boolean, private usePoisonedData: boolean,
private i18nNormalizeLineEndingsInICUs: boolean|undefined,
private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer,
private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder,
@ -369,6 +371,7 @@ export class ComponentDecoratorHandler implements
styles: styleResources,
template: templateResource,
},
isPoisoned: diagnostics !== undefined && diagnostics.length > 0,
},
diagnostics,
};
@ -393,6 +396,7 @@ export class ComponentDecoratorHandler implements
isComponent: true,
baseClass: analysis.baseClass,
...analysis.typeCheckMeta,
isPoisoned: analysis.isPoisoned,
});
this.resourceRegistry.registerResources(analysis.resources, node);
@ -401,15 +405,19 @@ export class ComponentDecoratorHandler implements
index(
context: IndexingContext, node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>) {
if (analysis.isPoisoned && !this.usePoisonedData) {
return null;
}
const scope = this.scopeReader.getScopeForComponent(node);
const selector = analysis.meta.selector;
const matcher = new SelectorMatcher<DirectiveMeta>();
if (scope === 'error') {
// Don't bother indexing components which had erroneous scopes.
if (scope !== null) {
if ((scope.compilation.isPoisoned || scope.exported.isPoisoned) && !this.usePoisonedData) {
// Don't bother indexing components which had erroneous scopes, unless specifically
// requested.
return null;
}
if (scope !== null) {
for (const directive of scope.compilation.directives) {
if (directive.selector !== null) {
matcher.addSelectables(CssSelector.parse(directive.selector), directive);
@ -436,9 +444,13 @@ export class ComponentDecoratorHandler implements
return;
}
if (meta.isPoisoned && !this.usePoisonedData) {
return;
}
const scope = this.typeCheckScopes.getTypeCheckScope(node);
if (scope === 'error') {
// Don't type-check components that had errors in their scopes.
if (scope.isPoisoned && !this.usePoisonedData) {
// Don't type-check components that had errors in their scopes, unless requested.
return;
}
@ -450,6 +462,10 @@ export class ComponentDecoratorHandler implements
resolve(node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>):
ResolveResult<ComponentResolutionData> {
if (analysis.isPoisoned && !this.usePoisonedData) {
return {};
}
const context = node.getSourceFile();
// Check whether this component was registered with an NgModule. If so, it should be compiled
// under that module's compilation scope.
@ -462,7 +478,7 @@ export class ComponentDecoratorHandler implements
wrapDirectivesAndPipesInClosure: false,
};
if (scope !== null && scope !== 'error') {
if (scope !== null && (!scope.compilation.isPoisoned || this.usePoisonedData)) {
// Replace the empty components and directives from the analyze() step with a fully expanded
// scope. This is possible now because during resolve() the whole compilation unit has been
// fully analyzed.

5
packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
View File

@ -41,6 +41,7 @@ export interface DirectiveHandlerData {
providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
inputs: ClassPropertyMapping;
outputs: ClassPropertyMapping;
isPoisoned: boolean;
}
export class DirectiveDecoratorHandler implements
@ -106,7 +107,8 @@ export class DirectiveDecoratorHandler implements
this.annotateForClosureCompiler),
baseClass: readBaseClass(node, this.reflector, this.evaluator),
typeCheckMeta: extractDirectiveTypeCheckMeta(node, directiveResult.inputs, this.reflector),
providersRequiringFactory
providersRequiringFactory,
isPoisoned: false,
}
};
}
@ -126,6 +128,7 @@ export class DirectiveDecoratorHandler implements
isComponent: false,
baseClass: analysis.baseClass,
...analysis.typeCheckMeta,
isPoisoned: analysis.isPoisoned,
});
this.injectableRegistry.registerInjectable(node);

5
packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
View File

@ -336,7 +336,7 @@ export class NgModuleDecoratorHandler implements
injectorImports: [],
};
if (scope !== null && scope !== 'error') {
if (scope !== null && !scope.compilation.isPoisoned) {
// Using the scope information, extend the injector's imports using the modules that are
// specified as module exports.
const context = getSourceFile(node);
@ -361,7 +361,8 @@ export class NgModuleDecoratorHandler implements
return {diagnostics};
}
if (scope === null || scope === 'error' || scope.reexports === null) {
if (scope === null || scope.compilation.isPoisoned || scope.exported.isPoisoned ||
scope.reexports === null) {
return {data};
} else {
return {

24
packages/compiler-cli/src/ngtsc/annotations/src/typecheck_scopes.ts
View File

@ -33,6 +33,12 @@ export interface TypeCheckScope {
* The schemas that are used in this scope.
*/
schemas: SchemaMetadata[];
/**
* Whether the original compilation scope which produced this `TypeCheckScope` was itself poisoned
* (contained semantic errors during its production).
*/
isPoisoned: boolean;
}
/**
@ -57,15 +63,18 @@ export class TypeCheckScopes {
* contains an error, then 'error' is returned. If the component is not declared in any NgModule,
* an empty type-check scope is returned.
*/
getTypeCheckScope(node: ClassDeclaration): TypeCheckScope|'error' {
getTypeCheckScope(node: ClassDeclaration): TypeCheckScope {
const matcher = new SelectorMatcher<DirectiveMeta>();
const pipes = new Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>();
const scope = this.scopeReader.getScopeForComponent(node);
if (scope === null) {
return {matcher, pipes, schemas: []};
} else if (scope === 'error') {
return scope;
return {
matcher,
pipes,
schemas: [],
isPoisoned: false,
};
}
if (this.scopeCache.has(scope.ngModule)) {
@ -87,7 +96,12 @@ export class TypeCheckScopes {
pipes.set(name, ref as Reference<ClassDeclaration<ts.ClassDeclaration>>);
}
const typeCheckScope: TypeCheckScope = {matcher, pipes, schemas: scope.schemas};
const typeCheckScope: TypeCheckScope = {
matcher,
pipes,
schemas: scope.schemas,
isPoisoned: scope.compilation.isPoisoned || scope.exported.isPoisoned,
};
this.scopeCache.set(scope.ngModule, typeCheckScope);
return typeCheckScope;
}

1
packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts
View File

@ -55,6 +55,7 @@ function setup(program: ts.Program, options: ts.CompilerOptions, host: ts.Compil
/* isCore */ false, new StubResourceLoader(), /* rootDirs */['/'],
/* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true,
/* enableI18nLegacyMessageIdFormat */ false,
/* usePoisonedData */ false,
/* i18nNormalizeLineEndingsInICUs */ undefined, moduleResolver, cycleAnalyzer, refEmitter,
NOOP_DEFAULT_IMPORT_RECORDER, /* depTracker */ null, injectableRegistry,
/* annotateForClosureCompiler */ false);

3
packages/compiler-cli/src/ngtsc/core/src/compiler.ts
View File

@ -106,6 +106,7 @@ export class NgCompiler {
private typeCheckingProgramStrategy: TypeCheckingProgramStrategy,
private incrementalStrategy: IncrementalBuildStrategy,
private enableTemplateTypeChecker: boolean,
private usePoisonedData: boolean,
oldProgram: ts.Program|null = null,
private perfRecorder: PerfRecorder = NOOP_PERF_RECORDER,
) {
@ -767,7 +768,7 @@ export class NgCompiler {
reflector, evaluator, metaRegistry, metaReader, scopeReader, scopeRegistry,
resourceRegistry, isCore, this.resourceManager, this.adapter.rootDirs,
this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false,
this.options.enableI18nLegacyMessageIdFormat !== false,
this.options.enableI18nLegacyMessageIdFormat !== false, this.usePoisonedData,
this.options.i18nNormalizeLineEndingsInICUs, this.moduleResolver, this.cycleAnalyzer,
refEmitter, defaultImportTracker, this.incrementalDriver.depGraph, injectableRegistry,
this.closureCompilerEnabled),

18
packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts
View File

@ -51,7 +51,8 @@ runInEachFileSystem(() => {
const program = ts.createProgram({host, options, rootNames: host.inputFiles});
const compiler = new NgCompiler(
host, options, program, new ReusedProgramStrategy(program, host, options, []),
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
/* usePoisonedData */ false);
const diags = compiler.getDiagnostics(getSourceFileOrError(program, COMPONENT));
expect(diags.length).toBe(1);
@ -100,7 +101,8 @@ runInEachFileSystem(() => {
const CmpC = getClass(getSourceFileOrError(program, cmpCFile), 'CmpC');
const compiler = new NgCompiler(
host, options, program, new ReusedProgramStrategy(program, host, options, []),
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
/* usePoisonedData */ false);
const components = compiler.getComponentsWithTemplateFile(templateFile);
expect(components).toEqual(new Set([CmpA, CmpC]));
});
@ -151,7 +153,8 @@ runInEachFileSystem(() => {
const CmpC = getClass(getSourceFileOrError(program, cmpCFile), 'CmpC');
const compiler = new NgCompiler(
host, options, program, new ReusedProgramStrategy(program, host, options, []),
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
/* usePoisonedData */ false);
const components = compiler.getComponentsWithStyleFile(styleFile);
expect(components).toEqual(new Set([CmpA, CmpC]));
});
@ -184,7 +187,8 @@ runInEachFileSystem(() => {
const CmpA = getClass(getSourceFileOrError(program, cmpAFile), 'CmpA');
const compiler = new NgCompiler(
host, options, program, new ReusedProgramStrategy(program, host, options, []),
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
/* usePoisonedData */ false);
const resources = compiler.getComponentResources(CmpA);
expect(resources).not.toBeNull();
const {template, styles} = resources!;
@ -219,7 +223,8 @@ runInEachFileSystem(() => {
const CmpA = getClass(getSourceFileOrError(program, cmpAFile), 'CmpA');
const compiler = new NgCompiler(
host, options, program, new ReusedProgramStrategy(program, host, options, []),
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
/* usePoisonedData */ false);
const resources = compiler.getComponentResources(CmpA);
expect(resources).not.toBeNull();
const {styles} = resources!;
@ -250,7 +255,8 @@ runInEachFileSystem(() => {
const program = ts.createProgram({host, options, rootNames: host.inputFiles});
const compiler = new NgCompiler(
host, options, program, new ReusedProgramStrategy(program, host, options, []),
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
/* usePoisonedData */ false);
const deps = compiler.getResourceDependencies(getSourceFileOrError(program, COMPONENT));
expect(deps.length).toBe(2);

6
packages/compiler-cli/src/ngtsc/metadata/src/api.ts
View File

@ -108,6 +108,12 @@ export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta {
* another type, it could not statically determine the base class.
*/
baseClass: Reference<ClassDeclaration>|'dynamic'|null;
/**
* Whether the directive had some issue with its declaration that means it might not have complete
* and reliable metadata.
*/
isPoisoned: boolean;
}
/**

1
packages/compiler-cli/src/ngtsc/metadata/src/dts.ts
View File

@ -92,6 +92,7 @@ export class DtsMetadataReader implements MetadataReader {
queries: readStringArrayType(def.type.typeArguments[5]),
...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector),
baseClass: readBaseClass(clazz, this.checker, this.reflector),
isPoisoned: false,
};
}

3
packages/compiler-cli/src/ngtsc/program.ts
View File

@ -100,7 +100,8 @@ export class NgtscProgram implements api.Program {
// Create the NgCompiler which will drive the rest of the compilation.
this.compiler = new NgCompiler(
this.host, options, this.tsProgram, reusedProgramStrategy, this.incrementalStrategy,
/** enableTemplateTypeChecker */ false, reuseProgram, this.perfRecorder);
/** enableTemplateTypeChecker */ false, /* usePoisonedData */ false, reuseProgram,
this.perfRecorder);
}
getTsProgram(): ts.Program {

6
packages/compiler-cli/src/ngtsc/scope/src/api.ts
View File

@ -29,6 +29,12 @@ export interface ScopeData {
* NgModules which contributed to the scope of the module.
*/
ngModules: ClassDeclaration[];
/**
* Whether some module or component in this scope contains errors and is thus semantically
* unreliable.
*/
isPoisoned: boolean;
}
/**

4
packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts
View File

@ -13,7 +13,7 @@ import {LocalModuleScope} from './local';
* Read information about the compilation scope of components.
*/
export interface ComponentScopeReader {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error';
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null;
/**
* Get the `RemoteScope` required for this component, if any.
@ -34,7 +34,7 @@ export interface ComponentScopeReader {
export class CompoundComponentScopeReader implements ComponentScopeReader {
constructor(private readers: ComponentScopeReader[]) {}
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error' {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
for (const reader of this.readers) {
const meta = reader.getScopeForComponent(clazz);
if (meta !== null) {

1
packages/compiler-cli/src/ngtsc/scope/src/dependency.ts
View File

@ -132,6 +132,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
directives,
pipes,
ngModules: Array.from(ngModules),
isPoisoned: false,
},
};
this.cache.set(clazz, exportScope);

118
packages/compiler-cli/src/ngtsc/scope/src/local.ts
View File

@ -89,11 +89,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
/**
* A cache of calculated `LocalModuleScope`s for each NgModule declared in the current program.
*
* A value of `undefined` indicates the scope was invalid and produced errors (therefore,
* diagnostics should exist in the `scopeErrors` map).
*/
private cache = new Map<ClassDeclaration, LocalModuleScope|undefined|null>();
private cache = new Map<ClassDeclaration, LocalModuleScope|null>();
/**
* Tracks the `RemoteScope` for components requiring "remote scoping".
@ -111,13 +109,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
private scopeErrors = new Map<ClassDeclaration, ts.Diagnostic[]>();
/**
* Tracks which NgModules are unreliable due to errors within their declarations.
*
* This provides a unified view of which modules have errors, across all of the different
* diagnostic categories that can be produced. Theoretically this can be inferred from the other
* properties of this class, but is tracked explicitly to simplify the logic.
* Tracks which NgModules have directives/pipes that are declared in more than one module.
*/
private taintedModules = new Set<ClassDeclaration>();
private modulesWithStructuralErrors = new Set<ClassDeclaration>();
constructor(
private localReader: MetadataReader, private dependencyScopeReader: DtsModuleScopeResolver,
@ -141,7 +135,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
registerPipeMetadata(pipe: PipeMeta): void {}
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error' {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
const scope = !this.declarationToModule.has(clazz) ?
null :
this.getScopeOfModule(this.declarationToModule.get(clazz)!.ngModule);
@ -171,17 +165,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
* `LocalModuleScope` for the given NgModule if one can be produced, `null` if no scope was ever
* defined, or the string `'error'` if the scope contained errors.
*/
getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope|'error'|null {
const scope = this.moduleToRef.has(clazz) ?
getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope|null {
return this.moduleToRef.has(clazz) ?
this.getScopeOfModuleReference(this.moduleToRef.get(clazz)!) :
null;
// If the NgModule class is marked as tainted, consider it an error.
if (this.taintedModules.has(clazz)) {
return 'error';
}
// Translate undefined -> 'error'.
return scope !== undefined ? scope : 'error';
}
/**
@ -207,7 +194,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
const scopes: CompilationScope[] = [];
this.declarationToModule.forEach((declData, declaration) => {
const scope = this.getScopeOfModule(declData.ngModule);
if (scope !== null && scope !== 'error') {
if (scope !== null) {
scopes.push({declaration, ngModule: declData.ngModule, ...scope.compilation});
}
});
@ -236,9 +223,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
const duplicateDeclMap = new Map<ClassDeclaration, DeclarationData>();
const firstDeclData = this.declarationToModule.get(decl.node)!;
// Mark both modules as tainted, since their declarations are missing a component.
this.taintedModules.add(firstDeclData.ngModule);
this.taintedModules.add(ngModule);
// Mark both modules as having duplicate declarations.
this.modulesWithStructuralErrors.add(firstDeclData.ngModule);
this.modulesWithStructuralErrors.add(ngModule);
// Being detected as a duplicate means there are two NgModules (for now) which declare this
// directive/pipe. Add both of them to the duplicate tracking map.
@ -256,16 +243,11 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
}
/**
* Implementation of `getScopeOfModule` which accepts a reference to a class and differentiates
* between:
*
* * no scope being available (returns `null`)
* * a scope being produced with errors (returns `undefined`).
* Implementation of `getScopeOfModule` which accepts a reference to a class.
*/
private getScopeOfModuleReference(ref: Reference<ClassDeclaration>): LocalModuleScope|null
|undefined {
private getScopeOfModuleReference(ref: Reference<ClassDeclaration>): LocalModuleScope|null {
if (this.cache.has(ref.node)) {
return this.cache.get(ref.node);
return this.cache.get(ref.node)!;
}
// Seal the registry to protect the integrity of the `LocalModuleScope` cache.
@ -315,20 +297,33 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
// c) If it's neither an NgModule nor a directive/pipe in the compilation scope, then this
// is an error.
//
let isPoisoned = false;
if (this.modulesWithStructuralErrors.has(ngModule.ref.node)) {
// If the module contains declarations that are duplicates, then it's considered poisoned.
isPoisoned = true;
}
// 1) process imports.
for (const decl of ngModule.imports) {
const importScope = this.getExportedScope(decl, diagnostics, ref.node, 'import');
if (importScope === null) {
// An import wasn't an NgModule, so record an error.
diagnostics.push(invalidRef(ref.node, decl, 'import'));
isPoisoned = true;
continue;
} else if (importScope === undefined) {
} else if (importScope === 'invalid' || importScope.exported.isPoisoned) {
// An import was an NgModule but contained errors of its own. Record this as an error too,
// because this scope is always going to be incorrect if one of its imports could not be
// read.
diagnostics.push(invalidTransitiveNgModuleRef(ref.node, decl, 'import'));
isPoisoned = true;
if (importScope === 'invalid') {
continue;
}
}
for (const directive of importScope.exported.directives) {
compilationDirectives.set(directive.ref.node, directive);
}
@ -346,11 +341,12 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
const pipe = this.localReader.getPipeMetadata(decl);
if (directive !== null) {
compilationDirectives.set(decl.node, {...directive, ref: decl});
if (directive.isPoisoned) {
isPoisoned = true;
}
} else if (pipe !== null) {
compilationPipes.set(decl.node, {...pipe, ref: decl});
} else {
this.taintedModules.add(ngModule.ref.node);
const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations!);
diagnostics.push(makeDiagnostic(
ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode,
@ -361,6 +357,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
`Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`,
[makeRelatedInformation(
decl.node.name, `'${decl.node.name.text}' is declared here.`)]));
isPoisoned = true;
continue;
}
@ -374,22 +371,26 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
// imported types.
for (const decl of ngModule.exports) {
// Attempt to resolve decl as an NgModule.
const importScope = this.getExportedScope(decl, diagnostics, ref.node, 'export');
if (importScope === undefined) {
const exportScope = this.getExportedScope(decl, diagnostics, ref.node, 'export');
if (exportScope === 'invalid' || (exportScope !== null && exportScope.exported.isPoisoned)) {
// An export was an NgModule but contained errors of its own. Record this as an error too,
// because this scope is always going to be incorrect if one of its exports could not be
// read.
diagnostics.push(invalidTransitiveNgModuleRef(ref.node, decl, 'export'));
isPoisoned = true;
if (exportScope === 'invalid') {
continue;
} else if (importScope !== null) {
}
} else if (exportScope !== null) {
// decl is an NgModule.
for (const directive of importScope.exported.directives) {
for (const directive of exportScope.exported.directives) {
exportDirectives.set(directive.ref.node, directive);
}
for (const pipe of importScope.exported.pipes) {
for (const pipe of exportScope.exported.pipes) {
exportPipes.set(pipe.ref.node, pipe);
}
for (const exportedModule of importScope.exported.ngModules) {
for (const exportedModule of exportScope.exported.ngModules) {
exportedModules.add(exportedModule);
}
} else if (compilationDirectives.has(decl.node)) {
@ -408,30 +409,20 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
} else {
diagnostics.push(invalidRef(ref.node, decl, 'export'));
}
isPoisoned = true;
continue;
}
}
const exported = {
const exported: ScopeData = {
directives: Array.from(exportDirectives.values()),
pipes: Array.from(exportPipes.values()),
ngModules: Array.from(exportedModules),
isPoisoned,
};
const reexports = this.getReexports(ngModule, ref, declared, exported, diagnostics);
// Check if this scope had any errors during production.
if (diagnostics.length > 0) {
// Cache undefined, to mark the fact that the scope is invalid.
this.cache.set(ref.node, undefined);
// Save the errors for retrieval.
this.scopeErrors.set(ref.node, diagnostics);
// Mark this module as being tainted.
this.taintedModules.add(ref.node);
return undefined;
}
// Finally, produce the `LocalModuleScope` with both the compilation and export scopes.
const scope: LocalModuleScope = {
@ -440,11 +431,22 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
directives: Array.from(compilationDirectives.values()),
pipes: Array.from(compilationPipes.values()),
ngModules: Array.from(compilationModules),
isPoisoned,
},
exported,
reexports,
schemas: ngModule.schemas,
};
// Check if this scope had any errors during production.
if (diagnostics.length > 0) {
// Save the errors for retrieval.
this.scopeErrors.set(ref.node, diagnostics);
// Mark this module as being tainted.
this.modulesWithStructuralErrors.add(ref.node);
}
this.cache.set(ref.node, scope);
return scope;
}
@ -471,15 +473,15 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
* The NgModule in question may be declared locally in the current ts.Program, or it may be
* declared in a .d.ts file.
*
* @returns `null` if no scope could be found, or `undefined` if an invalid scope
* was found.
* @returns `null` if no scope could be found, or `'invalid'` if the `Reference` is not a valid
* NgModule.
*
* May also contribute diagnostics of its own by adding to the given `diagnostics`
* array parameter.
*/
private getExportedScope(
ref: Reference<ClassDeclaration>, diagnostics: ts.Diagnostic[],
ownerForErrors: DeclarationNode, type: 'import'|'export'): ExportScope|null|undefined {
ownerForErrors: DeclarationNode, type: 'import'|'export'): ExportScope|null|'invalid' {
if (ref.node.getSourceFile().isDeclarationFile) {
// The NgModule is declared in a .d.ts file. Resolve it with the `DependencyScopeReader`.
if (!ts.isClassDeclaration(ref.node)) {
@ -491,7 +493,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
code, identifierOfNode(ref.node) || ref.node,
`Appears in the NgModule.${type}s of ${
nodeNameForError(ownerForErrors)}, but could not be resolved to an NgModule`));
return undefined;
return 'invalid';
}
return this.dependencyScopeReader.resolve(ref);
} else {

3
packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts
View File

@ -219,7 +219,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
expect(scopeRegistry.getScopeOfModule(ModuleA.node)).toBe('error');
expect(scopeRegistry.getScopeOfModule(ModuleA.node)!.compilation.isPoisoned).toBeTrue();
// ModuleA should have associated diagnostics as it exports `Dir` without declaring it.
expect(scopeRegistry.getDiagnosticsOfModule(ModuleA.node)).not.toBeNull();
@ -248,6 +248,7 @@ function fakeDirective(ref: Reference<ClassDeclaration>): DirectiveMeta {
undeclaredInputFields: new Set<string>(),
isGeneric: false,
baseClass: null,
isPoisoned: false,
};
}

2
packages/compiler-cli/src/ngtsc/tsc_plugin.ts
View File

@ -103,7 +103,7 @@ export class NgTscPlugin implements TscPlugin {
this._compiler = new NgCompiler(
this.host, this.options, program, typeCheckStrategy,
new PatchedProgramIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
oldProgram, NOOP_PERF_RECORDER);
/* usePoisonedData */ false, oldProgram, NOOP_PERF_RECORDER);
return {
ignoreForDiagnostics: this._compiler.ignoreForDiagnostics,
ignoreForEmit: this._compiler.ignoreForEmit,

12
packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts
View File

@ -502,16 +502,17 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
throw new Error(`AssertionError: components must have names`);
}
const scope = this.componentScopeReader.getScopeForComponent(component);
if (scope === null) {
return null;
}
const data: ScopeData = {
directives: [],
pipes: [],
isPoisoned: scope.compilation.isPoisoned,
};
const scope = this.componentScopeReader.getScopeForComponent(component);
if (scope === null || scope === 'error') {
return null;
}
const typeChecker = this.typeCheckingStrategy.getProgram().getTypeChecker();
for (const dir of scope.exported.directives) {
if (dir.selector === null) {
@ -739,4 +740,5 @@ class SingleShimTypeCheckingHost extends SingleFileTypeCheckingHost {
interface ScopeData {
directives: DirectiveInScope[];
pipes: PipeInScope[];
isPoisoned: boolean;
}

2
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
View File

@ -159,7 +159,7 @@ export class SymbolBuilder {
private getDirectiveModule(declaration: ts.ClassDeclaration): ClassDeclaration|null {
const scope = this.componentScopeReader.getScopeForComponent(declaration as ClassDeclaration);
if (scope === null || scope === 'error') {
if (scope === null) {
return null;
}
return scope.ngModule;

3
packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
View File

@ -435,6 +435,7 @@ export function setup(targets: TypeCheckingTarget[], overrides: {
directives: [],
ngModules: [],
pipes: [],
isPoisoned: false,
};
return {
ngModule,
@ -532,6 +533,7 @@ function makeScope(program: ts.Program, sf: ts.SourceFile, decls: TestDeclaratio
ngModules: [],
directives: [],
pipes: [],
isPoisoned: false,
};
for (const decl of decls) {
@ -561,6 +563,7 @@ function makeScope(program: ts.Program, sf: ts.SourceFile, decls: TestDeclaratio
stringLiteralInputFields: new Set(decl.stringLiteralInputFields ?? []),
undeclaredInputFields: new Set(decl.undeclaredInputFields ?? []),
isGeneric: decl.isGeneric ?? false,
isPoisoned: false,
});
} else if (decl.type === 'pipe') {
scope.pipes.push({

1
packages/language-service/ivy/compiler_factory.ts
View File

@ -45,6 +45,7 @@ export class CompilerFactory {
this.programStrategy,
this.incrementalStrategy,
true, // enableTemplateTypeChecker
true, // usePoisonedData
this.lastKnownProgram,
undefined, // perfRecorder (use default)
);

63
packages/language-service/ivy/test/compiler_spec.ts Normal file
View File

@ -0,0 +1,63 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {absoluteFrom} from '@angular/compiler-cli/src/ngtsc/file_system';
import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing';
import {LanguageServiceTestEnvironment} from './env';
describe('language-service/compiler integration', () => {
beforeEach(() => {
initMockFileSystem('Native');
});
it('should show type-checking errors from components with poisoned scopes', () => {
// Normally, the Angular compiler suppresses errors from components that belong to NgModules
// which themselves have errors (such scopes are considered "poisoned"), to avoid overwhelming
// the user with secondary errors that stem from a primary root cause. However, this prevents
// the generation of type check blocks and other metadata within the compiler which drive the
// Language Service's understanding of components. Therefore in the Language Service, the
// compiler is configured to make use of such data even if it's "poisoned". This test verifies
// that a component declared in an NgModule with a faulty import still generates template
// diagnostics.
const file = absoluteFrom('/test.ts');
const env = LanguageServiceTestEnvironment.setup([{
name: file,
contents: `
import {Component, Directive, Input, NgModule} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '<div [dir]="3"></div>',
})
export class Cmp {}
@Directive({
selector: '[dir]',
})
export class Dir {
@Input() dir!: string;
}
export class NotAModule {}
@NgModule({
declarations: [Cmp, Dir],
imports: [NotAModule],
})
export class Mod {}
`,
isRoot: true,
}]);
const diags = env.ngLS.getSemanticDiagnostics(file);
expect(diags.map(diag => diag.messageText))
.toContain(`Type 'number' is not assignable to type 'string'.`);
});
});