Skip to content

Commit

Permalink
fix(ngcc): do not attempt compilation when analysis fails (#34889)
Browse files Browse the repository at this point in the history 
In #34288, ngtsc was refactored to separate the result of the analysis
and resolve phase for more granular incremental rebuilds. In this model,
any errors in one phase transition the trait into an error state, which
prevents it from being ran through subsequent phases. The ngcc compiler
on the other hand did not adopt this strict error model, which would
cause incomplete metadata—due to errors in earlier phases—to be offered
for compilation that could result in a hard crash.

This commit updates ngcc to take advantage of ngtsc's `TraitCompiler`,
that internally manages all Ivy classes that are part of the
compilation. This effectively replaces ngcc's own `AnalyzedFile` and
`AnalyzedClass` types, together with all of the logic to drive the
`DecoratorHandler`s. All of this is now handled in the `TraitCompiler`,
benefiting from its explicit state transitions of `Trait`s so that the
ngcc crash is a thing of the past.

Fixes #34500
Resolves FW-1788

PR Close #34889
  • Loading branch information
@JoostK @AndrewKushnir
JoostK authored and AndrewKushnir committed Jan 23, 2020
1 parent 8f5c920 commit 080a8bf
Show file tree
Hide file tree
Showing 20 changed files with 778 additions and 598 deletions.
166 changes: 69 additions & 97 deletions packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,19 @@ import {FileSystem, LogicalFileSystem, absoluteFrom, dirname, resolve} from '../
import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports';
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../../src/ngtsc/metadata';
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
import {ClassDeclaration} from '../../../src/ngtsc/reflection';
import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../../src/ngtsc/scope';
import {CompileResult, DecoratorHandler} from '../../../src/ngtsc/transform';
import {NgccClassSymbol, NgccReflectionHost} from '../host/ngcc_host';
import {DecoratorHandler} from '../../../src/ngtsc/transform';
import {NgccReflectionHost} from '../host/ngcc_host';
import {Migration} from '../migrations/migration';
import {MissingInjectableMigration} from '../migrations/missing_injectable_migration';
import {UndecoratedChildMigration} from '../migrations/undecorated_child_migration';
import {UndecoratedParentMigration} from '../migrations/undecorated_parent_migration';
import {EntryPointBundle} from '../packages/entry_point_bundle';
import {isDefined} from '../utils';

import {DefaultMigrationHost} from './migration_host';
import {AnalyzedClass, AnalyzedFile, CompiledClass, CompiledFile, DecorationAnalyses} from './types';
import {NOOP_DEPENDENCY_TRACKER, analyzeDecorators, isWithinPackage} from './util';
import {NgccTraitCompiler} from './ngcc_trait_compiler';
import {CompiledClass, CompiledFile, DecorationAnalyses} from './types';
import {NOOP_DEPENDENCY_TRACKER, isWithinPackage} from './util';



Expand Down Expand Up @@ -57,10 +56,6 @@ export class DecorationAnalyzer {
private packagePath = this.bundle.entryPoint.package;
private isCore = this.bundle.isCore;

/**
* Map of NgModule declarations to the re-exports for that NgModule.
*/
private reexportMap = new Map<ts.Declaration, Map<string, [string, string]>>();
moduleResolver =
new ModuleResolver(this.program, this.options, this.host, /* moduleResolutionCache */ null);
resourceManager = new NgccResourceLoader(this.fs);
Expand Down Expand Up @@ -118,6 +113,7 @@ export class DecorationAnalyzer {
/* factoryTracker */ null, NOOP_DEFAULT_IMPORT_RECORDER,
/* annotateForClosureCompiler */ false, this.injectableRegistry),
];
compiler = new NgccTraitCompiler(this.handlers, this.reflectionHost);
migrations: Migration[] = [
new UndecoratedParentMigration(),
new UndecoratedChildMigration(),
Expand All @@ -135,56 +131,54 @@ export class DecorationAnalyzer {
* @returns a map of the source files to the analysis for those files.
*/
analyzeProgram(): DecorationAnalyses {
const decorationAnalyses = new DecorationAnalyses();
const analyzedFiles = this.program.getSourceFiles()
.filter(sourceFile => !sourceFile.isDeclarationFile)
.filter(sourceFile => isWithinPackage(this.packagePath, sourceFile))
.map(sourceFile => this.analyzeFile(sourceFile))
.filter(isDefined);
for (const sourceFile of this.program.getSourceFiles()) {
if (!sourceFile.isDeclarationFile && isWithinPackage(this.packagePath, sourceFile)) {
this.compiler.analyzeFile(sourceFile);
}
}

this.applyMigrations(analyzedFiles);
this.applyMigrations();

analyzedFiles.forEach(analyzedFile => this.resolveFile(analyzedFile));
const compiledFiles = analyzedFiles.map(analyzedFile => this.compileFile(analyzedFile));
compiledFiles.forEach(
compiledFile => decorationAnalyses.set(compiledFile.sourceFile, compiledFile));
return decorationAnalyses;
}
this.compiler.resolve();

protected analyzeFile(sourceFile: ts.SourceFile): AnalyzedFile|undefined {
const analyzedClasses = this.reflectionHost.findClassSymbols(sourceFile)
.map(symbol => this.analyzeClass(symbol))
.filter(isDefined);
return analyzedClasses.length ? {sourceFile, analyzedClasses} : undefined;
}
this.reportDiagnostics();

protected analyzeClass(symbol: NgccClassSymbol): AnalyzedClass|null {
const decorators = this.reflectionHost.getDecoratorsOfSymbol(symbol);
const analyzedClass = analyzeDecorators(symbol, decorators, this.handlers);
if (analyzedClass !== null && analyzedClass.diagnostics !== undefined) {
for (const diagnostic of analyzedClass.diagnostics) {
this.diagnosticHandler(diagnostic);
}
const decorationAnalyses = new DecorationAnalyses();
for (const analyzedFile of this.compiler.analyzedFiles) {
const compiledFile = this.compileFile(analyzedFile);
decorationAnalyses.set(compiledFile.sourceFile, compiledFile);
}
return analyzedClass;
return decorationAnalyses;
}

protected applyMigrations(analyzedFiles: AnalyzedFile[]): void {
protected applyMigrations(): void {
const migrationHost = new DefaultMigrationHost(
this.reflectionHost, this.fullMetaReader, this.evaluator, this.handlers,
this.bundle.entryPoint.path, analyzedFiles, this.diagnosticHandler);
this.reflectionHost, this.fullMetaReader, this.evaluator, this.compiler,
this.bundle.entryPoint.path);

this.migrations.forEach(migration => {
analyzedFiles.forEach(analyzedFile => {
analyzedFile.analyzedClasses.forEach(({declaration}) => {
this.compiler.analyzedFiles.forEach(analyzedFile => {
const records = this.compiler.recordsFor(analyzedFile);
if (records === null) {
throw new Error('Assertion error: file to migrate must have records.');
}

records.forEach(record => {
const addDiagnostic = (diagnostic: ts.Diagnostic) => {
if (record.metaDiagnostics === null) {
record.metaDiagnostics = [];
}
record.metaDiagnostics.push(diagnostic);
};

try {
const result = migration.apply(declaration, migrationHost);
const result = migration.apply(record.node, migrationHost);
if (result !== null) {
this.diagnosticHandler(result);
addDiagnostic(result);
}
} catch (e) {
if (isFatalDiagnosticError(e)) {
this.diagnosticHandler(e.toDiagnostic());
addDiagnostic(e.toDiagnostic());
} else {
throw e;
}
Expand All @@ -194,67 +188,45 @@ export class DecorationAnalyzer {
});
}

protected compileFile(analyzedFile: AnalyzedFile): CompiledFile {
const constantPool = new ConstantPool();
const compiledClasses: CompiledClass[] = analyzedFile.analyzedClasses.map(analyzedClass => {
const compilation = this.compileClass(analyzedClass, constantPool);
const declaration = analyzedClass.declaration;
const reexports: Reexport[] = this.getReexportsForClass(declaration);
return {...analyzedClass, compilation, reexports};
});
return {constantPool, sourceFile: analyzedFile.sourceFile, compiledClasses};
}
protected reportDiagnostics() { this.compiler.diagnostics.forEach(this.diagnosticHandler); }

protected compileClass(clazz: AnalyzedClass, constantPool: ConstantPool): CompileResult[] {
const compilations: CompileResult[] = [];
for (const {handler, analysis, resolution} of clazz.matches) {
const result = handler.compile(clazz.declaration, analysis, resolution, constantPool);
if (Array.isArray(result)) {
result.forEach(current => {
if (!compilations.some(compilation => compilation.name === current.name)) {
compilations.push(current);
}
});
} else if (!compilations.some(compilation => compilation.name === result.name)) {
compilations.push(result);
}
protected compileFile(sourceFile: ts.SourceFile): CompiledFile {
const constantPool = new ConstantPool();
const records = this.compiler.recordsFor(sourceFile);
if (records === null) {
throw new Error('Assertion error: file to compile must have records.');
}
return compilations;
}

protected resolveFile(analyzedFile: AnalyzedFile): void {
for (const {declaration, matches} of analyzedFile.analyzedClasses) {
for (const match of matches) {
const {handler, analysis} = match;
if ((handler.resolve !== undefined) && analysis) {
const {reexports, diagnostics, data} = handler.resolve(declaration, analysis);
if (reexports !== undefined) {
this.addReexports(reexports, declaration);
}
if (diagnostics !== undefined) {
diagnostics.forEach(error => this.diagnosticHandler(error));
}
match.resolution = data as Readonly<unknown>;
}
const compiledClasses: CompiledClass[] = [];

for (const record of records) {
const compilation = this.compiler.compile(record.node, constantPool);
if (compilation === null) {
continue;
}
}
}

private getReexportsForClass(declaration: ClassDeclaration<ts.Declaration>) {
const reexports: Reexport[] = [];
if (this.reexportMap.has(declaration)) {
this.reexportMap.get(declaration) !.forEach(([fromModule, symbolName], asAlias) => {
reexports.push({asAlias, fromModule, symbolName});
compiledClasses.push({
name: record.node.name.text,
decorators: this.compiler.getAllDecorators(record.node),
declaration: record.node, compilation
});
}
return reexports;

const reexports = this.getReexportsForSourceFile(sourceFile);
return {constantPool, sourceFile: sourceFile, compiledClasses, reexports};
}

private addReexports(reexports: Reexport[], declaration: ClassDeclaration<ts.Declaration>) {
const map = new Map<string, [string, string]>();
for (const reexport of reexports) {
map.set(reexport.asAlias, [reexport.fromModule, reexport.symbolName]);
private getReexportsForSourceFile(sf: ts.SourceFile): Reexport[] {
const exportStatements = this.compiler.exportStatements;
if (!exportStatements.has(sf.fileName)) {
return [];
}
this.reexportMap.set(declaration, map);
const exports = exportStatements.get(sf.fileName) !;

const reexports: Reexport[] = [];
exports.forEach(([fromModule, symbolName], asAlias) => {
reexports.push({asAlias, fromModule, symbolName});
});
return reexports;
}
}
87 changes: 12 additions & 75 deletions packages/compiler-cli/ngcc/src/analysis/migration_host.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,110 +7,47 @@
*/
import * as ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
import {MetadataReader} from '../../../src/ngtsc/metadata';
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection';
import {DecoratorHandler, HandlerFlags} from '../../../src/ngtsc/transform';
import {HandlerFlags, TraitState} from '../../../src/ngtsc/transform';
import {NgccReflectionHost} from '../host/ngcc_host';
import {MigrationHost} from '../migrations/migration';

import {AnalyzedClass, AnalyzedFile} from './types';
import {analyzeDecorators, isWithinPackage} from './util';
import {NgccTraitCompiler} from './ngcc_trait_compiler';
import {isWithinPackage} from './util';

/**
* The standard implementation of `MigrationHost`, which is created by the
* `DecorationAnalyzer`.
* The standard implementation of `MigrationHost`, which is created by the `DecorationAnalyzer`.
*/
export class DefaultMigrationHost implements MigrationHost {
constructor(
readonly reflectionHost: NgccReflectionHost, readonly metadata: MetadataReader,
readonly evaluator: PartialEvaluator,
private handlers: DecoratorHandler<unknown, unknown, unknown>[],
private entryPointPath: AbsoluteFsPath, private analyzedFiles: AnalyzedFile[],
private diagnosticHandler: (error: ts.Diagnostic) => void) {}
readonly evaluator: PartialEvaluator, private compiler: NgccTraitCompiler,
private entryPointPath: AbsoluteFsPath) {}

injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator, flags?: HandlerFlags):
void {
const classSymbol = this.reflectionHost.getClassSymbol(clazz) !;
const newAnalyzedClass = analyzeDecorators(classSymbol, [decorator], this.handlers, flags);
if (newAnalyzedClass === null) {
return;
}
const migratedTraits = this.compiler.injectSyntheticDecorator(clazz, decorator, flags);

if (newAnalyzedClass.diagnostics !== undefined) {
for (const diagnostic of newAnalyzedClass.diagnostics) {
this.diagnosticHandler(createMigrationDiagnostic(diagnostic, clazz, decorator));
for (const trait of migratedTraits) {
if (trait.state === TraitState.ERRORED) {
trait.diagnostics =
trait.diagnostics.map(diag => createMigrationDiagnostic(diag, clazz, decorator));
}
}

const analyzedFile = getOrCreateAnalyzedFile(this.analyzedFiles, clazz.getSourceFile());
const oldAnalyzedClass = analyzedFile.analyzedClasses.find(c => c.declaration === clazz);
if (oldAnalyzedClass === undefined) {
analyzedFile.analyzedClasses.push(newAnalyzedClass);
} else {
mergeAnalyzedClasses(oldAnalyzedClass, newAnalyzedClass);
}
}

getAllDecorators(clazz: ClassDeclaration): Decorator[]|null {
const sourceFile = clazz.getSourceFile();
const analyzedFile = this.analyzedFiles.find(file => file.sourceFile === sourceFile);
if (analyzedFile === undefined) {
return null;
}

const analyzedClass = analyzedFile.analyzedClasses.find(c => c.declaration === clazz);
if (analyzedClass === undefined) {
return null;
}

return analyzedClass.decorators;
return this.compiler.getAllDecorators(clazz);
}

isInScope(clazz: ClassDeclaration): boolean {
return isWithinPackage(this.entryPointPath, clazz.getSourceFile());
}
}

function getOrCreateAnalyzedFile(
analyzedFiles: AnalyzedFile[], sourceFile: ts.SourceFile): AnalyzedFile {
const analyzedFile = analyzedFiles.find(file => file.sourceFile === sourceFile);
if (analyzedFile !== undefined) {
return analyzedFile;
} else {
const newAnalyzedFile: AnalyzedFile = {sourceFile, analyzedClasses: []};
analyzedFiles.push(newAnalyzedFile);
return newAnalyzedFile;
}
}

function mergeAnalyzedClasses(oldClass: AnalyzedClass, newClass: AnalyzedClass) {
if (newClass.decorators !== null) {
if (oldClass.decorators === null) {
oldClass.decorators = newClass.decorators;
} else {
for (const newDecorator of newClass.decorators) {
if (oldClass.decorators.some(d => d.name === newDecorator.name)) {
throw new FatalDiagnosticError(
ErrorCode.NGCC_MIGRATION_DECORATOR_INJECTION_ERROR, newClass.declaration,
`Attempted to inject "${newDecorator.name}" decorator over a pre-existing decorator with the same name on the "${newClass.name}" class.`);
}
}
oldClass.decorators.push(...newClass.decorators);
}
}

if (newClass.diagnostics !== undefined) {
if (oldClass.diagnostics === undefined) {
oldClass.diagnostics = newClass.diagnostics;
} else {
oldClass.diagnostics.push(...newClass.diagnostics);
}
}
}

/**
* Creates a diagnostic from another one, containing additional information about the synthetic
* decorator.
Expand Down
Loading

0 comments on commit 080a8bf

Please sign in to comment.