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

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

Hide file tree

Showing 21 changed files with 156 additions and 92 deletions.
diff --git 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 TemplateMapping(), 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
@@ -70,6 +70,8 @@ export interface ComponentAnalysisData {
    * require an Angular factory definition at runtime.
    */
   viewProvidersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
+
+  isPoisoned: boolean;
 }
 
 export type ComponentResolutionData = Pick<R3ComponentMetadata, ComponentMetadataResolvedFields>;
@@ -86,7 +88,7 @@ export class ComponentDecoratorHandler implements
       private templateMapping: TemplateMapping, 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,
@@ -359,6 +361,7 @@ export class ComponentDecoratorHandler implements
         template,
         providersRequiringFactory,
         viewProvidersRequiringFactory,
+        isPoisoned: diagnostics !== undefined && diagnostics.length > 0,
       },
       diagnostics,
     };
@@ -383,6 +386,7 @@ export class ComponentDecoratorHandler implements
       isComponent: true,
       baseClass: analysis.baseClass,
       ...analysis.typeCheckMeta,
+      isPoisoned: analysis.isPoisoned,
     });
 
     if (!analysis.template.isInline) {
@@ -394,15 +398,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.
-      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);
@@ -429,9 +437,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;
     }
 
@@ -443,6 +455,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.
@@ -455,7 +471,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
@@ -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);

diff --git 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
@@ -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;
   }

diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts
@@ -73,6 +73,7 @@ runInEachFileSystem(() => {
           /* isCore */ false, new NoopResourceLoader(), /* 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
@@ -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,
   ) {
@@ -742,7 +743,7 @@ export class NgCompiler {
           reflector, evaluator, metaRegistry, metaReader, scopeReader, scopeRegistry,
           templateMapping, 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
@@ -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]));
       });
@@ -129,7 +131,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
@@ -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;
 }
 
 /**

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -132,6 +132,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
         directives,
         pipes,
         ngModules: Array.from(ngModules),
+        isPoisoned: false,
       },
     };
     this.cache.set(clazz, exportScope);