diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index b6cd153955..bc9a4fc646 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -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), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 53253cacb0..2a88fb2bec 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -72,6 +72,8 @@ export interface ComponentAnalysisData { viewProvidersRequiringFactory: Set>|null; resources: ComponentResources; + + isPoisoned: boolean; } export type ComponentResolutionData = Pick; @@ -88,7 +90,7 @@ export class ComponentDecoratorHandler implements private resourceRegistry: ResourceRegistry, private isCore: boolean, private resourceLoader: ResourceLoader, private rootDirs: ReadonlyArray, 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) { + if (analysis.isPoisoned && !this.usePoisonedData) { + return null; + } const scope = this.scopeReader.getScopeForComponent(node); const selector = analysis.meta.selector; const matcher = new SelectorMatcher(); - if (scope === 'error') { - // Don't bother indexing components which had erroneous scopes. - return null; - } - 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; + } + 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): ResolveResult { + 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. diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 304da8b032..f07b3d0411 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -41,6 +41,7 @@ export interface DirectiveHandlerData { providersRequiringFactory: Set>|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); diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 67ad6373dd..ef01a4bdce 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -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 { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/typecheck_scopes.ts b/packages/compiler-cli/src/ngtsc/annotations/src/typecheck_scopes.ts index aada9e0df4..f3319f9e57 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/typecheck_scopes.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/typecheck_scopes.ts @@ -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(); const pipes = new Map>>(); 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>); } - 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; } diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index 903745ba1c..b3c8134e0d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -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); diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 4a26f2f9a8..a566ed8ae6 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -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), diff --git a/packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts b/packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts index 37be7020c6..e9ff2151be 100644 --- a/packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts +++ b/packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts @@ -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); diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 3ec001db95..37accc62bb 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -108,6 +108,12 @@ export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta { * another type, it could not statically determine the base class. */ baseClass: Reference|'dynamic'|null; + + /** + * Whether the directive had some issue with its declaration that means it might not have complete + * and reliable metadata. + */ + isPoisoned: boolean; } /** diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts index 4bbdc14fd8..5c693a92e4 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -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, }; } diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 13e209fc0a..1af0e8643a 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -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 { diff --git a/packages/compiler-cli/src/ngtsc/scope/src/api.ts b/packages/compiler-cli/src/ngtsc/scope/src/api.ts index e293acda1c..70ba24bc13 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/api.ts @@ -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; } /** diff --git a/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts b/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts index 0f4d63913f..d1f06409d7 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts @@ -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) { diff --git a/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts b/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts index d6c073dc05..5bc06f47dc 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts @@ -132,6 +132,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver { directives, pipes, ngModules: Array.from(ngModules), + isPoisoned: false, }, }; this.cache.set(clazz, exportScope); diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 30c3a1ce26..464756104d 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -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(); + private cache = new Map(); /** * Tracks the `RemoteScope` for components requiring "remote scoping". @@ -111,13 +109,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop private scopeErrors = new Map(); /** - * 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(); + private modulesWithStructuralErrors = new Set(); 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(); 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): LocalModuleScope|null - |undefined { + private getScopeOfModuleReference(ref: Reference): 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')); - continue; + 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')); - continue; - } else if (importScope !== null) { + isPoisoned = true; + + if (exportScope === 'invalid') { + continue; + } + } 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, 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 { diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index 0216fd8864..dd289671fe 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -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): DirectiveMeta { undeclaredInputFields: new Set(), isGeneric: false, baseClass: null, + isPoisoned: false, }; } diff --git a/packages/compiler-cli/src/ngtsc/tsc_plugin.ts b/packages/compiler-cli/src/ngtsc/tsc_plugin.ts index dcd62d5093..f76d4eb50c 100644 --- a/packages/compiler-cli/src/ngtsc/tsc_plugin.ts +++ b/packages/compiler-cli/src/ngtsc/tsc_plugin.ts @@ -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, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index cd009e34ef..4260a7b402 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -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; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index df08a4016f..bf3df64c20 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts @@ -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; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 07bd93cb96..aab925cfd0 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -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({ diff --git a/packages/language-service/ivy/compiler_factory.ts b/packages/language-service/ivy/compiler_factory.ts index af2498d16e..b42b1e660e 100644 --- a/packages/language-service/ivy/compiler_factory.ts +++ b/packages/language-service/ivy/compiler_factory.ts @@ -45,6 +45,7 @@ export class CompilerFactory { this.programStrategy, this.incrementalStrategy, true, // enableTemplateTypeChecker + true, // usePoisonedData this.lastKnownProgram, undefined, // perfRecorder (use default) ); diff --git a/packages/language-service/ivy/test/compiler_spec.ts b/packages/language-service/ivy/test/compiler_spec.ts new file mode 100644 index 0000000000..f5acc12001 --- /dev/null +++ b/packages/language-service/ivy/test/compiler_spec.ts @@ -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: '
', + }) + 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'.`); + }); +});