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

Author: alxhub

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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);