diff --git a/.pullapprove.yml b/.pullapprove.yml index a8d5704f29a..2122274820e 100644 --- a/.pullapprove.yml +++ b/.pullapprove.yml @@ -1223,6 +1223,7 @@ groups: 'aio/content/errors/*.md', 'aio/content/guide/glossary.md', 'aio/content/guide/styleguide.md', + 'aio/content/examples/errors/**', 'aio/content/examples/styleguide/**', 'aio/content/images/guide/styleguide/*' ]) diff --git a/aio/content/errors/NG3003.md b/aio/content/errors/NG3003.md new file mode 100644 index 00000000000..6016d823d66 --- /dev/null +++ b/aio/content/errors/NG3003.md @@ -0,0 +1,55 @@ +@name Import Cycle Detected +@category compiler +@shortDescription Import cycles would need to be created to compile this component + +@description + +A component, directive or pipe that is referenced by this component would require the compiler +to add an import that would lead to a cycle of imports. For example, consider a scenario where +a `ParentComponent` references a `ChildComponent` in its template: + + + + + +There is already an import from `child.component.ts` to `parent.component.ts` since the `ChildComponent` +references the `ParentComponent` in its constructor. + +But note that the parent component's template contains ``. The generated code for this +template must therefore contain a reference to the `ChildComponent` class. In order to make this reference +the compiler would have to add an import from `parent.component.ts` to `child.component.ts`, which would +cause an import cycle: + +``` +parent.component.ts -> child.component.ts -> parent.component.ts +``` + +### Remote Scoping + +To avoid adding imports that create cycles, additional code is added to the `NgModule` class where +the component is declared that wires up the dependencies. This is known as "remote scoping". + + +### Libraries + +Unfortunately, "remote scoping" code is side-effectful, which prevents tree shaking, and cannot +be used in libraries. So when building libraries using the `"compilationMode": "partial"` setting, +any component that would require a cyclic import will cause this `NG3003` compiler error to be raised. + + +@debugging + +The cycle that would be generated is shown as part of the error message. For example: + + +The component ChildComponent is used in the template but importing it would create a cycle: +/parent.component.ts -> /child.component.ts -> /parent.component.ts + + +Use this to identify how the referenced component, pipe or directive has a dependency back to the +component being compiled. Here are some ideas for fixing the problem: + +* Try to re-arrange your dependencies to avoid the cycle. For example using an intermediate interface +that is stored in an independent file that can be imported to both dependent files without +causing an import cycle. +* Move the classes that reference each other into the same file, to avoid any imports between them. diff --git a/aio/content/examples/errors/cyclic-imports/child.component.ts b/aio/content/examples/errors/cyclic-imports/child.component.ts new file mode 100644 index 00000000000..94a1df51fe8 --- /dev/null +++ b/aio/content/examples/errors/cyclic-imports/child.component.ts @@ -0,0 +1,7 @@ +import { Component } from '@angular/core'; +import { ParentComponent } from './parent.component'; + +@Component({selector: 'app-child', template: 'The child!'}) +export class ChildComponent { + constructor(private parent: ParentComponent) {} +} diff --git a/aio/content/examples/errors/cyclic-imports/module.ts b/aio/content/examples/errors/cyclic-imports/module.ts new file mode 100644 index 00000000000..a153f4783ca --- /dev/null +++ b/aio/content/examples/errors/cyclic-imports/module.ts @@ -0,0 +1,8 @@ +import { NgModule } from '@angular/core'; +import { ChildComponent } from './child.component'; +import { ParentComponent } from './parent.component'; + +@NgModule({ + declarations: [ParentComponent, ChildComponent] +}) +export class AppModule {} diff --git a/aio/content/examples/errors/cyclic-imports/parent.component.ts b/aio/content/examples/errors/cyclic-imports/parent.component.ts new file mode 100644 index 00000000000..6f20a3bcc0f --- /dev/null +++ b/aio/content/examples/errors/cyclic-imports/parent.component.ts @@ -0,0 +1,4 @@ +import { Component } from '@angular/core'; + +@Component({selector: 'app-parent', template: ''}) +export class ParentComponent {} diff --git a/goldens/public-api/compiler-cli/error_code.d.ts b/goldens/public-api/compiler-cli/error_code.d.ts index a723b96758b..ccfbd47e710 100644 --- a/goldens/public-api/compiler-cli/error_code.d.ts +++ b/goldens/public-api/compiler-cli/error_code.d.ts @@ -17,6 +17,7 @@ export declare enum ErrorCode { COMPONENT_RESOURCE_NOT_FOUND = 2008, SYMBOL_NOT_EXPORTED = 3001, SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, + IMPORT_CYCLE_DETECTED = 3003, CONFIG_FLAT_MODULE_NO_INDEX = 4001, CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002, HOST_BINDING_PARSE_ERROR = 5001, diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 141b8160269..c0b48fd38ba 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -10,7 +10,7 @@ import * as ts from 'typescript'; import {ParsedConfiguration} from '../../..'; import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader} from '../../../src/ngtsc/annotations'; -import {CycleAnalyzer, ImportGraph} from '../../../src/ngtsc/cycles'; +import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../src/ngtsc/cycles'; import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics'; import {absoluteFromSourceFile, LogicalFileSystem, ReadonlyFileSystem} from '../../../src/ngtsc/file_system'; import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports'; @@ -101,8 +101,9 @@ export class DecorationAnalyzer { /* 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), + CycleHandlingStrategy.UseRemoteScoping, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, + NOOP_DEPENDENCY_TRACKER, this.injectableRegistry, + !!this.compilerOptions.annotateForClosureCompiler), // See the note in ngtsc about why this cast is needed. // clang-format off new DirectiveDecoratorHandler( diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 2769b064cd2..0dfddae2487 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -9,9 +9,9 @@ import {compileComponentFromMetadata, compileDeclareComponentFromMetadata, ConstantPool, CssSelector, DeclarationListEmitMode, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, Identifiers, InterpolationConfig, LexerRange, makeBindingParser, ParsedTemplate, ParseSourceFile, parseTemplate, R3ComponentDef, R3ComponentMetadata, R3FactoryTarget, R3TargetBinder, R3UsedDirectiveMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr} from '@angular/compiler'; import * as ts from 'typescript'; -import {CycleAnalyzer} from '../../cycles'; -import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; -import {absoluteFrom, AbsoluteFsPath, relative} from '../../file_system'; +import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../cycles'; +import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics'; +import {absoluteFrom, relative} from '../../file_system'; import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {DependencyTracker} from '../../incremental/api'; import {IndexingContext} from '../../indexer'; @@ -127,7 +127,8 @@ export class ComponentDecoratorHandler implements private enableI18nLegacyMessageIdFormat: boolean, private usePoisonedData: boolean, private i18nNormalizeLineEndingsInICUs: boolean|undefined, private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer, - private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder, + private cycleHandlingStrategy: CycleHandlingStrategy, private refEmitter: ReferenceEmitter, + private defaultImportRecorder: DefaultImportRecorder, private depTracker: DependencyTracker|null, private injectableRegistry: InjectableClassRegistry, private annotateForClosureCompiler: boolean) {} @@ -546,10 +547,12 @@ export class ComponentDecoratorHandler implements inputs: directive.inputs.propertyNames, outputs: directive.outputs.propertyNames, exportAs: directive.exportAs, + isComponent: directive.isComponent, }; }); - const usedPipes: {ref: Reference, pipeName: string, expression: Expression}[] = []; + type UsedPipe = {ref: Reference, pipeName: string, expression: Expression}; + const usedPipes: UsedPipe[] = []; for (const pipeName of bound.getUsedPipes()) { if (!pipes.has(pipeName)) { continue; @@ -564,10 +567,22 @@ export class ComponentDecoratorHandler implements // Scan through the directives/pipes actually used in the template and check whether any // import which needs to be generated would create a cycle. - const cycleDetected = usedDirectives.some(dir => this._isCyclicImport(dir.type, context)) || - usedPipes.some(pipe => this._isCyclicImport(pipe.expression, context)); + const cyclesFromDirectives = new Map(); + for (const usedDirective of usedDirectives) { + const cycle = this._checkForCyclicImport(usedDirective.ref, usedDirective.type, context); + if (cycle !== null) { + cyclesFromDirectives.set(usedDirective, cycle); + } + } + const cyclesFromPipes = new Map(); + for (const usedPipe of usedPipes) { + const cycle = this._checkForCyclicImport(usedPipe.ref, usedPipe.expression, context); + if (cycle !== null) { + cyclesFromPipes.set(usedPipe, cycle); + } + } - if (!cycleDetected) { + if (cyclesFromDirectives.size === 0 && cyclesFromPipes.size === 0) { // No cycle was detected. Record the imports that need to be created in the cycle detector // so that future cyclic import checks consider their production. for (const {type} of usedDirectives) { @@ -592,11 +607,28 @@ export class ComponentDecoratorHandler implements DeclarationListEmitMode.Closure : DeclarationListEmitMode.Direct; } else { - // Declaring the directiveDefs/pipeDefs arrays directly would require imports that would - // create a cycle. Instead, mark this component as requiring remote scoping, so that the - // NgModule file will take care of setting the directives for the component. - this.scopeRegistry.setComponentRemoteScope( - node, usedDirectives.map(dir => dir.ref), usedPipes.map(pipe => pipe.ref)); + if (this.cycleHandlingStrategy === CycleHandlingStrategy.UseRemoteScoping) { + // Declaring the directiveDefs/pipeDefs arrays directly would require imports that would + // create a cycle. Instead, mark this component as requiring remote scoping, so that the + // NgModule file will take care of setting the directives for the component. + this.scopeRegistry.setComponentRemoteScope( + node, usedDirectives.map(dir => dir.ref), usedPipes.map(pipe => pipe.ref)); + } else { + // We are not able to handle this cycle so throw an error. + const relatedMessages: ts.DiagnosticRelatedInformation[] = []; + for (const [dir, cycle] of cyclesFromDirectives) { + relatedMessages.push( + makeCyclicImportInfo(dir.ref, dir.isComponent ? 'component' : 'directive', cycle)); + } + for (const [pipe, cycle] of cyclesFromPipes) { + relatedMessages.push(makeCyclicImportInfo(pipe.ref, 'pipe', cycle)); + } + throw new FatalDiagnosticError( + ErrorCode.IMPORT_CYCLE_DETECTED, node, + 'One or more import cycles would need to be created to compile this component, ' + + 'which is not supported by the current compiler configuration.', + relatedMessages); + } } } @@ -1052,14 +1084,20 @@ export class ComponentDecoratorHandler implements return this.moduleResolver.resolveModule(expr.value.moduleName!, origin.fileName); } - private _isCyclicImport(expr: Expression, origin: ts.SourceFile): boolean { - const imported = this._expressionToImportedFile(expr, origin); - if (imported === null) { - return false; + /** + * Check whether adding an import from `origin` to the source-file corresponding to `expr` would + * create a cyclic import. + * + * @returns a `Cycle` object if a cycle would be created, otherwise `null`. + */ + private _checkForCyclicImport(ref: Reference, expr: Expression, origin: ts.SourceFile): Cycle + |null { + const importedFile = this._expressionToImportedFile(expr, origin); + if (importedFile === null) { + return null; } - // Check whether the import is legal. - return this.cycleAnalyzer.wouldCreateCycle(origin, imported); + return this.cycleAnalyzer.wouldCreateCycle(origin, importedFile); } private _recordSyntheticImport(expr: Expression, origin: ts.SourceFile): void { @@ -1219,3 +1257,15 @@ interface ExternalTemplateDeclaration extends CommonTemplateDeclaration { * record without needing to parse the original decorator again. */ type TemplateDeclaration = InlineTemplateDeclaration|ExternalTemplateDeclaration; + +/** + * Generate a diagnostic related information object that describes a potential cyclic import path. + */ +function makeCyclicImportInfo( + ref: Reference, type: string, cycle: Cycle): ts.DiagnosticRelatedInformation { + const name = ref.debugName || '(unknown)'; + const path = cycle.getPath().map(sf => sf.fileName).join(' -> '); + const message = + `The ${type} '${name}' is used in the template but importing it would create a cycle: `; + return makeRelatedInformation(ref.node, message + path); +} diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index eff64819b07..ac5db42f1f1 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -9,7 +9,7 @@ import {ConstantPool} from '@angular/compiler'; import * as ts from 'typescript'; -import {CycleAnalyzer, ImportGraph} from '../../cycles'; +import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../cycles'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {absoluteFrom} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; @@ -73,6 +73,7 @@ function setup(program: ts.Program, options: ts.CompilerOptions, host: ts.Compil /* i18nNormalizeLineEndingsInICUs */ undefined, moduleResolver, cycleAnalyzer, + CycleHandlingStrategy.UseRemoteScoping, refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, /* depTracker */ null, diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 3f55b98f97d..f2b4a8e7b43 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -10,7 +10,7 @@ import {Type} from '@angular/compiler'; import * as ts from 'typescript'; import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, NoopReferencesRegistry, PipeDecoratorHandler, ReferencesRegistry} from '../../annotations'; -import {CycleAnalyzer, ImportGraph} from '../../cycles'; +import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../cycles'; import {COMPILER_ERRORS_WITH_GUIDES, ERROR_DETAILS_PAGE_BASE_URL, ErrorCode, ngErrorCode} from '../../diagnostics'; import {checkForPrivateExports, ReferenceGraph} from '../../entry_point'; import {LogicalFileSystem, resolve} from '../../file_system'; @@ -985,6 +985,16 @@ export class NgCompiler { const defaultImportTracker = new DefaultImportTracker(); const resourceRegistry = new ResourceRegistry(); + const compilationMode = + this.options.compilationMode === 'partial' ? CompilationMode.PARTIAL : CompilationMode.FULL; + + // Cycles are handled in full compilation mode by "remote scoping". + // "Remote scoping" does not work well with tree shaking for libraries. + // So in partial compilation mode, when building a library, a cycle will cause an error. + const cycleHandlingStrategy = compilationMode === CompilationMode.FULL ? + CycleHandlingStrategy.UseRemoteScoping : + CycleHandlingStrategy.Error; + // Set up the IvyCompilation, which manages state for the Ivy transformer. const handlers: DecoratorHandler[] = [ new ComponentDecoratorHandler( @@ -994,8 +1004,8 @@ export class NgCompiler { this.options.i18nUseExternalIds !== false, this.options.enableI18nLegacyMessageIdFormat !== false, this.usePoisonedData, this.options.i18nNormalizeLineEndingsInICUs, this.moduleResolver, this.cycleAnalyzer, - refEmitter, defaultImportTracker, this.incrementalDriver.depGraph, injectableRegistry, - this.closureCompilerEnabled), + cycleHandlingStrategy, refEmitter, defaultImportTracker, this.incrementalDriver.depGraph, + injectableRegistry, this.closureCompilerEnabled), // TODO(alxhub): understand why the cast here is necessary (something to do with `null` // not being assignable to `unknown` when wrapped in `Readonly`). // clang-format off @@ -1022,8 +1032,6 @@ export class NgCompiler { this.closureCompilerEnabled, injectableRegistry, this.options.i18nInLocale), ]; - const compilationMode = - this.options.compilationMode === 'partial' ? CompilationMode.PARTIAL : CompilationMode.FULL; const traitCompiler = new TraitCompiler( handlers, reflector, this.perfRecorder, this.incrementalDriver, this.options.compileNonExportedClasses !== false, compilationMode, dtsTransforms); diff --git a/packages/compiler-cli/src/ngtsc/cycles/index.ts b/packages/compiler-cli/src/ngtsc/cycles/index.ts index ec2670b62ff..251d72e27eb 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/index.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/index.ts @@ -6,5 +6,5 @@ * found in the LICENSE file at https://angular.io/license */ -export {CycleAnalyzer} from './src/analyzer'; +export {Cycle, CycleAnalyzer, CycleHandlingStrategy} from './src/analyzer'; export {ImportGraph} from './src/imports'; diff --git a/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts b/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts index 7bcd1b4457f..d9cd9e4533b 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts @@ -17,11 +17,17 @@ export class CycleAnalyzer { constructor(private importGraph: ImportGraph) {} /** - * Check whether adding an import from `from` to `to` would create a cycle in the `ts.Program`. + * Check for a cycle to be created in the `ts.Program` by adding an import between `from` and + * `to`. + * + * @returns a `Cycle` object if an import between `from` and `to` would create a cycle; `null` + * otherwise. */ - wouldCreateCycle(from: ts.SourceFile, to: ts.SourceFile): boolean { + wouldCreateCycle(from: ts.SourceFile, to: ts.SourceFile): Cycle|null { // Import of 'from' -> 'to' is illegal if an edge 'to' -> 'from' already exists. - return this.importGraph.transitiveImportsOf(to).has(from); + return this.importGraph.transitiveImportsOf(to).has(from) ? + new Cycle(this.importGraph, from, to) : + null; } /** @@ -34,3 +40,35 @@ export class CycleAnalyzer { this.importGraph.addSyntheticImport(from, to); } } + +/** + * Represents an import cycle between `from` and `to` in the program. + * + * This class allows us to do the work to compute the cyclic path between `from` and `to` only if + * needed. + */ +export class Cycle { + constructor( + private importGraph: ImportGraph, readonly from: ts.SourceFile, readonly to: ts.SourceFile) {} + + /** + * Compute an array of source-files that illustrates the cyclic path between `from` and `to`. + * + * Note that a `Cycle` will not be created unless a path is available between `to` and `from`, + * so `findPath()` will never return `null`. + */ + getPath(): ts.SourceFile[] { + return [this.from, ...this.importGraph.findPath(this.to, this.from)!]; + } +} + + +/** + * What to do if a cycle is detected. + */ +export const enum CycleHandlingStrategy { + /** Add "remote scoping" code to avoid creating a cycle. */ + UseRemoteScoping, + /** Fail the compilation with an error. */ + Error, +} diff --git a/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts b/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts index 988f0da81d4..4f9915a283d 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts @@ -52,6 +52,44 @@ export class ImportGraph { }); } + /** + * Find an import path from the `start` SourceFile to the `end` SourceFile. + * + * This function implements a breadth first search that results in finding the + * shortest path between the `start` and `end` points. + * + * @param start the starting point of the path. + * @param end the ending point of the path. + * @returns an array of source files that connect the `start` and `end` source files, or `null` if + * no path could be found. + */ + findPath(start: ts.SourceFile, end: ts.SourceFile): ts.SourceFile[]|null { + if (start === end) { + // Escape early for the case where `start` and `end` are the same. + return [start]; + } + + const found = new Set([start]); + const queue: Found[] = [new Found(start, null)]; + + while (queue.length > 0) { + const current = queue.shift()!; + const imports = this.importsOf(current.sourceFile); + for (const importedFile of imports) { + if (!found.has(importedFile)) { + const next = new Found(importedFile, current); + if (next.sourceFile === end) { + // We have hit the target `end` path so we can stop here. + return next.toPath(); + } + found.add(importedFile); + queue.push(next); + } + } + } + return null; + } + /** * Add a record of an import from `sf` to `imported`, that's not present in the original * `ts.Program` but will be remembered by the `ImportGraph`. @@ -84,3 +122,27 @@ export class ImportGraph { function isLocalFile(sf: ts.SourceFile): boolean { return !sf.fileName.endsWith('.d.ts'); } + +/** + * A helper class to track which SourceFiles are being processed when searching for a path in + * `getPath()` above. + */ +class Found { + constructor(readonly sourceFile: ts.SourceFile, readonly parent: Found|null) {} + + /** + * Back track through this found SourceFile and its ancestors to generate an array of + * SourceFiles that form am import path between two SourceFiles. + */ + toPath(): ts.SourceFile[] { + const array: ts.SourceFile[] = []; + let current: Found|null = this; + while (current !== null) { + array.push(current.sourceFile); + current = current.parent; + } + // Pushing and then reversing, O(n), rather than unshifting repeatedly, O(n^2), avoids + // manipulating the array on every iteration: https://stackoverflow.com/a/26370620 + return array.reverse(); + } +} diff --git a/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts b/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts index f800020cb1b..0d697e30051 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts @@ -9,9 +9,9 @@ import * as ts from 'typescript'; import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; import {ModuleResolver} from '../../imports'; -import {CycleAnalyzer} from '../src/analyzer'; +import {Cycle, CycleAnalyzer} from '../src/analyzer'; import {ImportGraph} from '../src/imports'; -import {makeProgramFromGraph} from './util'; +import {importPath, makeProgramFromGraph} from './util'; runInEachFileSystem(() => { describe('cycle analyzer', () => { @@ -22,41 +22,53 @@ runInEachFileSystem(() => { const {program, analyzer} = makeAnalyzer('a:b,c;b;c'); const b = getSourceFileOrError(program, (_('/b.ts'))); const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(analyzer.wouldCreateCycle(b, c)).toBe(false); - expect(analyzer.wouldCreateCycle(c, b)).toBe(false); + expect(analyzer.wouldCreateCycle(b, c)).toBe(null); + expect(analyzer.wouldCreateCycle(c, b)).toBe(null); }); it('should detect a simple cycle between two files', () => { const {program, analyzer} = makeAnalyzer('a:b;b'); const a = getSourceFileOrError(program, (_('/a.ts'))); const b = getSourceFileOrError(program, (_('/b.ts'))); - expect(analyzer.wouldCreateCycle(a, b)).toBe(false); - expect(analyzer.wouldCreateCycle(b, a)).toBe(true); + expect(analyzer.wouldCreateCycle(a, b)).toBe(null); + const cycle = analyzer.wouldCreateCycle(b, a); + expect(cycle).toBeInstanceOf(Cycle); + expect(importPath(cycle!.getPath())).toEqual('b,a,b'); }); it('should detect a cycle with a re-export in the chain', () => { const {program, analyzer} = makeAnalyzer('a:*b;b:c;c'); const a = getSourceFileOrError(program, (_('/a.ts'))); + const b = getSourceFileOrError(program, (_('/b.ts'))); const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(analyzer.wouldCreateCycle(a, c)).toBe(false); - expect(analyzer.wouldCreateCycle(c, a)).toBe(true); + expect(analyzer.wouldCreateCycle(a, c)).toBe(null); + const cycle = analyzer.wouldCreateCycle(c, a); + expect(cycle).toBeInstanceOf(Cycle); + expect(importPath(cycle!.getPath())).toEqual('c,a,b,c'); }); it('should detect a cycle in a more complex program', () => { const {program, analyzer} = makeAnalyzer('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f:c;g;h:g'); const b = getSourceFileOrError(program, (_('/b.ts'))); + const c = getSourceFileOrError(program, (_('/c.ts'))); + const e = getSourceFileOrError(program, (_('/e.ts'))); + const f = getSourceFileOrError(program, (_('/f.ts'))); const g = getSourceFileOrError(program, (_('/g.ts'))); - expect(analyzer.wouldCreateCycle(b, g)).toBe(false); - expect(analyzer.wouldCreateCycle(g, b)).toBe(true); + expect(analyzer.wouldCreateCycle(b, g)).toBe(null); + const cycle = analyzer.wouldCreateCycle(g, b); + expect(cycle).toBeInstanceOf(Cycle); + expect(importPath(cycle!.getPath())).toEqual('g,b,f,c,g'); }); it('should detect a cycle caused by a synthetic edge', () => { const {program, analyzer} = makeAnalyzer('a:b,c;b;c'); const b = getSourceFileOrError(program, (_('/b.ts'))); const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(analyzer.wouldCreateCycle(b, c)).toBe(false); + expect(analyzer.wouldCreateCycle(b, c)).toBe(null); analyzer.recordSyntheticImport(c, b); - expect(analyzer.wouldCreateCycle(b, c)).toBe(true); + const cycle = analyzer.wouldCreateCycle(b, c); + expect(cycle).toBeInstanceOf(Cycle); + expect(importPath(cycle!.getPath())).toEqual('b,c,b'); }); }); diff --git a/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts b/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts index b834fa70fc8..6f1fe6ad4ed 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts @@ -10,46 +10,76 @@ import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../file_syst import {runInEachFileSystem} from '../../file_system/testing'; import {ModuleResolver} from '../../imports'; import {ImportGraph} from '../src/imports'; -import {makeProgramFromGraph} from './util'; +import {importPath, makeProgramFromGraph} from './util'; runInEachFileSystem(() => { - describe('import graph', () => { + describe('ImportGraph', () => { let _: typeof absoluteFrom; beforeEach(() => _ = absoluteFrom); - it('should record imports of a simple program', () => { - const {program, graph} = makeImportGraph('a:b;b:c;c'); - const a = getSourceFileOrError(program, (_('/a.ts'))); - const b = getSourceFileOrError(program, (_('/b.ts'))); - const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(importsToString(graph.importsOf(a))).toBe('b'); - expect(importsToString(graph.importsOf(b))).toBe('c'); + describe('importsOf()', () => { + it('should record imports of a simple program', () => { + const {program, graph} = makeImportGraph('a:b;b:c;c'); + const a = getSourceFileOrError(program, (_('/a.ts'))); + const b = getSourceFileOrError(program, (_('/b.ts'))); + const c = getSourceFileOrError(program, (_('/c.ts'))); + expect(importsToString(graph.importsOf(a))).toBe('b'); + expect(importsToString(graph.importsOf(b))).toBe('c'); + }); }); - it('should calculate transitive imports of a simple program', () => { - const {program, graph} = makeImportGraph('a:b;b:c;c'); - const a = getSourceFileOrError(program, (_('/a.ts'))); - const b = getSourceFileOrError(program, (_('/b.ts'))); - const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(importsToString(graph.transitiveImportsOf(a))).toBe('a,b,c'); + describe('transitiveImportsOf()', () => { + it('should calculate transitive imports of a simple program', () => { + const {program, graph} = makeImportGraph('a:b;b:c;c'); + const a = getSourceFileOrError(program, (_('/a.ts'))); + const b = getSourceFileOrError(program, (_('/b.ts'))); + const c = getSourceFileOrError(program, (_('/c.ts'))); + expect(importsToString(graph.transitiveImportsOf(a))).toBe('a,b,c'); + }); + + it('should calculate transitive imports in a more complex program (with a cycle)', () => { + const {program, graph} = makeImportGraph('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f;g:e;h:g'); + const c = getSourceFileOrError(program, (_('/c.ts'))); + expect(importsToString(graph.transitiveImportsOf(c))).toBe('c,e,f,g,h'); + }); + + it('should reflect the addition of a synthetic import', () => { + const {program, graph} = makeImportGraph('a:b,c,d;b;c;d:b'); + const b = getSourceFileOrError(program, (_('/b.ts'))); + const c = getSourceFileOrError(program, (_('/c.ts'))); + const d = getSourceFileOrError(program, (_('/d.ts'))); + expect(importsToString(graph.importsOf(b))).toEqual(''); + expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,d'); + graph.addSyntheticImport(b, c); + expect(importsToString(graph.importsOf(b))).toEqual('c'); + expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,c,d'); + }); }); - it('should calculate transitive imports in a more complex program (with a cycle)', () => { - const {program, graph} = makeImportGraph('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f;g:e;h:g'); - const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(importsToString(graph.transitiveImportsOf(c))).toBe('c,e,f,g,h'); - }); + describe('findPath()', () => { + it('should be able to compute the path between two source files if there is a cycle', () => { + const {program, graph} = makeImportGraph('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f;g:e;h:g'); + const a = getSourceFileOrError(program, (_('/a.ts'))); + const b = getSourceFileOrError(program, (_('/b.ts'))); + const c = getSourceFileOrError(program, (_('/c.ts'))); + const e = getSourceFileOrError(program, (_('/e.ts'))); + expect(importPath(graph.findPath(a, a)!)).toBe('a'); + expect(importPath(graph.findPath(a, b)!)).toBe('a,b'); + expect(importPath(graph.findPath(c, e)!)).toBe('c,g,e'); + expect(graph.findPath(e, c)).toBe(null); + expect(graph.findPath(b, c)).toBe(null); + }); - it('should reflect the addition of a synthetic import', () => { - const {program, graph} = makeImportGraph('a:b,c,d;b;c;d:b'); - const b = getSourceFileOrError(program, (_('/b.ts'))); - const c = getSourceFileOrError(program, (_('/c.ts'))); - const d = getSourceFileOrError(program, (_('/d.ts'))); - expect(importsToString(graph.importsOf(b))).toEqual(''); - expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,d'); - graph.addSyntheticImport(b, c); - expect(importsToString(graph.importsOf(b))).toEqual('c'); - expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,c,d'); + it('should handle circular dependencies within the path between `from` and `to`', () => { + // a -> b -> c -> d + // ^----/ | + // ^---------/ + const {program, graph} = makeImportGraph('a:b;b:a,c;c:a,d;d'); + const a = getSourceFileOrError(program, (_('/a.ts'))); + const c = getSourceFileOrError(program, (_('/c.ts'))); + const d = getSourceFileOrError(program, (_('/d.ts'))); + expect(importPath(graph.findPath(a, d)!)).toBe('a,b,c,d'); + }); }); }); diff --git a/packages/compiler-cli/src/ngtsc/cycles/test/util.ts b/packages/compiler-cli/src/ngtsc/cycles/test/util.ts index 09f602c6d12..c4a8ca4ed42 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/test/util.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/test/util.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import * as ts from 'typescript'; -import {PathManipulation} from '../../file_system'; +import {getFileSystem, PathManipulation} from '../../file_system'; import {TestFile} from '../../file_system/testing'; import {makeProgram} from '../../testing'; @@ -56,3 +56,8 @@ export function makeProgramFromGraph(fs: PathManipulation, graph: string): { }); return makeProgram(files); } + +export function importPath(files: ts.SourceFile[]): string { + const fs = getFileSystem(); + return files.map(sf => fs.basename(sf.fileName).replace('.ts', '')).join(','); +} diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index d1fed60b829..89292b56fa3 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -52,6 +52,11 @@ export enum ErrorCode { SYMBOL_NOT_EXPORTED = 3001, SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, + /** + * Raised when a relationship between directives and/or pipes would cause a cyclic import to be + * created that cannot be handled, such as in partial compilation mode. + */ + IMPORT_CYCLE_DETECTED = 3003, CONFIG_FLAT_MODULE_NO_INDEX = 4001, CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002, @@ -192,6 +197,7 @@ export const ERROR_DETAILS_PAGE_BASE_URL = 'https://angular.io/errors'; */ export const COMPILER_ERRORS_WITH_GUIDES = new Set([ ErrorCode.DECORATOR_ARG_NOT_LITERAL, + ErrorCode.IMPORT_CYCLE_DETECTED, ErrorCode.PARAM_MISSING_TOKEN, ErrorCode.SCHEMA_INVALID_ELEMENT, ErrorCode.SCHEMA_INVALID_ATTRIBUTE, diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index cfe8fb32c2d..1a6761191d2 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -4774,6 +4774,53 @@ runInEachFileSystem(os => { const jsContents = env.getContents('test.js'); expect(jsContents).not.toMatch(/i\d\.ɵɵsetComponentScope\([^)]*OtherComponent[^)]*\)/); }); + + it('should detect a simple cycle and fatally error if doing partial-compilation', () => { + env.tsconfig({ + compilationMode: 'partial', + }); + + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + import {NormalComponent} from './cyclic'; + + @Component({ + selector: 'cyclic-component', + template: 'Importing this causes a cycle', + }) + export class CyclicComponent {} + + @NgModule({ + declarations: [NormalComponent, CyclicComponent], + }) + export class Module {} + `); + + env.write('cyclic.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'normal-component', + template: '', + }) + export class NormalComponent {} + `); + + const diagnostics = env.driveDiagnostics(); + expect(diagnostics.length).toEqual(1); + const error = diagnostics[0]; + expect(error.code).toBe(ngErrorCode(ErrorCode.IMPORT_CYCLE_DETECTED)); + expect(error.messageText) + .toEqual( + 'One or more import cycles would need to be created to compile this component, ' + + 'which is not supported by the current compiler configuration.'); + const _abs = absoluteFrom; + expect(error.relatedInformation?.map(diag => diag.messageText)).toEqual([ + `The component 'CyclicComponent' is used in the template ` + + `but importing it would create a cycle: ` + + `${_abs('/cyclic.ts')} -> ${_abs('/test.ts')} -> ${_abs('/cyclic.ts')}`, + ]); + }); }); describe('local refs', () => { diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index 89946ac5f92..139326d856c 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -277,6 +277,11 @@ export interface R3UsedDirectiveMetadata { * Name under which the directive is exported, if any (exportAs in Angular). Null otherwise. */ exportAs: string[]|null; + + /** + * If true then this directive is actually a component; otherwise it is not. + */ + isComponent?: boolean; } /**