Skip to content

Commit

Permalink
fix(compiler-cli): remove the concept of an errored trait (#39967)
Browse files Browse the repository at this point in the history 
Previously, if a trait's analysis step resulted in diagnostics, the trait
would be considered "errored" and no further operations, including register,
would be performed. Effectively, this meant that the compiler would pretend
the class in question was actually undecorated.

However, this behavior is problematic for several reasons:

1. It leads to inaccurate diagnostics being reported downstream.

For example, if a component is put into the error state, for example due to
a template error, the NgModule which declares the component would produce a
diagnostic claiming that the declaration is neither a directive nor a pipe.
This happened because the compiler wouldn't register() the component trait,
so the component would not be recorded as actually being a directive.

2. It can cause incorrect behavior on incremental builds.

This bug is more complex, but the general issue is that if the compiler
fails to associate a component and its module, then incremental builds will
not correctly re-analyze the module when the component's template changes.
Failing to register the component as such is one link in the larger chain of
issues that result in these kinds of issues.

3. It lumps together diagnostics produced during analysis and resolve steps.

This is not causing issues currently as the dependency graph ensures the
right classes are re-analyzed when needed, instead of showing stale
diagnostics. However, the dependency graph was not intended to serve this
role, and could potentially be optimized in ways that would break this
functionality.

This commit removes the concept of an "errored" trait entirely from the
trait system. Instead, analyzed and resolved traits have corresponding (and
separate) diagnostics, in addition to potentially `null` analysis results.
Analysis (but not resolution) diagnostics are carried forward during
incremental build operations. Compilation (emit) is only performed when
a trait reaches the resolved state with no diagnostics.

This change is functionally different than before as the `register` step is
now performed even in the presence of analysis errors, as long as analysis
results are also produced. This fixes problem 1 above, and is part of the
larger solution to problem 2.

PR Close #39967
  • Loading branch information
@alxhub @mhevery
alxhub authored and mhevery committed Dec 5, 2020
1 parent 1e3534f commit 0aa35ec
Show file tree
Hide file tree
Showing 9 changed files with 164 additions and 140 deletions.
11 changes: 8 additions & 3 deletions packages/compiler-cli/ngcc/src/analysis/migration_host.ts
View file
Original file line number Diff line number Diff line change
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
4 changes: 2 additions & 2 deletions packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts
View file
Original file line number Diff line number Diff line change
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
8 changes: 5 additions & 3 deletions packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts
View file
Original file line number Diff line number Diff line change
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
8 changes: 5 additions & 3 deletions packages/compiler-cli/ngcc/test/analysis/ngcc_trait_compiler_spec.ts
View file
Original file line number Diff line number Diff line change
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
View file
Original file line number Diff line number Diff line change
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;
}
}
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/transform/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ export * from './src/api';
export {aliasTransformFactory} from './src/alias';
export {ClassRecord, TraitCompiler} from './src/compilation';
export {declarationTransformFactory, DtsTransformRegistry, IvyDeclarationDtsTransform, ReturnTypeTransform} from './src/declaration';
export {AnalyzedTrait, ErroredTrait, PendingTrait, ResolvedTrait, SkippedTrait, Trait, TraitState} from './src/trait';
export {AnalyzedTrait, PendingTrait, ResolvedTrait, SkippedTrait, Trait, TraitState} from './src/trait';
export {ivyTransformFactory} from './src/transform';
93 changes: 47 additions & 46 deletions packages/compiler-cli/src/ngtsc/transform/src/compilation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
const handler = this.handlersByName.get(priorTrait.handler.name)!;
let trait: Trait<unknown, unknown, unknown> = Trait.pending(handler, priorTrait.detected);

