Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit adeeb84

Browse filesBrowse files
alxhubmhevery
alxhub
authored and
mhevery
committed
fix(compiler-cli): correct incremental behavior even with broken imports (#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
1 parent 178cc51 commit adeeb84
Copy full SHA for adeeb84

File tree

5 files changed

+32
-0
lines changed
Filter options

5 files changed

+32
-0
lines changed

‎packages/compiler-cli/ngcc/src/analysis/util.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/ngcc/src/analysis/util.ts
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ class NoopDependencyTracker implements DependencyTracker {
1818
addResourceDependency(): void {}
1919
addTransitiveDependency(): void {}
2020
addTransitiveResources(): void {}
21+
recordDependencyAnalysisFailure(): void {}
2122
}
2223

2324
export const NOOP_DEPENDENCY_TRACKER: DependencyTracker = new NoopDependencyTracker();

‎packages/compiler-cli/src/ngtsc/incremental/api.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/incremental/api.ts
+8Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,4 +64,12 @@ export interface DependencyTracker<T extends {fileName: string} = ts.SourceFile>
6464
* `resourcesOf` they will not automatically be added to `from`.
6565
*/
6666
addTransitiveResources(from: T, resourcesOf: T): void;
67+
68+
/**
69+
* Record that the given file contains unresolvable dependencies.
70+
*
71+
* In practice, this means that the dependency graph cannot provide insight into the effects of
72+
* future changes on that file.
73+
*/
74+
recordDependencyAnalysisFailure(file: T): void;
6775
}

‎packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts
+13Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,10 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
5353
}
5454
}
5555

56+
recordDependencyAnalysisFailure(file: T): void {
57+
this.nodeFor(file).failedAnalysis = true;
58+
}
59+
5660
getResourceDependencies(from: T): AbsoluteFsPath[] {
5761
const node = this.nodes.get(from);
5862

@@ -97,6 +101,7 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
97101
this.nodes.set(sf, {
98102
dependsOn: new Set(node.dependsOn),
99103
usesResources: new Set(node.usesResources),
104+
failedAnalysis: false,
100105
});
101106
}
102107
}
@@ -109,6 +114,7 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
109114
this.nodes.set(sf, {
110115
dependsOn: new Set<string>(),
111116
usesResources: new Set<AbsoluteFsPath>(),
117+
failedAnalysis: false,
112118
});
113119
}
114120
return this.nodes.get(sf)!;
@@ -122,6 +128,12 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
122128
function isLogicallyChanged<T extends {fileName: string}>(
123129
sf: T, node: FileNode, changedTsPaths: ReadonlySet<string>, deletedTsPaths: ReadonlySet<string>,
124130
changedResources: ReadonlySet<AbsoluteFsPath>): boolean {
131+
// A file is assumed to have logically changed if its dependencies could not be determined
132+
// accurately.
133+
if (node.failedAnalysis) {
134+
return true;
135+
}
136+
125137
// A file is logically changed if it has physically changed itself (including being deleted).
126138
if (changedTsPaths.has(sf.fileName) || deletedTsPaths.has(sf.fileName)) {
127139
return true;
@@ -146,6 +158,7 @@ function isLogicallyChanged<T extends {fileName: string}>(
146158
interface FileNode {
147159
dependsOn: Set<string>;
148160
usesResources: Set<AbsoluteFsPath>;
161+
failedAnalysis: boolean;
149162
}
150163

151164
const EMPTY_SET: ReadonlySet<any> = new Set<any>();

‎packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts
+9Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,15 @@ export class StaticInterpreter {
220220
if (node.originalKeywordKind === ts.SyntaxKind.UndefinedKeyword) {
221221
return undefined;
222222
} else {
223+
// Check if the symbol here is imported.
224+
if (this.dependencyTracker !== null && this.host.getImportOfIdentifier(node) !== null) {
225+
// It was, but no declaration for the node could be found. This means that the dependency
226+
// graph for the current file cannot be properly updated to account for this (broken)
227+
// import. Instead, the originating file is reported as failing dependency analysis,
228+
// ensuring that future compilations will always attempt to re-resolve the previously
229+
// broken identifier.
230+
this.dependencyTracker.recordDependencyAnalysisFailure(context.originatingFile);
231+
}
223232
return DynamicValue.fromUnknownIdentifier(node);
224233
}
225234
}

‎packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -991,4 +991,5 @@ const fakeDepTracker: DependencyTracker = {
991991
addResourceDependency: () => undefined,
992992
addTransitiveDependency: () => undefined,
993993
addTransitiveResources: () => undefined,
994+
recordDependencyAnalysisFailure: () => undefined,
994995
};

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.