Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport of incremental correctness fixes to patch branch #39967

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -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),
Expand Down
11 changes: 8 additions & 3 deletions packages/compiler-cli/ngcc/src/analysis/migration_host.ts
Expand Up @@ -32,9 +32,14 @@ export class DefaultMigrationHost implements MigrationHost {
const migratedTraits = this.compiler.injectSyntheticDecorator(clazz, decorator, flags);

for (const trait of migratedTraits) {
if (trait.state === TraitState.ERRORED) {
trait.diagnostics =
trait.diagnostics.map(diag => createMigrationDiagnostic(diag, clazz, decorator));
if ((trait.state === TraitState.Analyzed || trait.state === TraitState.Resolved) &&
trait.analysisDiagnostics !== null) {
trait.analysisDiagnostics = trait.analysisDiagnostics.map(
diag => createMigrationDiagnostic(diag, clazz, decorator));
}
if (trait.state === TraitState.Resolved && trait.resolveDiagnostics !== null) {
trait.resolveDiagnostics =
trait.resolveDiagnostics.map(diag => createMigrationDiagnostic(diag, clazz, decorator));
}
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/ngcc/src/analysis/util.ts
Expand Up @@ -18,6 +18,7 @@ class NoopDependencyTracker implements DependencyTracker {
addResourceDependency(): void {}
addTransitiveDependency(): void {}
addTransitiveResources(): void {}
recordDependencyAnalysisFailure(): void {}
}

export const NOOP_DEPENDENCY_TRACKER: DependencyTracker = new NoopDependencyTracker();
Expand Up @@ -79,7 +79,7 @@ runInEachFileSystem(() => {
handler.analyze.and.callFake((decl: DeclarationNode, dec: Decorator) => {
logs.push(`analyze: ${(decl as any).name.text}@${dec.name}`);
return {
analysis: {decoratorName: dec.name},
analysis: !options.analyzeError ? {decoratorName: dec.name} : undefined,
diagnostics: options.analyzeError ? [makeDiagnostic(9999, decl, 'analyze diagnostic')] :
undefined
};
Expand Down Expand Up @@ -407,7 +407,7 @@ runInEachFileSystem(() => {
`,
},
],
{analyzeError: true, resolveError: true});
{analyzeError: true, resolveError: false});
analyzer.analyzeProgram();
expect(diagnosticLogs.length).toEqual(1);
expect(diagnosticLogs[0]).toEqual(jasmine.objectContaining({code: -999999}));
Expand Down
Expand Up @@ -22,6 +22,7 @@ import {Esm2015ReflectionHost} from '../../src/host/esm2015_host';
import {createComponentDecorator} from '../../src/migrations/utils';
import {EntryPointBundle} from '../../src/packages/entry_point_bundle';
import {makeTestEntryPointBundle} from '../helpers/utils';
import {getTraitDiagnostics} from '../host/util';

runInEachFileSystem(() => {
describe('DefaultMigrationHost', () => {
Expand Down Expand Up @@ -79,12 +80,13 @@ runInEachFileSystem(() => {

const record = compiler.recordFor(mockClazz)!;
const migratedTrait = record.traits[0];
if (migratedTrait.state !== TraitState.ERRORED) {
const diagnostics = getTraitDiagnostics(migratedTrait);
if (diagnostics === null) {
return fail('Expected migrated class trait to be in an error state');
}

expect(migratedTrait.diagnostics.length).toBe(1);
expect(ts.flattenDiagnosticMessageText(migratedTrait.diagnostics[0].messageText, '\n'))
expect(diagnostics.length).toBe(1);
expect(ts.flattenDiagnosticMessageText(diagnostics[0].messageText, '\n'))
.toEqual(
`test diagnostic\n` +
` Occurs for @Component decorator inserted by an automatic migration\n` +
Expand Down
Expand Up @@ -19,6 +19,7 @@ import {Esm2015ReflectionHost} from '../../src/host/esm2015_host';
import {createComponentDecorator} from '../../src/migrations/utils';
import {EntryPointBundle} from '../../src/packages/entry_point_bundle';
import {makeTestEntryPointBundle} from '../helpers/utils';
import {getTraitDiagnostics} from '../host/util';

runInEachFileSystem(() => {
describe('NgccTraitCompiler', () => {
Expand Down Expand Up @@ -234,12 +235,13 @@ runInEachFileSystem(() => {

const record = compiler.recordFor(mockClazz)!;
const migratedTrait = record.traits[0];
if (migratedTrait.state !== TraitState.ERRORED) {
const diagnostics = getTraitDiagnostics(migratedTrait);
if (diagnostics === null) {
return fail('Expected migrated class trait to be in an error state');
}

expect(migratedTrait.diagnostics.length).toBe(1);
expect(migratedTrait.diagnostics[0].messageText).toEqual(`test diagnostic`);
expect(diagnostics.length).toBe(1);
expect(diagnostics[0].messageText).toEqual(`test diagnostic`);
});
});

Expand Down
15 changes: 15 additions & 0 deletions packages/compiler-cli/ngcc/test/host/util.ts
Expand Up @@ -5,6 +5,7 @@
* 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 {Trait, TraitState} from '@angular/compiler-cli/src/ngtsc/transform';
import * as ts from 'typescript';
import {CtorParameter, TypeValueReferenceKind} from '../../../src/ngtsc/reflection';

Expand Down Expand Up @@ -48,3 +49,17 @@ export function expectTypeValueReferencesForParameters(
}
});
}

export function getTraitDiagnostics(trait: Trait<unknown, unknown, unknown>): ts.Diagnostic[]|null {
if (trait.state === TraitState.Analyzed) {
return trait.analysisDiagnostics;
} else if (trait.state === TraitState.Resolved) {
const diags = [
...(trait.analysisDiagnostics ?? []),
...(trait.resolveDiagnostics ?? []),
];
return diags.length > 0 ? diags : null;
} else {
return null;
}
}
34 changes: 25 additions & 9 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
Expand Up @@ -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>;
Expand All @@ -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,
Expand Down Expand Up @@ -359,6 +361,7 @@ export class ComponentDecoratorHandler implements
template,
providersRequiringFactory,
viewProvidersRequiringFactory,
isPoisoned: diagnostics !== undefined && diagnostics.length > 0,
},
diagnostics,
};
Expand All @@ -383,6 +386,7 @@ export class ComponentDecoratorHandler implements
isComponent: true,
baseClass: analysis.baseClass,
...analysis.typeCheckMeta,
isPoisoned: analysis.isPoisoned,
});

if (!analysis.template.isInline) {
Expand All @@ -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);
Expand All @@ -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;
}

Expand All @@ -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.
Expand All @@ -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.
Expand Down
5 changes: 4 additions & 1 deletion packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
Expand Up @@ -41,6 +41,7 @@ export interface DirectiveHandlerData {
providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
inputs: ClassPropertyMapping;
outputs: ClassPropertyMapping;
isPoisoned: boolean;
}

export class DirectiveDecoratorHandler implements
Expand Down Expand Up @@ -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,
}
};
}
Expand All @@ -126,6 +128,7 @@ export class DirectiveDecoratorHandler implements
isComponent: false,
baseClass: analysis.baseClass,
...analysis.typeCheckMeta,
isPoisoned: analysis.isPoisoned,
});

this.injectableRegistry.registerInjectable(node);
Expand Down
5 changes: 3 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
Expand Up @@ -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);
Expand All @@ -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 {
Expand Down
Expand Up @@ -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;
}

/**
Expand All @@ -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)) {
Expand All @@ -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;
}
Expand Down
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion packages/compiler-cli/src/ngtsc/core/src/compiler.ts
Expand Up @@ -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,
) {
Expand Down Expand Up @@ -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),
Expand Down
9 changes: 6 additions & 3 deletions packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts
Expand Up @@ -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);
Expand Down Expand Up @@ -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]));
});
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 8 additions & 0 deletions packages/compiler-cli/src/ngtsc/incremental/api.ts
Expand Up @@ -64,4 +64,12 @@ export interface DependencyTracker<T extends {fileName: string} = ts.SourceFile>
* `resourcesOf` they will not automatically be added to `from`.
*/
addTransitiveResources(from: T, resourcesOf: T): void;

/**
* Record that the given file contains unresolvable dependencies.
*
* In practice, this means that the dependency graph cannot provide insight into the effects of
* future changes on that file.
*/
recordDependencyAnalysisFailure(file: T): void;
}