From 08236222026135a4009a4049855c6187e3842c9d Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 25 Nov 2020 15:02:23 -0800 Subject: [PATCH] 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 --- .../ngcc/src/analysis/decoration_analyzer.ts | 1 + .../src/ngtsc/annotations/src/component.ts | 34 +++-- .../src/ngtsc/annotations/src/directive.ts | 5 +- .../src/ngtsc/annotations/src/ng_module.ts | 5 +- .../ngtsc/annotations/src/typecheck_scopes.ts | 24 +++- .../ngtsc/annotations/test/component_spec.ts | 1 + .../src/ngtsc/core/src/compiler.ts | 3 +- .../src/ngtsc/core/test/compiler_test.ts | 18 ++- .../src/ngtsc/metadata/src/api.ts | 6 + .../src/ngtsc/metadata/src/dts.ts | 1 + packages/compiler-cli/src/ngtsc/program.ts | 3 +- .../compiler-cli/src/ngtsc/scope/src/api.ts | 6 + .../src/ngtsc/scope/src/component_scope.ts | 4 +- .../src/ngtsc/scope/src/dependency.ts | 1 + .../compiler-cli/src/ngtsc/scope/src/local.ts | 122 +++++++++--------- .../src/ngtsc/scope/test/local_spec.ts | 3 +- packages/compiler-cli/src/ngtsc/tsc_plugin.ts | 2 +- .../src/ngtsc/typecheck/src/checker.ts | 12 +- .../typecheck/src/template_symbol_builder.ts | 2 +- .../src/ngtsc/typecheck/test/test_utils.ts | 3 + .../language-service/ivy/compiler_factory.ts | 1 + .../ivy/test/compiler_spec.ts | 63 +++++++++ 22 files changed, 225 insertions(+), 95 deletions(-) create mode 100644 packages/language-service/ivy/test/compiler_spec.ts 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'.`); + }); +});