Navigation Menu

Skip to content

Commit

Permalink
fix(compiler-cli): correct incremental behavior even with broken impo…
Browse files Browse the repository at this point in the history
…rts (#39967)

When the compiler is invoked via ngc or the Angular CLI, its APIs are used
under the assumption that Angular analysis/diagnostics are only requested if
the program has no TypeScript-level errors. A result of this assumption is
that the incremental engine has not needed to resolve changes via its
dependency graph when the program contained broken imports, since broken
imports are a TypeScript error.

The Angular Language Service for Ivy is using the compiler as a backend, and
exercising its incremental compilation APIs without enforcing this
assumption. As a result, the Language Service has run into issues where
broken imports cause incremental compilation to fail and produce incorrect
results.

This commit introduces a mechanism within the compiler to keep track of
files for which dependency analysis has failed, and to always treat such
files as potentially affected by future incremental steps.

PR Close #39967
  • Loading branch information
alxhub authored and mhevery committed Dec 5, 2020
1 parent 178cc51 commit adeeb84
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 0 deletions.
1 change: 1 addition & 0 deletions packages/compiler-cli/ngcc/src/analysis/util.ts
Expand Up @@ -18,6 +18,7 @@ class NoopDependencyTracker implements DependencyTracker {
addResourceDependency(): void {}
addTransitiveDependency(): void {}
addTransitiveResources(): void {}
recordDependencyAnalysisFailure(): void {}
}

export const NOOP_DEPENDENCY_TRACKER: DependencyTracker = new NoopDependencyTracker();
8 changes: 8 additions & 0 deletions packages/compiler-cli/src/ngtsc/incremental/api.ts
Expand Up @@ -64,4 +64,12 @@ export interface DependencyTracker<T extends {fileName: string} = ts.SourceFile>
* `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;
}
Expand Up @@ -53,6 +53,10 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
}
}

recordDependencyAnalysisFailure(file: T): void {
this.nodeFor(file).failedAnalysis = true;
}

getResourceDependencies(from: T): AbsoluteFsPath[] {
const node = this.nodes.get(from);

Expand Down Expand Up @@ -97,6 +101,7 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
this.nodes.set(sf, {
dependsOn: new Set(node.dependsOn),
usesResources: new Set(node.usesResources),
failedAnalysis: false,
});
}
}
Expand All @@ -109,6 +114,7 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
this.nodes.set(sf, {
dependsOn: new Set<string>(),
usesResources: new Set<AbsoluteFsPath>(),
failedAnalysis: false,
});
}
return this.nodes.get(sf)!;
Expand All @@ -122,6 +128,12 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
function isLogicallyChanged<T extends {fileName: string}>(
sf: T, node: FileNode, changedTsPaths: ReadonlySet<string>, deletedTsPaths: ReadonlySet<string>,
changedResources: ReadonlySet<AbsoluteFsPath>): 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;
Expand All @@ -146,6 +158,7 @@ function isLogicallyChanged<T extends {fileName: string}>(
interface FileNode {
dependsOn: Set<string>;
usesResources: Set<AbsoluteFsPath>;
failedAnalysis: boolean;
}

const EMPTY_SET: ReadonlySet<any> = new Set<any>();
Expand Up @@ -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);
}
}
Expand Down
Expand Up @@ -991,4 +991,5 @@ const fakeDepTracker: DependencyTracker = {
addResourceDependency: () => undefined,
addTransitiveDependency: () => undefined,
addTransitiveResources: () => undefined,
recordDependencyAnalysisFailure: () => undefined,
};

0 comments on commit adeeb84

Please sign in to comment.