From 0aa35ecee144a8ba5243f5d67ea176af19869dba Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 25 Nov 2020 15:01:24 -0800 Subject: [PATCH] fix(compiler-cli): remove the concept of an errored trait (#39967) 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 --- .../ngcc/src/analysis/migration_host.ts | 11 +- .../test/analysis/decoration_analyzer_spec.ts | 4 +- .../ngcc/test/analysis/migration_host_spec.ts | 8 +- .../test/analysis/ngcc_trait_compiler_spec.ts | 8 +- packages/compiler-cli/ngcc/test/host/util.ts | 15 ++ .../compiler-cli/src/ngtsc/transform/index.ts | 2 +- .../src/ngtsc/transform/src/compilation.ts | 93 ++++++------ .../src/ngtsc/transform/src/trait.ts | 139 +++++++----------- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 24 +++ 9 files changed, 164 insertions(+), 140 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/analysis/migration_host.ts b/packages/compiler-cli/ngcc/src/analysis/migration_host.ts index e1e486e54a853..60b6f89bd68c9 100644 --- a/packages/compiler-cli/ngcc/src/analysis/migration_host.ts +++ b/packages/compiler-cli/ngcc/src/analysis/migration_host.ts @@ -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)); } } } diff --git a/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts b/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts index 632490941456e..d3274552a9d0b 100644 --- a/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts @@ -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 }; @@ -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})); diff --git a/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts b/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts index b4053aca9787b..77e4a00db6684 100644 --- a/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts @@ -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', () => { @@ -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` + diff --git a/packages/compiler-cli/ngcc/test/analysis/ngcc_trait_compiler_spec.ts b/packages/compiler-cli/ngcc/test/analysis/ngcc_trait_compiler_spec.ts index ab09980e3a2d9..14e17e1b97b13 100644 --- a/packages/compiler-cli/ngcc/test/analysis/ngcc_trait_compiler_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/ngcc_trait_compiler_spec.ts @@ -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', () => { @@ -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`); }); }); diff --git a/packages/compiler-cli/ngcc/test/host/util.ts b/packages/compiler-cli/ngcc/test/host/util.ts index 5767be4bf789f..dbec4d00d64c3 100644 --- a/packages/compiler-cli/ngcc/test/host/util.ts +++ b/packages/compiler-cli/ngcc/test/host/util.ts @@ -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'; @@ -48,3 +49,17 @@ export function expectTypeValueReferencesForParameters( } }); } + +export function getTraitDiagnostics(trait: Trait): 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; + } +} diff --git a/packages/compiler-cli/src/ngtsc/transform/index.ts b/packages/compiler-cli/src/ngtsc/transform/index.ts index 6d9d3a61a0c34..a3fe99dfe68c4 100644 --- a/packages/compiler-cli/src/ngtsc/transform/index.ts +++ b/packages/compiler-cli/src/ngtsc/transform/index.ts @@ -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'; diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index 01415026c3eb5..a3347323ef3a7 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -181,15 +181,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { const handler = this.handlersByName.get(priorTrait.handler.name)!; let trait: Trait = 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); @@ -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; @@ -332,7 +330,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { protected analyzeTrait( clazz: ClassDeclaration, trait: Trait, 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)`); } @@ -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 { @@ -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; } @@ -393,22 +387,14 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { result = handler.resolve(clazz, trait.analysis as Readonly); } 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; @@ -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); + } } } } @@ -450,7 +438,7 @@ 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) { @@ -458,7 +446,9 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { continue; } - trait.handler.index(ctx, clazz, trait.analysis, trait.resolution); + if (trait.resolution !== null) { + trait.handler.index(ctx, clazz, trait.analysis, trait.resolution); + } } } } @@ -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` 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; @@ -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; } @@ -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); } } } diff --git a/packages/compiler-cli/src/ngtsc/transform/src/trait.ts b/packages/compiler-cli/src/ngtsc/transform/src/trait.ts index 2d60ffc3701df..53cc026b15eab 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/trait.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/trait.ts @@ -13,28 +13,22 @@ export enum TraitState { /** * Pending traits are freshly created and have never been analyzed. */ - PENDING = 0x01, + Pending, /** * Analyzed traits have successfully been analyzed, but are pending resolution. */ - ANALYZED = 0x02, + Analyzed, /** * Resolved traits have successfully been analyzed and resolved and are ready for compilation. */ - RESOLVED = 0x04, - - /** - * Errored traits have failed either analysis or resolution and as a result contain diagnostics - * describing the failure(s). - */ - ERRORED = 0x08, + Resolved, /** * Skipped traits are no longer considered for compilation. */ - SKIPPED = 0x10, + Skipped, } /** @@ -50,8 +44,8 @@ export enum TraitState { * This not only simplifies the implementation, but ensures traits are monomorphic objects as * they're all just "views" in the type system of the same object (which never changes shape). */ -export type Trait = PendingTrait|SkippedTrait|AnalyzedTrait| - ResolvedTrait|ErroredTrait; +export type Trait = + PendingTrait|SkippedTrait|AnalyzedTrait|ResolvedTrait; /** * The value side of `Trait` exposes a helper to create a `Trait` in a pending state (by delegating @@ -93,19 +87,13 @@ export interface TraitBase { * Pending traits have yet to be analyzed in any way. */ export interface PendingTrait extends TraitBase { - state: TraitState.PENDING; + state: TraitState.Pending; /** * This pending trait has been successfully analyzed, and should transition to the "analyzed" * state. */ - toAnalyzed(analysis: A): AnalyzedTrait; - - /** - * This trait failed analysis, and should transition to the "errored" state with the resulting - * diagnostics. - */ - toErrored(errors: ts.Diagnostic[]): ErroredTrait; + toAnalyzed(analysis: A|null, diagnostics: ts.Diagnostic[]|null): AnalyzedTrait; /** * During analysis it was determined that this trait is not eligible for compilation after all, @@ -114,22 +102,6 @@ export interface PendingTrait extends TraitBase { toSkipped(): SkippedTrait; } -/** - * A trait in the "errored" state. - * - * Errored traits contain `ts.Diagnostic`s indicating any problem(s) with the class. - * - * This is a terminal state. - */ -export interface ErroredTrait extends TraitBase { - state: TraitState.ERRORED; - - /** - * Diagnostics which were produced while attempting to analyze the trait. - */ - diagnostics: ts.Diagnostic[]; -} - /** * A trait in the "skipped" state. * @@ -138,20 +110,7 @@ export interface ErroredTrait extends TraitBase { * This is a terminal state. */ export interface SkippedTrait extends TraitBase { - state: TraitState.SKIPPED; -} - -/** - * The part of the `Trait` interface for any trait which has been successfully analyzed. - * - * Mainly, this is used to share the comment on the `analysis` field. - */ -export interface TraitWithAnalysis { - /** - * The results returned by a successful analysis of the given class/`DecoratorHandler` - * combination. - */ - analysis: Readonly; + state: TraitState.Skipped; } /** @@ -159,20 +118,25 @@ export interface TraitWithAnalysis { * * Analyzed traits have analysis results available, and are eligible for resolution. */ -export interface AnalyzedTrait extends TraitBase, TraitWithAnalysis { - state: TraitState.ANALYZED; +export interface AnalyzedTrait extends TraitBase { + state: TraitState.Analyzed; /** - * This analyzed trait has been successfully resolved, and should be transitioned to the - * "resolved" state. + * Analysis results of the given trait (if able to be produced), or `null` if analysis failed + * completely. */ - toResolved(resolution: R): ResolvedTrait; + analysis: Readonly|null; /** - * This trait failed resolution, and should transition to the "errored" state with the resulting - * diagnostics. + * Any diagnostics that resulted from analysis, or `null` if none. */ - toErrored(errors: ts.Diagnostic[]): ErroredTrait; + analysisDiagnostics: ts.Diagnostic[]|null; + + /** + * This analyzed trait has been successfully resolved, and should be transitioned to the + * "resolved" state. + */ + toResolved(resolution: R|null, diagnostics: ts.Diagnostic[]|null): ResolvedTrait; } /** @@ -183,14 +147,29 @@ export interface AnalyzedTrait extends TraitBase, TraitWithAna * * This is a terminal state. */ -export interface ResolvedTrait extends TraitBase, TraitWithAnalysis { - state: TraitState.RESOLVED; +export interface ResolvedTrait extends TraitBase { + state: TraitState.Resolved; + + /** + * Resolved traits must have produced valid analysis results. + */ + analysis: Readonly; + + /** + * Analysis may have still resulted in diagnostics. + */ + analysisDiagnostics: ts.Diagnostic[]|null; + + /** + * Diagnostics resulting from resolution are tracked separately from + */ + resolveDiagnostics: ts.Diagnostic[]|null; /** * The results returned by a successful resolution of the given class/`DecoratorHandler` * combination. */ - resolution: Readonly; + resolution: Readonly|null; } /** @@ -198,48 +177,44 @@ export interface ResolvedTrait extends TraitBase, TraitWithAna * `TraitState`s. */ class TraitImpl { - state: TraitState = TraitState.PENDING; + state: TraitState = TraitState.Pending; handler: DecoratorHandler; detected: DetectResult; analysis: Readonly|null = null; resolution: Readonly|null = null; - diagnostics: ts.Diagnostic[]|null = null; + analysisDiagnostics: ts.Diagnostic[]|null = null; + resolveDiagnostics: ts.Diagnostic[]|null = null; constructor(handler: DecoratorHandler, detected: DetectResult) { this.handler = handler; this.detected = detected; } - toAnalyzed(analysis: A): AnalyzedTrait { + toAnalyzed(analysis: A|null, diagnostics: ts.Diagnostic[]|null): AnalyzedTrait { // Only pending traits can be analyzed. - this.assertTransitionLegal(TraitState.PENDING, TraitState.ANALYZED); + this.assertTransitionLegal(TraitState.Pending, TraitState.Analyzed); this.analysis = analysis; - this.state = TraitState.ANALYZED; + this.analysisDiagnostics = diagnostics; + this.state = TraitState.Analyzed; return this as AnalyzedTrait; } - toErrored(diagnostics: ts.Diagnostic[]): ErroredTrait { - // Pending traits (during analysis) or analyzed traits (during resolution) can produce - // diagnostics and enter an errored state. - this.assertTransitionLegal(TraitState.PENDING | TraitState.ANALYZED, TraitState.RESOLVED); - this.diagnostics = diagnostics; - this.analysis = null; - this.state = TraitState.ERRORED; - return this as ErroredTrait; - } - - toResolved(resolution: R): ResolvedTrait { + toResolved(resolution: R|null, diagnostics: ts.Diagnostic[]|null): ResolvedTrait { // Only analyzed traits can be resolved. - this.assertTransitionLegal(TraitState.ANALYZED, TraitState.RESOLVED); + this.assertTransitionLegal(TraitState.Analyzed, TraitState.Resolved); + if (this.analysis === null) { + throw new Error(`Cannot transition an Analyzed trait with a null analysis to Resolved`); + } this.resolution = resolution; - this.state = TraitState.RESOLVED; + this.state = TraitState.Resolved; + this.resolveDiagnostics = diagnostics; return this as ResolvedTrait; } toSkipped(): SkippedTrait { // Only pending traits can be skipped. - this.assertTransitionLegal(TraitState.PENDING, TraitState.SKIPPED); - this.state = TraitState.SKIPPED; + this.assertTransitionLegal(TraitState.Pending, TraitState.Skipped); + this.state = TraitState.Skipped; return this as SkippedTrait; } @@ -252,7 +227,7 @@ class TraitImpl { * transitions to take place. Hence, this assertion provides a little extra runtime protection. */ private assertTransitionLegal(allowedState: TraitState, transitionTo: TraitState): void { - if (!(this.state & allowedState)) { + if (!(this.state === allowedState)) { throw new Error(`Assertion failure: cannot transition from ${TraitState[this.state]} to ${ TraitState[transitionTo]}.`); } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index cbf673384fc76..81f36187cc6ca 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -364,6 +364,30 @@ runInEachFileSystem(os => { expect(jsContents).toContain('Hello World'); }); + it('should not report that broken components in modules are not components', () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'broken-cmp', + template: '{{ broken = "true" }}', // assignment not legal in this context + }) + export class BrokenCmp {} + + @NgModule({ + declarations: [BrokenCmp], + }) + export class Module { + broken = "false"; + } + `); + + const diags = env.driveDiagnostics(); + if (diags.some(diag => diag.code === ngErrorCode(ErrorCode.NGMODULE_INVALID_DECLARATION))) { + fail('Should not produce a diagnostic that BrokenCmp is not a component'); + } + }); + // This test triggers the Tsickle compiler which asserts that the file-paths // are valid for the real OS. When on non-Windows systems it doesn't like paths // that start with `C:`.