From ecffc3557fe1bff9718c01277498e877ca44588d Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 19 Mar 2020 11:29:58 -0700 Subject: [PATCH] perf(compiler-cli): perform template type-checking incrementally (#36211) This optimization builds on a lot of prior work to finally make type- checking of templates incremental. Incrementality requires two main components: - the ability to reuse work from a prior compilation. - the ability to know when changes in the current program invalidate that prior work. Prior to this commit, on every type-checking pass the compiler would generate new .ngtypecheck files for each original input file in the program. 1. (Build #1 main program): empty .ngtypecheck files generated for each original input file. 2. (Build #1 type-check program): .ngtypecheck contents overridden for those which have corresponding components that need type-checked. 3. (Build #2 main program): throw away old .ngtypecheck files and generate new empty ones. 4. (Build #2 type-check program): same as step 2. With this commit, the `IncrementalDriver` now tracks template type-checking _metadata_ for each input file. The metadata contains information about source mappings for generated type-checking code, as well as some diagnostics which were discovered at type-check analysis time. The actual type-checking code is stored in the TypeScript AST for type-checking files, which is now re-used between programs as follows: 1. (Build #1 main program): empty .ngtypecheck files generated for each original input file. 2. (Build #1 type-check program): .ngtypecheck contents overridden for those which have corresponding components that need type-checked, and the metadata registered in the `IncrementalDriver`. 3. (Build #2 main program): The `TypeCheckShimGenerator` now reuses _all_ .ngtypecheck `ts.SourceFile` shims from build #1's type-check program in the construction of build #2's main program. Some of the contents of these files might be stale (if a component's template changed, for example), but wholesale reuse here prevents unnecessary changes in the contents of the program at this point and makes TypeScript's job a lot easier. 4. (Build #2 type-check program): For those input files which have not "logically changed" (meaning components within are semantically the same as they were before), the compiler will re-use the type-check file metadata from build #1, and _not_ generate a new .ngtypecheck shim. For components which have logically changed or where the previous .ngtypecheck contents cannot otherwise be reused, code generation happens as before. PR Close #36211 --- .../ngcc/src/analysis/ngcc_trait_compiler.ts | 6 +- .../src/ngtsc/core/src/compiler.ts | 7 +- .../src/ngtsc/incremental/BUILD.bazel | 1 + .../src/ngtsc/incremental/README.md | 27 +- .../compiler-cli/src/ngtsc/incremental/api.ts | 9 +- .../src/ngtsc/incremental/src/noop.ts | 5 +- .../src/ngtsc/incremental/src/state.ts | 53 +++- .../compiler-cli/src/ngtsc/shims/index.ts | 2 +- .../src/ngtsc/shims/src/expando.ts | 10 + .../src/ngtsc/transform/src/compilation.ts | 14 +- .../src/ngtsc/typecheck/BUILD.bazel | 1 + .../src/ngtsc/typecheck/src/checker.ts | 30 +- .../src/ngtsc/typecheck/src/context.ts | 20 ++ .../src/ngtsc/typecheck/src/host.ts | 57 ++-- .../src/ngtsc/typecheck/src/shim.ts | 12 +- .../src/ngtsc/typecheck/test/BUILD.bazel | 1 + .../src/ngtsc/typecheck/test/test_utils.ts | 12 +- .../test/ngtsc/incremental_spec.ts | 256 ++++++++++++++++++ 18 files changed, 451 insertions(+), 72 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts b/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts index 1c526bc78a..55de1e82a2 100644 --- a/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts +++ b/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts @@ -82,8 +82,12 @@ export class NgccTraitCompiler extends TraitCompiler { } } -class NoIncrementalBuild implements IncrementalBuild { +class NoIncrementalBuild implements IncrementalBuild { priorWorkFor(sf: ts.SourceFile): any[]|null { return null; } + + priorTypeCheckingResultsFor(): null { + return null; + } } diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 83f7f91b91..7b411a30a0 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -482,8 +482,6 @@ export class NgCompiler { } private getTemplateDiagnostics(): ReadonlyArray { - const host = this.host; - // Skip template type-checking if it's disabled. if (this.options.ivyTemplateTypeCheck === false && !this.fullTemplateTypeCheck) { return []; @@ -493,7 +491,8 @@ export class NgCompiler { // Execute the typeCheck phase of each decorator in the program. const prepSpan = this.perfRecorder.start('typeCheckPrep'); - compilation.templateTypeChecker.refresh(); + const results = compilation.templateTypeChecker.refresh(); + this.incrementalDriver.recordSuccessfulTypeCheck(results.perFileData); this.perfRecorder.stop(prepSpan); // Get the diagnostics. @@ -728,7 +727,7 @@ export class NgCompiler { const templateTypeChecker = new TemplateTypeChecker( this.tsProgram, this.typeCheckingProgramStrategy, traitCompiler, - this.getTypeCheckingConfig(), refEmitter, reflector); + this.getTypeCheckingConfig(), refEmitter, reflector, this.incrementalDriver); return { isCore, diff --git a/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel b/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel index 9894e3e96e..db2afaa805 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel @@ -16,6 +16,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/scope", "//packages/compiler-cli/src/ngtsc/transform", + "//packages/compiler-cli/src/ngtsc/typecheck", "//packages/compiler-cli/src/ngtsc/util", "@npm//typescript", ], diff --git a/packages/compiler-cli/src/ngtsc/incremental/README.md b/packages/compiler-cli/src/ngtsc/incremental/README.md index a65e5e7c24..94faa48071 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/README.md +++ b/packages/compiler-cli/src/ngtsc/incremental/README.md @@ -33,6 +33,22 @@ If the file is logically unchanged, ngtsc will reuse the previous analysis and o If the file is logically changed, ngtsc will re-analyze it. +## Reuse of template type-checking code + +Generally speaking, the generation of a template type-checking "shim" for an input component file is a time-consuming operation. Such generation produces several outputs: + +1) The text of the template type-checking shim file, which can later be fed to TypeScript for the production of raw diagnostics. +2) Metadata regarding source mappings within the template type-checking shim, which can be used to convert the raw diagnostics into mapped template diagnostics. +3) "Construction" diagnostics, which are diagnostics produced as a side effect of generation of the shim itself. + +When a component file is logically unchanged, ngtsc attempts to reuse this generation work. As part of creating both the new emit program and template type-checking program, the `ts.SourceFile` of the shim for the component file is included directly and not re-generated. + +At the same time, the metadata and construction diagnostics are passed via the incremental build system. When TS gets diagnostics for the shim file, this metadata is used to convert them into mapped template diagnostics for delivery to the user. + +### Limitations on template type-checking reuse + +In certain cases the template type-checking system is unable to use the existing shim code. If the component is logically changed, the shim is regenerated in case its contents may have changed. If generating the shim itself required the use of any "inline" code (type-checking code which needs to be inserted into the component file instead for some reason), it also becomes ineligible for reuse. + ## Skipping emit ngtsc makes a decision to skip the emit of a file if it can prove that the contents of the file will not have changed since the last good compilation. To prove this, two conditions must be true. @@ -135,13 +151,4 @@ Currently the compiler does not distinguish these two cases, and conservatively ## Skipping template type-checking -For certain kinds of changes, it may be possible to avoid the cost of generating and checking template type-checking files. Several levels of this can be imagined. - -For resource-only changes, only the component(s) which have changed resources need to be re-checked. No other components could be affected, so previously produced diagnostics are still valid. - -For arbitrary source changes, things get a bit more complicated. A change to any .ts file could affect types anywhere in the program (think `declare global ...`). If a set of affected components can be determined (perhaps via the import graph that the cycle analyzer extracts?) and it can be proven that the change does not impact any global types (exactly how to do this is left as an exercise for the reader), then type-checking could be skipped for other components in the mix. - -If the above is too complex, then certain kinds of type changes might allow for the reuse of the text of some template type-checking files, if it can be proven that none of the inputs to their generation have changed. This is useful for two very important reasons. - -1) Generating (and subsequently parsing) the template type-checking files is expensive. -2) Under ideal conditions, after an initial template type-checking program is created, it may be possible to reuse it for emit _and_ type-checking in subsequent builds. This would be a pretty advanced optimization but would save creation of a second `ts.Program` on each valid rebuild. +Under ideal conditions, after an initial template type-checking program is created, it may be possible to reuse it for emit _and_ type-checking in subsequent builds. This would be a pretty advanced optimization but would save creation of a second `ts.Program` on each valid rebuild. diff --git a/packages/compiler-cli/src/ngtsc/incremental/api.ts b/packages/compiler-cli/src/ngtsc/incremental/api.ts index 41c3f61bbf..7292f6d816 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/api.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/api.ts @@ -14,12 +14,19 @@ import {AbsoluteFsPath} from '../file_system'; * * `W` is a generic type representing a unit of work. This is generic to avoid a cyclic dependency * between the incremental engine API definition and its consumer(s). + * `T` is a generic type representing template type-checking data for a particular file, which is + * generic for the same reason. */ -export interface IncrementalBuild { +export interface IncrementalBuild { /** * Retrieve the prior analysis work, if any, done for the given source file. */ priorWorkFor(sf: ts.SourceFile): W[]|null; + + /** + * Retrieve the prior type-checking work, if any, that's been done for the given source file. + */ + priorTypeCheckingResultsFor(sf: ts.SourceFile): T|null; } /** diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/noop.ts b/packages/compiler-cli/src/ngtsc/incremental/src/noop.ts index 3ad486d2c4..cdb3d06a48 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/noop.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/noop.ts @@ -8,6 +8,7 @@ import {IncrementalBuild} from '../api'; -export const NOOP_INCREMENTAL_BUILD: IncrementalBuild = { - priorWorkFor: () => null +export const NOOP_INCREMENTAL_BUILD: IncrementalBuild = { + priorWorkFor: () => null, + priorTypeCheckingResultsFor: () => null, }; diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts index f2497cbb87..1cb0745b05 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts @@ -8,8 +8,9 @@ import * as ts from 'typescript'; -import {absoluteFrom, AbsoluteFsPath} from '../../file_system'; +import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath} from '../../file_system'; import {ClassRecord, TraitCompiler} from '../../transform'; +import {FileTypeCheckingData} from '../../typecheck/src/context'; import {IncrementalBuild} from '../api'; import {FileDependencyGraph} from './dependency_tracking'; @@ -17,7 +18,7 @@ import {FileDependencyGraph} from './dependency_tracking'; /** * Drives an incremental build, by tracking changes and determining which files need to be emitted. */ -export class IncrementalDriver implements IncrementalBuild { +export class IncrementalDriver implements IncrementalBuild { /** * State of the current build. * @@ -190,10 +191,21 @@ export class IncrementalDriver implements IncrementalBuild { lastGood: { depGraph: this.depGraph, traitCompiler: traitCompiler, - } + typeCheckingResults: null, + }, + + priorTypeCheckingResults: + this.state.lastGood !== null ? this.state.lastGood.typeCheckingResults : null, }; } + recordSuccessfulTypeCheck(results: Map): void { + if (this.state.lastGood === null || this.state.kind !== BuildStateKind.Analyzed) { + return; + } + this.state.lastGood.typeCheckingResults = results; + } + recordSuccessfulEmit(sf: ts.SourceFile): void { this.state.pendingEmit.delete(sf.fileName); } @@ -214,6 +226,28 @@ export class IncrementalDriver implements IncrementalBuild { return this.state.lastGood.traitCompiler.recordsFor(sf); } } + + priorTypeCheckingResultsFor(sf: ts.SourceFile): FileTypeCheckingData|null { + if (this.state.kind !== BuildStateKind.Analyzed || + this.state.priorTypeCheckingResults === null || this.logicalChanges === null) { + return null; + } + + if (this.logicalChanges.has(sf.fileName)) { + return null; + } + + const fileName = absoluteFromSourceFile(sf); + if (!this.state.priorTypeCheckingResults.has(fileName)) { + return null; + } + const data = this.state.priorTypeCheckingResults.get(fileName)!; + if (data.hasInlines) { + return null; + } + + return data; + } } type BuildState = PendingBuildState|AnalyzedBuildState; @@ -236,7 +270,8 @@ interface BaseBuildState { * After analysis, it's updated to include any files which might have changed and need a re-emit * as a result of incremental changes. * - * If an emit happens, any written files are removed from the `Set`, as they're no longer pending. + * If an emit happens, any written files are removed from the `Set`, as they're no longer + * pending. * * Thus, after compilation `pendingEmit` should be empty (on a successful build) or contain the * files which still need to be emitted but have not yet been (due to errors). @@ -267,6 +302,11 @@ interface BaseBuildState { * This is used to extract "prior work" which might be reusable in this compilation. */ traitCompiler: TraitCompiler; + + /** + * Type checking results which will be passed onto the next build. + */ + typeCheckingResults: Map| null; }|null; } @@ -306,6 +346,11 @@ interface AnalyzedBuildState extends BaseBuildState { * analyzed build. */ pendingEmit: Set; + + /** + * Type checking results from the previous compilation, which can be reused in this one. + */ + priorTypeCheckingResults: Map|null; } function tsOnlyFiles(program: ts.Program): ReadonlyArray { diff --git a/packages/compiler-cli/src/ngtsc/shims/index.ts b/packages/compiler-cli/src/ngtsc/shims/index.ts index c81210ed8d..0090e22736 100644 --- a/packages/compiler-cli/src/ngtsc/shims/index.ts +++ b/packages/compiler-cli/src/ngtsc/shims/index.ts @@ -10,7 +10,7 @@ export {PerFileShimGenerator, TopLevelShimGenerator} from './api'; export {ShimAdapter} from './src/adapter'; -export {isShim} from './src/expando'; +export {copyFileShimData, isShim} from './src/expando'; export {FactoryGenerator, FactoryInfo, FactoryTracker, generatedFactoryTransform} from './src/factory_generator'; export {ShimReferenceTagger} from './src/reference_tagger'; export {SummaryGenerator} from './src/summary_generator'; diff --git a/packages/compiler-cli/src/ngtsc/shims/src/expando.ts b/packages/compiler-cli/src/ngtsc/shims/src/expando.ts index 173533c52c..1f5565d95a 100644 --- a/packages/compiler-cli/src/ngtsc/shims/src/expando.ts +++ b/packages/compiler-cli/src/ngtsc/shims/src/expando.ts @@ -100,3 +100,13 @@ export function isFileShimSourceFile(sf: ts.SourceFile): sf is NgFileShimSourceF export function isShim(sf: ts.SourceFile): boolean { return isExtended(sf) && (sf[NgExtension].fileShim !== null || sf[NgExtension].isTopLevelShim); } + +/** + * Copy any shim data from one `ts.SourceFile` to another. + */ +export function copyFileShimData(from: ts.SourceFile, to: ts.SourceFile): void { + if (!isFileShimSourceFile(from)) { + return; + } + sfExtensionData(to).fileShim = sfExtensionData(from).fileShim; +} diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index f28b7e2d74..f50b4b6af5 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -87,7 +87,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { constructor( private handlers: DecoratorHandler[], private reflector: ReflectionHost, private perf: PerfRecorder, - private incrementalBuild: IncrementalBuild, + private incrementalBuild: IncrementalBuild, private compileNonExportedClasses: boolean, private dtsTransforms: DtsTransformRegistry) { for (const handler of handlers) { this.handlersByName.set(handler.name, handler); @@ -423,8 +423,16 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { } } - typeCheck(ctx: TypeCheckContext): void { - for (const clazz of this.classes.keys()) { + /** + * Generate type-checking code into the `TypeCheckContext` for any components within the given + * `ts.SourceFile`. + */ + typeCheck(sf: ts.SourceFile, ctx: TypeCheckContext): void { + if (!this.fileToClasses.has(sf)) { + return; + } + + 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) { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel index 484b3aa6ee..349905994c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel @@ -11,6 +11,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/imports", + "//packages/compiler-cli/src/ngtsc/incremental:api", "//packages/compiler-cli/src/ngtsc/metadata", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/shims", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index 76d2026401..9ee90723a1 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -10,10 +10,12 @@ import * as ts from 'typescript'; import {absoluteFromSourceFile, AbsoluteFsPath, getSourceFileOrError} from '../../file_system'; import {ReferenceEmitter} from '../../imports'; +import {IncrementalBuild} from '../../incremental/api'; import {ReflectionHost} from '../../reflection'; +import {isShim} from '../../shims'; import {TypeCheckingConfig, TypeCheckingProgramStrategy, UpdateMode} from './api'; -import {FileTypeCheckingData, TypeCheckContext} from './context'; +import {FileTypeCheckingData, TypeCheckContext, TypeCheckRequest} from './context'; import {shouldReportDiagnostic, translateDiagnostic} from './diagnostics'; /** @@ -21,7 +23,7 @@ import {shouldReportDiagnostic, translateDiagnostic} from './diagnostics'; * `TypeCheckContext`. */ export interface ProgramTypeCheckAdapter { - typeCheck(ctx: TypeCheckContext): void; + typeCheck(sf: ts.SourceFile, ctx: TypeCheckContext): void; } /** @@ -36,26 +38,42 @@ export class TemplateTypeChecker { private originalProgram: ts.Program, private typeCheckingStrategy: TypeCheckingProgramStrategy, private typeCheckAdapter: ProgramTypeCheckAdapter, private config: TypeCheckingConfig, - private refEmitter: ReferenceEmitter, private reflector: ReflectionHost) {} + private refEmitter: ReferenceEmitter, private reflector: ReflectionHost, + private priorBuild: IncrementalBuild) {} /** * Reset the internal type-checking program by generating type-checking code from the user's * program. */ - refresh(): void { + refresh(): TypeCheckRequest { this.files.clear(); const ctx = new TypeCheckContext(this.config, this.originalProgram, this.refEmitter, this.reflector); // Typecheck all the files. - this.typeCheckAdapter.typeCheck(ctx); + for (const sf of this.originalProgram.getSourceFiles()) { + if (sf.isDeclarationFile || isShim(sf)) { + continue; + } + + const previousResults = this.priorBuild.priorTypeCheckingResultsFor(sf); + if (previousResults === null) { + // Previous results were not available, so generate new type-checking code for this file. + this.typeCheckAdapter.typeCheck(sf, ctx); + } else { + // Previous results were available, and can be adopted into the current build. + ctx.adoptPriorResults(sf, previousResults); + } + } const results = ctx.finalize(); this.typeCheckingStrategy.updateFiles(results.updates, UpdateMode.Complete); for (const [file, fileData] of results.perFileData) { - this.files.set(file, {...fileData}); + this.files.set(file, fileData); } + + return results; } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index d2ebe90531..2e92257e18 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -128,6 +128,22 @@ export class TypeCheckContext { */ private typeCtorPending = new Set(); + /** + * Map of data for file paths which was adopted from a prior compilation. + * + * This data allows the `TypeCheckContext` to generate a `TypeCheckRequest` which can interpret + * diagnostics from type-checking shims included in the prior compilation. + */ + private adoptedFiles = new Map(); + + /** + * Record the `FileTypeCheckingData` from a previous program that's associated with a particular + * source file. + */ + adoptPriorResults(sf: ts.SourceFile, data: FileTypeCheckingData): void { + this.adoptedFiles.set(absoluteFromSourceFile(sf), data); + } + /** * Record a template for the given component `node`, with a `SelectorMatcher` for directive * matching. @@ -274,6 +290,10 @@ export class TypeCheckContext { }); } + for (const [sfPath, fileData] of this.adoptedFiles.entries()) { + results.perFileData.set(sfPath, fileData); + } + return results; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts index b02b16f16d..71f83064c9 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts @@ -8,13 +8,7 @@ import * as ts from 'typescript'; -import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath, resolve} from '../../file_system'; -import {ShimAdapter, ShimReferenceTagger} from '../../shims'; - -import {TypeCheckContext} from './context'; -import {TypeCheckShimGenerator} from './shim'; - - +import {copyFileShimData, ShimReferenceTagger} from '../../shims'; /** * A `ts.CompilerHost` which augments source files with type checking code from a @@ -26,21 +20,16 @@ export class TypeCheckProgramHost implements ts.CompilerHost { */ private sfMap: Map; - // A special `ShimAdapter` is constructed to cause fresh type-checking shims to be generated for - // every input file in the program. - // - // tsRootFiles is explicitly passed empty here. This is okay because at type-checking time, shim - // root files have already been included in the ts.Program's root files by the `NgCompilerHost`'s - // `ShimAdapter`. - // - // The oldProgram is also explicitly not passed here, even though one exists. This is because: - // - ngfactory/ngsummary shims, if present, should be treated effectively as original files. As - // they are still marked as shims, they won't have a typecheck shim generated for them, but - // otherwise they should be reused as-is. - // - ngtypecheck shims are always generated as fresh, and not reused. - private shimAdapter = new ShimAdapter( - this.delegate, /* tsRootFiles */[], [], [new TypeCheckShimGenerator()], - /* oldProgram */ null); + /** + * The `ShimReferenceTagger` responsible for tagging `ts.SourceFile`s loaded via this host. + * + * The `TypeCheckProgramHost` is used in the creation of a new `ts.Program`. Even though this new + * program is based on a prior one, TypeScript will still start from the root files and enumerate + * all source files to include in the new program. This means that just like during the original + * program's creation, these source files must be tagged with references to per-file shims in + * order for those shims to be loaded, and then cleaned up afterwards. Thus the + * `TypeCheckProgramHost` has its own `ShimReferenceTagger` to perform this function. + */ private shimTagger = new ShimReferenceTagger(this.shimExtensionPrefixes); readonly resolveModuleNames?: ts.CompilerHost['resolveModuleNames']; @@ -63,25 +52,19 @@ export class TypeCheckProgramHost implements ts.CompilerHost { fileName: string, languageVersion: ts.ScriptTarget, onError?: ((message: string) => void)|undefined, shouldCreateNewSourceFile?: boolean|undefined): ts.SourceFile|undefined { - // Look in the cache for the source file. + const delegateSf = + this.delegate.getSourceFile(fileName, languageVersion, onError, shouldCreateNewSourceFile)!; + if (delegateSf === undefined) { + return undefined; + } + + // Look for replacements. let sf: ts.SourceFile; if (this.sfMap.has(fileName)) { sf = this.sfMap.get(fileName)!; + copyFileShimData(delegateSf, sf); } else { - const sfShim = this.shimAdapter.maybeGenerate(absoluteFrom(fileName)); - - if (sfShim === undefined) { - return undefined; - } else if (sfShim === null) { - const maybeSf = this.delegate.getSourceFile( - fileName, languageVersion, onError, shouldCreateNewSourceFile)!; - if (maybeSf === undefined) { - return undefined; - } - sf = maybeSf; - } else { - sf = sfShim; - } + sf = delegateSf; } // TypeScript doesn't allow returning redirect source files. To avoid unforseen errors we // return the original source file instead of the redirect target. diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/shim.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/shim.ts index bd6d308f39..99e570df82 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/shim.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/shim.ts @@ -22,7 +22,17 @@ export class TypeCheckShimGenerator implements PerFileShimGenerator { readonly extensionPrefix = 'ngtypecheck'; readonly shouldEmit = false; - generateShimForFile(sf: ts.SourceFile, genFilePath: AbsoluteFsPath): ts.SourceFile { + generateShimForFile( + sf: ts.SourceFile, genFilePath: AbsoluteFsPath, + priorShimSf: ts.SourceFile|null): ts.SourceFile { + if (priorShimSf !== null) { + // If this shim existed in the previous program, reuse it now. It might not be correct, but + // reusing it in the main program allows the shape of its imports to potentially remain the + // same and TS can then use the fastest path for incremental program creation. Later during + // the type-checking phase it's going to either be reused, or replaced anyways. Thus there's + // no harm in reuse here even if it's out of date. + return priorShimSf; + } return ts.createSourceFile( genFilePath, 'export const USED_FOR_NG_TYPE_CHECKING = true;', ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel index 8aebe02977..b9974dee6d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel @@ -14,6 +14,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/imports", + "//packages/compiler-cli/src/ngtsc/incremental", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/testing", "//packages/compiler-cli/src/ngtsc/typecheck", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index c91b7f9be3..1908f3f4c7 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -12,6 +12,7 @@ import * as ts from 'typescript'; import {absoluteFrom, AbsoluteFsPath, LogicalFileSystem} from '../../file_system'; import {TestFile} from '../../file_system/testing'; import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; +import {NOOP_INCREMENTAL_BUILD} from '../../incremental'; import {ClassDeclaration, isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection'; import {makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; @@ -301,14 +302,21 @@ export function typecheck( const programStrategy = new ReusedProgramStrategy(program, host, options, []); const templateTypeChecker = new TemplateTypeChecker( - program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost); + program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost, + NOOP_INCREMENTAL_BUILD); templateTypeChecker.refresh(); return templateTypeChecker.getDiagnosticsForFile(sf); } function createTypeCheckAdapter(fn: (ctx: TypeCheckContext) => void): ProgramTypeCheckAdapter { + let called = false; return { - typeCheck: fn, + typeCheck: (sf: ts.SourceFile, ctx: TypeCheckContext) => { + if (!called) { + fn(ctx); + } + called = true; + }, }; } diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index a3c97fac04..94dc9e59f7 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -70,6 +70,7 @@ runInEachFileSystem(() => { }); it('should rebuild components that have changed', () => { + env.tsconfig({strictTemplates: true}); env.write('component1.ts', ` import {Component} from '@angular/core'; @@ -471,6 +472,261 @@ runInEachFileSystem(() => { env.invalidateCachedFile('test.ts'); env.driveMain(); }); + + describe('template type-checking', () => { + beforeEach(() => { + env.tsconfig({strictTemplates: true}); + }); + + it('should repeat type errors across rebuilds, even if nothing has changed', () => { + // This test verifies that when a project is rebuilt multiple times with no changes, all + // template diagnostics are produced each time. Different types of errors are checked: + // - a general type error + // - an unmatched binding + // - a DOM schema error + env.write('component.ts', ` + import {Component} from '@angular/core'; + @Component({ + selector: 'test-cmp', + template: \` + {{notAProperty}} + +
+ \`, + }) + export class TestCmp {} + `); + + let diags = env.driveDiagnostics(); + // Should get a diagnostic for each line in the template. + expect(diags.length).toBe(3); + + // Now rebuild without any changes, and verify they're still produced. + diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + + // If it's worth testing, it's worth overtesting. + // + // Actually, the above only tests the transition from "initial" to "incremental" + // compilation. The next build verifies that an "incremental to incremental" transition + // preserves the diagnostics as well. + diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + }); + + it('should pick up errors caused by changing an unrelated interface', () => { + // The premise of this test is that `iface.ts` declares an interface which is used to type + // a property of a component. The interface is then changed in a subsequent compilation in + // a way that introduces a type error in the template. The test verifies the resulting + // diagnostic is produced. + env.write('iface.ts', ` + export interface SomeType { + field: string; + } + `); + env.write('component.ts', ` + import {Component} from '@angular/core'; + import {SomeType} from './iface'; + + @Component({ + selector: 'test-cmp', + template: '{{ doSomething(value.field) }}', + }) + export class TestCmp { + value!: SomeType; + // Takes a string value only. + doSomething(param: string): string { + return param; + } + } + `); + + expect(env.driveDiagnostics().length).toBe(0); + env.flushWrittenFileTracking(); + + // Change the interface. + env.write('iface.ts', ` + export interface SomeType { + field: number; + } + `); + + expect(env.driveDiagnostics().length).toBe(1); + }); + + it('should recompile when a remote change happens to a scope', () => { + // The premise of this test is that the component Cmp has a template error (a binding to an + // unknown property). Cmp is in ModuleA, which imports ModuleB, which declares Dir that has + // the property. Because ModuleB doesn't export Dir, it's not visible to Cmp - hence the + // error. + // In the test, during the incremental rebuild Dir is exported from ModuleB. This is a + // change to the scope of ModuleA made by changing ModuleB (hence, a "remote change"). The + // test verifies that incremental template type-checking. + env.write('cmp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '
', + }) + export class Cmp {} + `); + env.write('module-a.ts', ` + import {NgModule} from '@angular/core'; + import {Cmp} from './cmp'; + import {ModuleB} from './module-b'; + + @NgModule({ + declarations: [Cmp], + imports: [ModuleB], + }) + export class ModuleA {} + `); + + // Declare ModuleB and a directive Dir, but ModuleB does not yet export Dir. + env.write('module-b.ts', ` + import {NgModule} from '@angular/core'; + import {Dir} from './dir'; + + @NgModule({ + declarations: [Dir], + }) + export class ModuleB {} + `); + env.write('dir.ts', ` + import {Directive, Input} from '@angular/core'; + + @Directive({selector: '[dir]'}) + export class Dir { + @Input() someInput!: any; + } + `); + + let diags = env.driveDiagnostics(); + // Should get a diagnostic about [dir] not being a valid binding. + expect(diags.length).toBe(1); + + + // As a precautionary check, run the build a second time with no changes, to ensure the + // diagnostic is repeated. + diags = env.driveDiagnostics(); + // Should get a diagnostic about [dir] not being a valid binding. + expect(diags.length).toBe(1); + + // Modify ModuleB to now export Dir. + env.write('module-b.ts', ` + import {NgModule} from '@angular/core'; + import {Dir} from './dir'; + + @NgModule({ + declarations: [Dir], + exports: [Dir], + }) + export class ModuleB {} + `); + + diags = env.driveDiagnostics(); + // Diagnostic should be gone, as the error has been fixed. + expect(diags.length).toBe(0); + }); + + describe('inline operations', () => { + it('should still pick up on errors from inlined type check blocks', () => { + // In certain cases the template type-checker has to inline type checking blocks into user + // code, instead of placing it in a parallel template type-checking file. In these cases + // incremental checking cannot be used, and the type-checking code must be regenerated on + // each build. This test verifies that the above mechanism works properly, by performing + // type-checking on an unexported class (not exporting the class forces the inline + // checking de-optimization). + env.write('cmp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '{{test}}', + }) + class Cmp {} + `); + + // On the first compilation, an error should be produced. + let diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + + // Next, two more builds are run, one with no changes made to the file, and the other with + // changes made that should remove the error. + + // The error should still be present when rebuilding. + diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + + // Now, correct the error by adding the 'test' property to the component. + env.write('cmp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '{{test}}', + }) + class Cmp { + test!: string; + } + `); + + env.driveMain(); + + // The error should be gone. + diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should still pick up on errors caused by inlined type constructors', () => { + // In certain cases the template type-checker cannot generate a type constructor for a + // directive within the template type-checking file which requires it, but must inline the + // type constructor within its original source file. In these cases, template type + // checking cannot be performed incrementally. This test verifies that such cases still + // work correctly, by repeatedly performing diagnostics on a component which depends on a + // directive with an inlined type constructor. + env.write('dir.ts', ` + import {Directive, Input} from '@angular/core'; + export interface Keys { + alpha: string; + beta: string; + } + @Directive({ + selector: '[dir]' + }) + export class Dir { + // The use of 'keyof' in the generic bound causes a deopt to an inline type + // constructor. + @Input() dir: T; + } + `); + + env.write('cmp.ts', ` + import {Component, NgModule} from '@angular/core'; + import {Dir} from './dir'; + @Component({ + selector: 'test-cmp', + template: '
', + }) + export class Cmp {} + @NgModule({ + declarations: [Cmp, Dir], + }) + export class Module {} + `); + + let diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('"alpha" | "beta"'); + + // On a rebuild, the same errors should be present. + diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('"alpha" | "beta"'); + }); + }); + }); }); function setupFooBarProgram(env: NgtscTestEnvironment) {