if (priorTrait.state === TraitState.ANALYZED || priorTrait.state === TraitState.RESOLVED) {
trait = trait.toAnalyzed(priorTrait.analysis);
if (trait.handler.register !== undefined) {
if (priorTrait.state === TraitState.Analyzed || priorTrait.state === TraitState.Resolved) {
trait = trait.toAnalyzed(priorTrait.analysis, priorTrait.analysisDiagnostics);
if (trait.analysis !== null && trait.handler.register !== undefined) {
trait.handler.register(record.node, trait.analysis);
}
} else if (priorTrait.state === TraitState.SKIPPED) {
} else if (priorTrait.state === TraitState.Skipped) {
trait = trait.toSkipped();
} else if (priorTrait.state === TraitState.ERRORED) {
trait = trait.toErrored(priorTrait.diagnostics);
}

record.traits.push(trait);
Expand Down Expand Up @@ -314,7 +312,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
preanalysis = trait.handler.preanalyze(clazz, trait.detected.metadata) || null;
} catch (err) {
if (err instanceof FatalDiagnosticError) {
trait.toErrored([err.toDiagnostic()]);
trait.toAnalyzed(null, [err.toDiagnostic()]);
return;
} else {
throw err;
Expand All @@ -332,7 +330,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
protected analyzeTrait(
clazz: ClassDeclaration, trait: Trait<unknown, unknown, unknown>,
flags?: HandlerFlags): void {
if (trait.state !== TraitState.PENDING) {
if (trait.state !== TraitState.Pending) {
throw new Error(`Attempt to analyze trait of ${clazz.name.text} in state ${
TraitState[trait.state]} (expected DETECTED)`);
}
Expand All @@ -343,26 +341,18 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
result = trait.handler.analyze(clazz, trait.detected.metadata, flags);
} catch (err) {
if (err instanceof FatalDiagnosticError) {
trait = trait.toErrored([err.toDiagnostic()]);
trait.toAnalyzed(null, [err.toDiagnostic()]);
return;
} else {
throw err;
}
}

if (result.diagnostics !== undefined) {
trait = trait.toErrored(result.diagnostics);
} else if (result.analysis !== undefined) {
// Analysis was successful. Trigger registration.
if (trait.handler.register !== undefined) {
trait.handler.register(clazz, result.analysis);
}

// Successfully analyzed and registered.
trait = trait.toAnalyzed(result.analysis);
} else {
trait = trait.toSkipped();
if (result.analysis !== undefined && trait.handler.register !== undefined) {
trait.handler.register(clazz, result.analysis);
}

trait = trait.toAnalyzed(result.analysis ?? null, result.diagnostics ?? null);
}

resolve(): void {
Expand All @@ -372,19 +362,23 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
for (let trait of record.traits) {
const handler = trait.handler;
switch (trait.state) {
case TraitState.SKIPPED:
case TraitState.ERRORED:
case TraitState.Skipped:
continue;
case TraitState.PENDING:
case TraitState.Pending:
throw new Error(`Resolving a trait that hasn't been analyzed: ${clazz.name.text} / ${
Object.getPrototypeOf(trait.handler).constructor.name}`);
case TraitState.RESOLVED:
case TraitState.Resolved:
throw new Error(`Resolving an already resolved trait`);
}

if (trait.analysis === null) {
// No analysis results, cannot further process this trait.
continue;
}

if (handler.resolve === undefined) {
// No resolution of this trait needed - it's considered successful by default.
trait = trait.toResolved(null);
trait = trait.toResolved(null, null);
continue;
}

Expand All @@ -393,22 +387,14 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
result = handler.resolve(clazz, trait.analysis as Readonly<unknown>);
} catch (err) {
if (err instanceof FatalDiagnosticError) {
trait = trait.toErrored([err.toDiagnostic()]);
trait = trait.toResolved(null, [err.toDiagnostic()]);
continue;
} else {
throw err;
}
}

if (result.diagnostics !== undefined && result.diagnostics.length > 0) {
trait = trait.toErrored(result.diagnostics);
} else {
if (result.data !== undefined) {
trait = trait.toResolved(result.data);
} else {
trait = trait.toResolved(null);
}
}
trait = trait.toResolved(result.data ?? null, result.diagnostics ?? null);

if (result.reexports !== undefined) {
const fileName = clazz.getSourceFile().fileName;
Expand Down Expand Up @@ -436,12 +422,14 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
for (const clazz of this.fileToClasses.get(sf)!) {
const record = this.classes.get(clazz)!;
for (const trait of record.traits) {
if (trait.state !== TraitState.RESOLVED) {
if (trait.state !== TraitState.Resolved) {
continue;
} else if (trait.handler.typeCheck === undefined) {
continue;
}
trait.handler.typeCheck(ctx, clazz, trait.analysis, trait.resolution);
if (trait.resolution !== null) {
trait.handler.typeCheck(ctx, clazz, trait.analysis, trait.resolution);
}
}
}
}
Expand All @@ -450,15 +438,17 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
for (const clazz of this.classes.keys()) {
const record = this.classes.get(clazz)!;
for (const trait of record.traits) {
if (trait.state !== TraitState.RESOLVED) {
if (trait.state !== TraitState.Resolved) {
// Skip traits that haven't been resolved successfully.
continue;
} else if (trait.handler.index === undefined) {
// Skip traits that don't affect indexing.
continue;
}

trait.handler.index(ctx, clazz, trait.analysis, trait.resolution);
if (trait.resolution !== null) {
trait.handler.index(ctx, clazz, trait.analysis, trait.resolution);
}
}
}
}
Expand All @@ -475,19 +465,26 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
let res: CompileResult[] = [];

for (const trait of record.traits) {
if (trait.state !== TraitState.RESOLVED) {
if (trait.state !== TraitState.Resolved || trait.analysisDiagnostics !== null ||
trait.resolveDiagnostics !== null) {
// Cannot compile a trait that is not resolved, or had any errors in its declaration.
continue;
}

const compileSpan = this.perf.start('compileClass', original);


// `trait.resolution` is non-null asserted here because TypeScript does not recognize that
// `Readonly<unknown>` is nullable (as `unknown` itself is nullable) due to the way that
// `Readonly` works.

let compileRes: CompileResult|CompileResult[];
if (this.compilationMode === CompilationMode.PARTIAL &&
trait.handler.compilePartial !== undefined) {
compileRes = trait.handler.compilePartial(clazz, trait.analysis, trait.resolution);
compileRes = trait.handler.compilePartial(clazz, trait.analysis, trait.resolution!);
} else {
compileRes =
trait.handler.compileFull(clazz, trait.analysis, trait.resolution, constantPool);
trait.handler.compileFull(clazz, trait.analysis, trait.resolution!, constantPool);
}

const compileMatchRes = compileRes;
Expand Down Expand Up @@ -522,7 +519,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
const decorators: ts.Decorator[] = [];

for (const trait of record.traits) {
if (trait.state !== TraitState.RESOLVED) {
if (trait.state !== TraitState.Resolved) {
continue;
}

Expand All @@ -542,8 +539,12 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
diagnostics.push(...record.metaDiagnostics);
}
for (const trait of record.traits) {
if (trait.state === TraitState.ERRORED) {
diagnostics.push(...trait.diagnostics);
if ((trait.state === TraitState.Analyzed || trait.state === TraitState.Resolved) &&
trait.analysisDiagnostics !== null) {
diagnostics.push(...trait.analysisDiagnostics);
}
if (trait.state === TraitState.Resolved && trait.resolveDiagnostics !== null) {
diagnostics.push(...trait.resolveDiagnostics);
}
}
}
Expand Down
Loading

0 comments on commit 0aa35ec

Please sign in to comment.