8000 fix(compiler-cli): track poisoned scopes with a flag (#39967) · angular/angular@178cc51 · GitHub
[go: up one dir, main page]
Skip to content

Commit 178cc51

Browse files
alxhubmhevery
alxhub
authored and
mhevery
committed
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
1 parent 0aa35ec commit 178cc51

File tree

21 files changed

+156
-92
lines changed
  • test
  • language-service/ivy

    • 21 files changed

    +156
    -92
    lines changed

    ‎packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts

    +1

    ‎packages/compiler-cli/src/ngtsc/annotations/src/component.ts

    +25-9
    Original file line numberDiff line numberDiff line change
    @@ -97,6 +97,7 @@ export class DecorationAnalyzer {
    9797
    this.scopeRegistry, this.scopeRegistry, new TemplateMapping(), this.isCore,
    9898
    this.resourceManager, this.rootDirs, !!this.compilerOptions.preserveWhitespaces,
    9999
    /* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat,
    100+
    /* usePoisonedData */ false,
    100101
    /* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
    101102
    this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, NOOP_DEPENDENCY_TRACKER,
    102103
    this.injectableRegistry, !!this.compilerOptions.annotateForClosureCompiler),
    Original file line numberDiff line numberDiff line change
    @@ -70,6 +70,8 @@ export interface ComponentAnalysisData {
    7070
    * require an Angular factory definition at runtime.
    7171
    */
    7272
    viewProvidersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
    73+
    74+
    isPoisoned: boolean;
    7375
    }
    7476

    7577
    export type ComponentResolutionData = Pick<R3ComponentMetadata, ComponentMetadataResolvedFields>;
    @@ -86,7 +88,7 @@ export class ComponentDecoratorHandler implements
    8688
    private templateMapping: TemplateMapping, private isCore: boolean,
    8789
    private resourceLoader: ResourceLoader, private rootDirs: ReadonlyArray<string>,
    8890
    private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean,
    89-
    private enableI18nLegacyMessageIdFormat: boolean,
    91+
    private enableI18nLegacyMessageIdFormat: boolean, private usePoisonedData: boolean,
    9092
    private i18nNormalizeLineEndingsInICUs: boolean|undefined,
    9193
    private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer,
    9294
    private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder,
    @@ -359,6 +361,7 @@ export class ComponentDecoratorHandler implements
    359361
    template,
    360362
    providersRequiringFactory,
    361363
    viewProvidersRequiringFactory,
    364+
    isPoisoned: diagnostics !== undefined && diagnostics.length > 0,
    362365
    },
    363366
    diagnostics,
    364367
    };
    @@ -383,6 +386,7 @@ export class ComponentDecoratorHandler implements
    383386
    isComponent: true,
    384387
    baseClass: analysis.baseClass,
    385388
    ...analysis.typeCheckMeta,
    389+
    isPoisoned: analysis.isPoisoned,
    386390
    });
    387391

    388392
    if (!analysis.template.isInline) {
    @@ -394,15 +398,19 @@ export class ComponentDecoratorHandler implements
    394398

    395399
    index(
    396400
    context: IndexingContext, node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>) {
    401+
    if (analysis.isPoisoned && !this.usePoisonedData) {
    402+
    return null;
    403+
    }
    397404
    const scope = this.scopeReader.getScopeForComponent(node);
    398405
    const selector = analysis.meta.selector;
    399406
    const matcher = new SelectorMatcher<DirectiveMeta>();
    400-
    if (scope === 'error') {
    401-
    // Don't bother indexing components which had erroneous scopes.
    402-
    return null;
    403-
    }
    404-
    405407
    if (scope !== null) {
    408+
    if ((scope.compilation.isPoisoned || scope.exported.isPoisoned) && !this.usePoisonedData) {
    409+
    // Don't bother indexing components which had erroneous scopes, unless specifically
    410+
    // requested.
    411+
    return null;
    412+
    }
    413+
    406414
    for (const directive of scope.compilation.directives) {
    407415
    if (directive.selector !== null) {
    408416
    matcher.addSelectables(CssSelector.parse(directive.selector), directive);
    @@ -429,9 +437,13 @@ export class ComponentDecoratorHandler implements
    429437
    return;
    430438
    }
    431439

    440+
    if (meta.isPoisoned && !this.usePoisonedData) {
    441+
    return;
    442+
    }
    443+
    432444
    const scope = this.typeCheckScopes.getTypeCheckScope(node);
    433-
    if (scope === 'error') {
    434-
    // Don't type-check components that had errors in their scopes.
    445+
    if (scope.isPoisoned && !this.usePoisonedData) {
    446+
    // Don't type-check components that had errors in their scopes, unless requested.
    435447
    return;
    436448
    }
    437449

    @@ -443,6 +455,10 @@ export class ComponentDecoratorHandler implements
    443455

    444456
    resolve(node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>):
    445457
    ResolveResult<ComponentResolutionData> {
    458+
    if (analysis.isPoisoned && !this.usePoisonedData) {
    459+
    return {};
    460+
    }
    461+
    446462
    const context = node.getSourceFile();
    447463
    // Check whether this component was registered with an NgModule. If so, it should be compiled
    448464
    // under that module's compilation scope.
    @@ -455,7 +471,7 @@ export class ComponentDecoratorHandler implements
    455471
    wrapDirectivesAndPipesInClosure: false,
    456472
    };
    457473

    458-
    if (scope !== null && scope !== 'error') {
    474+
    if (scope !== null && (!scope.compilation.isPoisoned || this.usePoisonedData)) {
    459475
    // Replace the empty components and directives from the analyze() step with a fully expanded
    460476
    // scope. This is possible now because during resolve() the whole compilation unit has been
    461477
    // fully analyzed.

    ‎packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

    +4-1
    Original file line numberDiff line numberDiff line change
    @@ -41,6 +41,7 @@ export interface DirectiveHandlerData {
    4141
    providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
    4242
    inputs: ClassPropertyMapping;
    4343
    outputs: ClassPropertyMapping;
    44+
    isPoisoned: boolean;
    < 1241 code>4445
    }
    4546

    4647
    export class DirectiveDecoratorHandler implements
    @@ -106,7 +107,8 @@ export class DirectiveDecoratorHandler implements
    106107
    this.annotateForClosureCompiler),
    107108
    baseClass: readBaseClass(node, this.reflector, this.evaluator),
    108109
    typeCheckMeta: extractDirectiveTypeCheckMeta(node, directiveResult.inputs, this.reflector),
    109-
    providersRequiringFactory
    110+
    providersRequiringFactory,
    111+
    isPoisoned: false,
    110112
    }
    111113
    };
    112114
    }
    @@ -126,6 +128,7 @@ export class DirectiveDecoratorHandler implements
    126128
    isComponent: false,
    127129
    baseClass: analysis.baseClass,
    128130
    ...analysis.typeCheckMeta,
    131+
    isPoisoned: analysis.isPoisoned,
    129132
    });
    130133

    131134
    this.injectableRegistry.registerInjectable(node);

    ‎packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts

    +3-2
    Original file line numberDiff line numberDiff line change
    @@ -336,7 +336,7 @@ export class NgModuleDecoratorHandler implements
    336336
    injectorImports: [],
    337337
    };
    338338

    339-
    if (scope !== null && scope !== 'error') {
    339+
    if (scope !== null && !scope.compilation.isPoisoned) {
    340340
    // Using the scope information, extend the injector's imports using the modules that are
    341341
    // specified as module exports.
    342342
    const context = getSourceFile(node);
    @@ -361,7 +361,8 @@ export class NgModuleDecoratorHandler implements
    361361
    return {diagnostics};
    362362
    }
    363363

    364-
    if (scope === null || scope === 'error' || scope.reexports === null) {
    364+
    if (scope === null || scope.compilation.isPoisoned || scope.exported.isPoisoned ||
    365+
    scope.reexports === null) {
    365366
    return {data};
    366367
    } else {
    367368
    return {

    ‎packages/compiler-cli/src/ngtsc/annotations/src/typecheck_scopes.ts

    +19-5
    Original file line numberDiff line numberDiff line change
    @@ -33,6 +33,12 @@ export interface TypeCheckScope {
    3333
    * The schemas that are used in this scope.
    3434
    */
    3535
    schemas: SchemaMetadata[];
    36+
    37+
    /**
    38+
    * Whether the original compilation scope which produced this `TypeCheckScope` was itself poisoned
    39+
    * (contained semantic errors during its production).
    40+
    */
    41+
    isPoisoned: boolean;
    3642
    }
    3743

    3844
    /**
    @@ -57,15 +63,18 @@ export class TypeCheckScopes {
    5763
    * contains an error, then 'error' is returned. If the component is not declared in any NgModule,
    5864
    * an empty type-check scope is returned.
    5965
    */
    60-
    getTypeCheckScope(node: ClassDeclaration): TypeCheckScope|'error' {
    66+
    getTypeCheckScope(node: ClassDeclaration): TypeCheckScope {
    6167
    const matcher = new SelectorMatcher<DirectiveMeta>();
    6268
    const pipes = new Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>();
    6369

    6470
    const scope = this.scopeReader.getScopeForComponent(node);
    6571
    if (scope === null) {
    66-
    return {matcher, pipes, schemas: []};
    67-
    } else if (scope === 'error') {
    68-
    return scope;
    72+
    return {
    73+
    matcher,
    74+
    pipes,
    75+
    schemas: [],
    76+
    isPoisoned: false,
    77+
    };
    6978
    }
    7079

    7180
    if (this.scopeCache.has(scope.ngModule)) {
    @@ -87,7 +96,12 @@ export class TypeCheckScopes {
    8796
    pipes.set(name, ref as Reference<ClassDeclaration<ts.ClassDeclaration>>);
    8897
    }
    8998

    90-
    const typeCheckScope: TypeCheckScope = {matcher, pipes, schemas: scope.schemas};
    99+
    const typeCheckScope: TypeCheckScope = {
    100+
    matcher,
    101+
    pipes,
    102+
    schemas: scope.schemas,
    103+
    isPoisoned: scope.compilation.isPoisoned || scope.exported.isPoisoned,
    104+
    };
    91105
    this.scopeCache.set(scope.ngModule, typeCheckScope);
    92106
    return typeCheckScope;
    93107
    }

    ‎packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts

    +1
    Original file line numberDiff line numberDiff line change
    @@ -73,6 +73,7 @@ runInEachFileSystem(() => {
    7373
    /* isCore */ false, new NoopResourceLoader(), /* rootDirs */[''],
    7474
    /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true,
    7575
    /* enableI18nLegacyMessageIdFormat */ false,
    76+
    /* usePoisonedData */ false,
    7677
    /* i18nNormalizeLineEndingsInICUs */ undefined, moduleResolver, cycleAnalyzer, refEmitter,
    7778
    NOOP_DEFAULT_IMPORT_RECORDER, /* depTracker */ null, injectableRegistry,
    7879
    /* annotateForClosureCompiler */ false);

    ‎packages/compiler-cli/src/ngtsc/core/src/compiler.ts

    +2-1
    Original file line numberDiff line numberDiff line change
    @@ -106,6 +106,7 @@ export class NgCompiler {
    106106
    private typeCheckingProgramStrategy: TypeCheckingProgramStrategy,
    107107
    private incrementalStrategy: IncrementalBuildStrategy,
    108108
    private enableTemplateTypeChecker: boolean,
    109+
    private usePoisonedData: boolean,
    109110
    oldProgram: ts.Program|null = null,
    110111
    private perfRecorder: PerfRecorder = NOOP_PERF_RECORDER,
    111112
    ) {
    @@ -742,7 +743,7 @@ export class NgCompiler {
    742743
    reflector, evaluator, metaRegistry, metaReader, scopeReader, scopeRegistry,
    743744
    templateMapping, isCore, this.resourceManager, this.adapter.rootDirs,
    744745
    this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false,
    745-
    this.options.enableI18nLegacyMessageIdFormat !== false,
    746+
    this.options.enableI18nLegacyMessageIdFormat !== false, this.usePoisonedData,
    746747
    this.options.i18nNormalizeLineEndingsInICUs, this.moduleResolver, this.cycleAnalyzer,
    747748
    refEmitter, defaultImportTracker, this.incrementalDriver.depGraph, injectableRegistry,
    748749
    this.closureCompilerEnabled),

    ‎packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts

    +6-3
    Original file line numberDiff line numberDiff line change
    @@ -51,7 +51,8 @@ runInEachFileSystem(() => {
    5151
    const program = ts.createProgram({host, options, rootNames: host.inputFiles});
    5252
    const compiler = new NgCompiler(
    5353
    host, options, program, new ReusedProgramStrategy(program, host, options, []),
    54-
    new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
    54+
    new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
    55+
    /* usePoisonedData */ false);
    5556

    5657
    const diags = compiler.getDiagnostics(getSourceFileOrError(program, COMPONENT));
    5758
    expect(diags.length).toBe(1);
    @@ -100,7 +101,8 @@ runInEachFileSystem(() => {
    100101
    const CmpC = getClass(getSourceFileOrError(program, cmpCFile), 'CmpC');
    101102
    const compiler = new NgCompiler(
    102103
    host, options, program, new ReusedProgramStrategy(program, host, options, []),
    103-
    new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
    104+
    new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
    105+
    /* usePoisonedData */ false);
    104106
    const components = compiler.getComponentsWithTemplateFile(templateFile);
    105107
    expect(components).toEqual(new Set([CmpA, CmpC]));
    106108
    });
    @@ -129,7 +131,8 @@ runInEachFileSystem(() => {
    129131
    const program = ts.createProgram({host, options, rootNames: host.inputFiles});
    130132
    const compiler = new NgCompiler(
    131133
    host, options, program, new ReusedProgramStrategy(program, host, options, []),
    132-
    new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false);
    134+
    new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
    135+
    /* usePoisonedData */ false);
    133136

    134137
    const deps = compiler.getResourceDependencies(getSourceFileOrError(program, COMPONENT));
    135138
    expect(deps.length).toBe(2);

    ‎packages/compiler-cli/src/ngtsc/metadata/src/api.ts

    +6
    Original file line numberDiff line numberDiff line change
    @@ -108,6 +108,12 @@ export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta {
    108108
    * another type, it could not statically determine the base class.
    109109
    */< 48DA /div>
    110110
    baseClass: Reference<ClassDeclaration>|'dynamic'|null;
    111+
    112+
    /**
    113+
    * Whether the directive had some issue with its declaration that means it might not have complete
    114+
    * and reliable metadata.
    115+
    */
    116+
    isPoisoned: boolean;
    111117
    }
    112118

    113119
    /**

    ‎packages/compiler-cli/src/ngtsc/metadata/src/dts.ts

    +1
    Original file line numberDiff line numberDiff line change
    @@ -92,6 +92,7 @@ export class DtsMetadataReader implements MetadataReader {
    9292
    queries: readStringArrayType(def.type.typeArguments[5]),
    9393
    ...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector),
    9494
    baseClass: readBaseClass(clazz, this.checker, this.reflector),
    95+
    isPoisoned: false,
    9596
    };
    9697
    }
    9798

    ‎packages/compiler-cli/src/ngtsc/program.ts

    +2-1
    Original file line numberDiff line numberDiff line change
    @@ -100,7 +100,8 @@ export class NgtscProgram implements api.Program {
    100100
    // Create the NgCompiler which will drive the rest of the compilation.
    101101
    this.compiler = new NgCompiler(
    102102
    this.host, options, this.tsProgram, reusedProgramStrategy, this.incrementalStrategy,
    103-
    /** enableTemplateTypeChecker */ false, reuseProgram, this.perfRecorder);
    103+
    /** enableTemplateTypeChecker */ false, /* usePoisonedData */ false, reuseProgram,
    104+
    this.perfRecorder);
    104105
    }
    105106

    106107
    getTsProgram(): ts.Program {

    ‎packages/compiler-cli/src/ngtsc/scope/src/api.ts

    +6
    Original file line numberDiff line numberDiff line change
    @@ -29,6 +29,12 @@ export interface ScopeData {
    2929
    * NgModules which contributed to the scope of the module.
    3030
    */
    3131
    ngModules: ClassDeclaration[];
    32+
    33+
    /**
    34+
    * Whether some module or component in this scope contains errors and is thus semantically
    35+
    * unreliable.
    36+
    */
    37+
    isPoisoned: boolean;
    3238
    }
    3339

    3440
    /**

    ‎packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts

    +2-2
    Original file line numberDiff line numberDiff line change
    @@ -13,7 +13,7 @@ import {LocalModuleScope} from './local';
    1313
    * Read information about the compilation scope of components.
    1414
    */
    1515
    export interface ComponentScopeReader {
    16-
    getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error';
    16+
    getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null;
    1717

    1818
    /**
    1919
    * Get the `RemoteScope` required for this component, if any.
    @@ -34,7 +34,7 @@ export interface ComponentScopeReader {
    3434
    export class CompoundComponentScopeReader implements ComponentScopeReader {
    3535
    constructor(private readers: ComponentScopeReader[]) {}
    3636

    37-
    getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error' {
    37+
    getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
    3838
    for (const reader of this.readers) {
    3939
    const meta = reader.getScopeForComponent(clazz);
    4040
    if (meta !== null) {

    ‎packages/compiler-cli/src/ngtsc/scope/src/dependency.ts

    +1
    Original file line numberDiff line numberDiff line change
    @@ -132,6 +132,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
    132132
    directives,
    133133
    pipes,
    134134
    ngModules: Array.from(ngModules),
    135+
    isPoisoned: false,
    135136
    },
    136137
    };
    137138
    this.cache.set(clazz, exportScope);

    0 commit comments

    Comments
     (0)
    0