diff --git a/packages/compiler-cli/ngcc/src/analysis/util.ts b/packages/compiler-cli/ngcc/src/analysis/util.ts index dfe2b6188b..8261241305 100644 --- a/packages/compiler-cli/ngcc/src/analysis/util.ts +++ b/packages/compiler-cli/ngcc/src/analysis/util.ts @@ -18,6 +18,7 @@ class NoopDependencyTracker implements DependencyTracker { addResourceDependency(): void {} addTransitiveDependency(): void {} addTransitiveResources(): void {} + recordDependencyAnalysisFailure(): void {} } export const NOOP_DEPENDENCY_TRACKER: DependencyTracker = new NoopDependencyTracker(); diff --git a/packages/compiler-cli/src/ngtsc/incremental/api.ts b/packages/compiler-cli/src/ngtsc/incremental/api.ts index 10a222e2bd..d2f6a635c2 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/api.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/api.ts @@ -64,4 +64,12 @@ export interface DependencyTracker * `resourcesOf` they will not automatically be added to `from`. */ addTransitiveResources(from: T, resourcesOf: T): void; + + /** + * Record that the given file contains unresolvable dependencies. + * + * In practice, this means that the dependency graph cannot provide insight into the effects of + * future changes on that file. + */ + recordDependencyAnalysisFailure(file: T): void; } diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts b/packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts index 42fc122f00..97cf6cca5a 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts @@ -53,6 +53,10 @@ export class FileDependencyGraph i } } + recordDependencyAnalysisFailure(file: T): void { + this.nodeFor(file).failedAnalysis = true; + } + getResourceDependencies(from: T): AbsoluteFsPath[] { const node = this.nodes.get(from); @@ -97,6 +101,7 @@ export class FileDependencyGraph i this.nodes.set(sf, { dependsOn: new Set(node.dependsOn), usesResources: new Set(node.usesResources), + failedAnalysis: false, }); } } @@ -109,6 +114,7 @@ export class FileDependencyGraph i this.nodes.set(sf, { dependsOn: new Set(), usesResources: new Set(), + failedAnalysis: false, }); } return this.nodes.get(sf)!; @@ -122,6 +128,12 @@ export class FileDependencyGraph i function isLogicallyChanged( sf: T, node: FileNode, changedTsPaths: ReadonlySet, deletedTsPaths: ReadonlySet, changedResources: ReadonlySet): boolean { + // A file is assumed to have logically changed if its dependencies could not be determined + // accurately. + if (node.failedAnalysis) { + return true; + } + // A file is logically changed if it has physically changed itself (including being deleted). if (changedTsPaths.has(sf.fileName) || deletedTsPaths.has(sf.fileName)) { return true; @@ -146,6 +158,7 @@ function isLogicallyChanged( interface FileNode { dependsOn: Set; usesResources: Set; + failedAnalysis: boolean; } const EMPTY_SET: ReadonlySet = new Set(); diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts index 20f1932055..5579e6ccf3 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts @@ -220,6 +220,15 @@ export class StaticInterpreter { if (node.originalKeywordKind === ts.SyntaxKind.UndefinedKeyword) { return undefined; } else { + // Check if the symbol here is imported. + if (this.dependencyTracker !== null && this.host.getImportOfIdentifier(node) !== null) { + // It was, but no declaration for the node could be found. This means that the dependency + // graph for the current file cannot be properly updated to account for this (broken) + // import. Instead, the originating file is reported as failing dependency analysis, + // ensuring that future compilations will always attempt to re-resolve the previously + // broken identifier. + this.dependencyTracker.recordDependencyAnalysisFailure(context.originatingFile); + } return DynamicValue.fromUnknownIdentifier(node); } } diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts index 0f728bae44..4b6ad7443c 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts @@ -991,4 +991,5 @@ const fakeDepTracker: DependencyTracker = { addResourceDependency: () => undefined, addTransitiveDependency: () => undefined, addTransitiveResources: () => undefined, + recordDependencyAnalysisFailure: () => undefined, }; diff --git a/packages/language-service/ivy/test/compiler_spec.ts b/packages/language-service/ivy/test/compiler_spec.ts index f5acc12001..162502b880 100644 --- a/packages/language-service/ivy/test/compiler_spec.ts +++ b/packages/language-service/ivy/test/compiler_spec.ts @@ -60,4 +60,72 @@ describe('language-service/compiler integration', () => { expect(diags.map(diag => diag.messageText)) .toContain(`Type 'number' is not assignable to type 'string'.`); }); + + it('should handle broken imports during incremental build steps', () => { + // This test validates that the compiler's incremental APIs correctly handle a broken import + // when invoked via the Language Service. Testing this via the LS is important as only the LS + // requests Angular analysis in the presence of TypeScript-level errors. In the case of broken + // imports this distinction is especially important: Angular's incremental analysis is + // built on the the compiler's dependency graph, and this graph must be able to function even + // with broken imports. + // + // The test works by creating a component/module pair where the module imports and declares a + // component from a separate file. That component is initially not exported, meaning the + // module's import is broken. Angular will correctly complain that the NgModule is declaring a + // value which is not statically analyzable. + // + // Then, the component file is fixed to properly export the component class, and an incremental + // build step is performed. The compiler should recognize that the module's previous analysis + // is stale, even though it was not able to fully understand the import during the first pass. + + const moduleFile = absoluteFrom('/mod.ts'); + const componentFile = absoluteFrom('/cmp.ts'); + + const componentSource = (isExported: boolean): string => ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'some-cmp', + template: 'Not important', + }) + ${isExported ? 'export' : ''} class Cmp {} + `; + + const env = LanguageServiceTestEnvironment.setup([ + { + name: moduleFile, + contents: ` + import {NgModule} from '@angular/core'; + + import {Cmp} from './cmp'; + + @NgModule({ + declarations: [Cmp], + }) + export class Mod {} + `, + isRoot: true, + }, + { + name: componentFile, + contents: componentSource(/* start with component not exported */ false), + isRoot: true, + } + ]); + + // Angular should be complaining about the module not being understandable. + const programBefore = env.tsLS.getProgram()!; + const moduleSfBefore = programBefore.getSourceFile(moduleFile)!; + const ngDiagsBefore = env.ngLS.compilerFactory.getOrCreate().getDiagnostics(moduleSfBefore); + expect(ngDiagsBefore.length).toBe(1); + + // Fix the import. + env.updateFile(componentFile, componentSource(/* properly export the component */ true)); + + // Angular should stop complaining about the NgModule. + const programAfter = env.tsLS.getProgram()!; + const moduleSfAfter = programAfter.getSourceFile(moduleFile)!; + const ngDiagsAfter = env.ngLS.compilerFactory.getOrCreate().getDiagnostics(moduleSfAfter); + expect(ngDiagsAfter.length).toBe(0); + }); }); diff --git a/packages/language-service/ivy/test/env.ts b/packages/language-service/ivy/test/env.ts index aa7a1e011b..bfce6f1ad5 100644 --- a/packages/language-service/ivy/test/env.ts +++ b/packages/language-service/ivy/test/env.ts @@ -55,8 +55,8 @@ export interface TemplateOverwriteResult { export class LanguageServiceTestEnvironment { private constructor( - private tsLS: ts.LanguageService, readonly ngLS: LanguageService, - readonly host: MockServerHost) {} + readonly tsLS: ts.LanguageService, readonly ngLS: LanguageService, + readonly projectService: ts.server.ProjectService, readonly host: MockServerHost) {} static setup(files: TestFile[], options: TestableOptions = {}): LanguageServiceTestEnvironment { const fs = getFileSystem(); @@ -106,7 +106,7 @@ export class LanguageServiceTestEnvironment { const tsLS = project.getLanguageService(); const ngLS = new LanguageService(project, tsLS); - return new LanguageServiceTestEnvironment(tsLS, ngLS, host); + return new LanguageServiceTestEnvironment(tsLS, ngLS, projectService, host); } getClass(fileName: AbsoluteFsPath, className: string): ts.ClassDeclaration { @@ -136,6 +136,17 @@ export class LanguageServiceTestEnvironment { return {cursor, nodes, component, text}; } + updateFile(fileName: AbsoluteFsPath, contents: string): void { + const scriptInfo = this.projectService.getScriptInfo(fileName); + if (scriptInfo === undefined) { + throw new Error(`Could not find a file named ${fileName}`); + } + + // Get the current contents to find the length + const len = scriptInfo.getSnapshot().getLength(); + scriptInfo.editContent(0, len, contents); + } + expectNoSourceDiagnostics(): void { const program = this.tsLS.getProgram(); if (program === undefined) {