From 5103d908c89481d7b8a9f9a1cab2b996d86bc6a3 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 19 Jun 2020 12:55:13 -0700 Subject: [PATCH] perf(compiler-cli): fix regressions in incremental program reuse (#37641) Commit 4213e8d5 introduced shim reference tagging into the compiler, and changed how the `TypeCheckProgramHost` worked under the hood during the creation of a template type-checking program. This work enabled a more incremental flow for template type-checking, but unintentionally introduced several regressions in performance, caused by poor incrementality during `ts.Program` creation. 1. The `TypeCheckProgramHost` was made to rely on the `ts.CompilerHost` to retrieve instances of `ts.SourceFile`s from the original program. If the host does not return the original instance of such files, but instead creates new instances, this has two negative effects: it incurs additional parsing time, and it interferes with TypeScript's ability to reuse information about such files. 2. During the incremental creation of a `ts.Program`, TypeScript compares the `referencedFiles` of `ts.SourceFile` instances from the old program with those in the new program. If these arrays differ, TypeScript cannot fully reuse the old program. The implementation of reference tagging introduced in 4213e8d5 restores the original `referencedFiles` array after a `ts.Program` is created, which means that future incremental operations involving that program will always fail this comparison, effectively limiting the incrementality TypeScript can achieve. Problem 1 exacerbates problem 2: if a new `ts.SourceFile` is created by the host after shim generation has been disabled, it will have an untagged `referencedFiles` array even if the original file's `referencedFiles` was not restored, triggering problem 2 when creating the template type-checking program. To fix these issues, `referencedFiles` arrays are now restored on the old `ts.Program` prior to the creation of a new incremental program. This allows TypeScript to get the most out of reusing the old program's data. Additionally, the `TypeCheckProgramHost` now uses the original `ts.Program` to retrieve original instances of `ts.SourceFile`s where possible, preventing issues when a host would otherwise return fresh instances. Together, these fixes ensure that program reuse is as incremental as possible, and tests have been added to verify this for certain scenarios. An optimization was further added to prevent the creation of a type-checking `ts.Program` in the first place if no type-checking is necessary. PR Close #37641 --- packages/compiler-cli/BUILD.bazel | 1 + packages/compiler-cli/src/ngtsc/program.ts | 20 +++- .../compiler-cli/src/ngtsc/shims/index.ts | 2 +- .../src/ngtsc/shims/src/adapter.ts | 2 +- .../src/ngtsc/shims/src/expando.ts | 60 +++++++++++ .../src/ngtsc/shims/src/reference_tagger.ts | 30 +++--- .../ngtsc/shims/test/reference_tagger_spec.ts | 85 +++++++++------ .../compiler-cli/src/ngtsc/testing/index.ts | 2 +- .../src/ngtsc/testing/src/utils.ts | 17 +++ packages/compiler-cli/src/ngtsc/tsc_plugin.ts | 6 ++ .../ngtsc/typecheck/src/augmented_program.ts | 33 +++++- .../src/ngtsc/typecheck/src/host.ts | 15 ++- .../src/ngtsc/typecheck/test/BUILD.bazel | 1 + .../src/ngtsc/typecheck/test/program_spec.ts | 98 +++++++++++++++++ .../src/ngtsc/typecheck/test/test_utils.ts | 101 +++++++++++------- packages/compiler-cli/test/ngtsc/BUILD.bazel | 1 + packages/compiler-cli/test/ngtsc/env.ts | 7 ++ .../test/ngtsc/template_typecheck_spec.ts | 29 +++-- 18 files changed, 400 insertions(+), 110 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts diff --git a/packages/compiler-cli/BUILD.bazel b/packages/compiler-cli/BUILD.bazel index 773af85c62..cb3c783dd7 100644 --- a/packages/compiler-cli/BUILD.bazel +++ b/packages/compiler-cli/BUILD.bazel @@ -31,6 +31,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/indexer", "//packages/compiler-cli/src/ngtsc/perf", "//packages/compiler-cli/src/ngtsc/reflection", + "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/typecheck", "@npm//@bazel/typescript", "@npm//@types/node", diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index d1e3d33885..bf81c77638 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -17,6 +17,7 @@ import {NgCompilerOptions} from './core/api'; import {TrackedIncrementalBuildStrategy} from './incremental'; import {IndexedComponent} from './indexer'; import {NOOP_PERF_RECORDER, PerfRecorder, PerfTracker} from './perf'; +import {retagAllTsFiles, untagAllTsFiles} from './shims'; import {ReusedProgramStrategy} from './typecheck'; @@ -68,14 +69,26 @@ export class NgtscProgram implements api.Program { } this.closureCompilerEnabled = !!options.annotateForClosureCompiler; - const reuseProgram = oldProgram && oldProgram.reuseTsProgram; + const reuseProgram = oldProgram?.reuseTsProgram; this.host = NgCompilerHost.wrap(delegateHost, rootNames, options, reuseProgram ?? null); + if (reuseProgram !== undefined) { + // Prior to reusing the old program, restore shim tagging for all its `ts.SourceFile`s. + // TypeScript checks the `referencedFiles` of `ts.SourceFile`s for changes when evaluating + // incremental reuse of data from the old program, so it's important that these match in order + // to get the most benefit out of reuse. + retagAllTsFiles(reuseProgram); + } + this.tsProgram = ts.createProgram(this.host.inputFiles, options, this.host, reuseProgram); this.reuseTsProgram = this.tsProgram; this.host.postProgramCreationCleanup(); + // Shim tagging has served its purpose, and tags can now be removed from all `ts.SourceFile`s in + // the program. + untagAllTsFiles(this.tsProgram); + const reusedProgramStrategy = new ReusedProgramStrategy( this.tsProgram, this.host, this.options, this.host.shimExtensionPrefixes); @@ -93,6 +106,10 @@ export class NgtscProgram implements api.Program { return this.tsProgram; } + getReuseTsProgram(): ts.Program { + return this.reuseTsProgram; + } + getTsOptionDiagnostics(cancellationToken?: ts.CancellationToken| undefined): readonly ts.Diagnostic[] { return this.tsProgram.getOptionsDiagnostics(cancellationToken); @@ -248,6 +265,7 @@ export class NgtscProgram implements api.Program { })); this.perfRecorder.stop(fileEmitSpan); } + this.perfRecorder.stop(emitSpan); if (this.perfTracker !== null && this.options.tracePerformance !== undefined) { diff --git a/packages/compiler-cli/src/ngtsc/shims/index.ts b/packages/compiler-cli/src/ngtsc/shims/index.ts index d897845c4e..9cc388f5b7 100644 --- a/packages/compiler-cli/src/ngtsc/shims/index.ts +++ b/packages/compiler-cli/src/ngtsc/shims/index.ts @@ -9,7 +9,7 @@ /// export {ShimAdapter} from './src/adapter'; -export {copyFileShimData, isShim} from './src/expando'; +export {copyFileShimData, isShim, retagAllTsFiles, retagTsFile, sfExtensionData, untagAllTsFiles, untagTsFile} from './src/expando'; export {FactoryGenerator, 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/adapter.ts b/packages/compiler-cli/src/ngtsc/shims/src/adapter.ts index f07d8b6ea6..ab4494568d 100644 --- a/packages/compiler-cli/src/ngtsc/shims/src/adapter.ts +++ b/packages/compiler-cli/src/ngtsc/shims/src/adapter.ts @@ -12,7 +12,7 @@ import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath} from '../../file_s import {isDtsPath} from '../../util/src/typescript'; import {PerFileShimGenerator, TopLevelShimGenerator} from '../api'; -import {isFileShimSourceFile, isShim, NgExtension, sfExtensionData} from './expando'; +import {isFileShimSourceFile, isShim, sfExtensionData} from './expando'; import {makeShimFileName} from './util'; interface ShimGeneratorData { diff --git a/packages/compiler-cli/src/ngtsc/shims/src/expando.ts b/packages/compiler-cli/src/ngtsc/shims/src/expando.ts index 71b3c81e6f..3d03ce42ba 100644 --- a/packages/compiler-cli/src/ngtsc/shims/src/expando.ts +++ b/packages/compiler-cli/src/ngtsc/shims/src/expando.ts @@ -21,7 +21,16 @@ export const NgExtension = Symbol('NgExtension'); export interface NgExtensionData { isTopLevelShim: boolean; fileShim: NgFileShimData|null; + + /** + * The contents of the `referencedFiles` array, before modification by a `ShimReferenceTagger`. + */ originalReferencedFiles: ReadonlyArray|null; + + /** + * The contents of the `referencedFiles` array, after modification by a `ShimReferenceTagger`. + */ + taggedReferenceFiles: ReadonlyArray|null; } /** @@ -65,6 +74,7 @@ export function sfExtensionData(sf: ts.SourceFile): NgExtensionData { isTopLevelShim: false, fileShim: null, originalReferencedFiles: null, + taggedReferenceFiles: null, }; extSf[NgExtension] = extension; return extension; @@ -110,3 +120,53 @@ export function copyFileShimData(from: ts.SourceFile, to: ts.SourceFile): void { } sfExtensionData(to).fileShim = sfExtensionData(from).fileShim; } + +/** + * For those `ts.SourceFile`s in the `program` which have previously been tagged by a + * `ShimReferenceTagger`, restore the original `referencedFiles` array that does not have shim tags. + */ +export function untagAllTsFiles(program: ts.Program): void { + for (const sf of program.getSourceFiles()) { + untagTsFile(sf); + } +} + +/** + * For those `ts.SourceFile`s in the `program` which have previously been tagged by a + * `ShimReferenceTagger`, re-apply the effects of tagging by updating the `referencedFiles` array to + * the tagged version produced previously. + */ +export function retagAllTsFiles(program: ts.Program): void { + for (const sf of program.getSourceFiles()) { + retagTsFile(sf); + } +} + +/** + * Restore the original `referencedFiles` for the given `ts.SourceFile`. + */ +export function untagTsFile(sf: ts.SourceFile): void { + if (sf.isDeclarationFile || !isExtended(sf)) { + return; + } + + const ext = sfExtensionData(sf); + if (ext.originalReferencedFiles !== null) { + sf.referencedFiles = ext.originalReferencedFiles as Array; + } +} + +/** + * Apply the previously tagged `referencedFiles` to the given `ts.SourceFile`, if it was previously + * tagged. + */ +export function retagTsFile(sf: ts.SourceFile): void { + if (sf.isDeclarationFile || !isExtended(sf)) { + return; + } + + const ext = sfExtensionData(sf); + if (ext.taggedReferenceFiles !== null) { + sf.referencedFiles = ext.taggedReferenceFiles as Array; + } +} diff --git a/packages/compiler-cli/src/ngtsc/shims/src/reference_tagger.ts b/packages/compiler-cli/src/ngtsc/shims/src/reference_tagger.ts index 17899251d4..3e0a3fe428 100644 --- a/packages/compiler-cli/src/ngtsc/shims/src/reference_tagger.ts +++ b/packages/compiler-cli/src/ngtsc/shims/src/reference_tagger.ts @@ -8,10 +8,10 @@ import * as ts from 'typescript'; -import {absoluteFrom, absoluteFromSourceFile} from '../../file_system'; +import {absoluteFromSourceFile} from '../../file_system'; import {isNonDeclarationTsPath} from '../../util/src/typescript'; -import {isExtended as isExtendedSf, isShim, NgExtension, sfExtensionData} from './expando'; +import {isShim, sfExtensionData} from './expando'; import {makeShimFileName} from './util'; /** @@ -48,8 +48,16 @@ export class ShimReferenceTagger { return; } - sfExtensionData(sf).originalReferencedFiles = sf.referencedFiles; - const referencedFiles = [...sf.referencedFiles]; + const ext = sfExtensionData(sf); + + // If this file has never been tagged before, capture its `referencedFiles` in the extension + // data. + if (ext.originalReferencedFiles === null) { + ext.originalReferencedFiles = sf.referencedFiles; + } + + const referencedFiles = [...ext.originalReferencedFiles]; + const sfPath = absoluteFromSourceFile(sf); for (const suffix of this.suffixes) { @@ -60,26 +68,16 @@ export class ShimReferenceTagger { }); } + ext.taggedReferenceFiles = referencedFiles; sf.referencedFiles = referencedFiles; this.tagged.add(sf); } /** - * Restore the original `referencedFiles` values of all tagged `ts.SourceFile`s and disable the - * `ShimReferenceTagger`. + * Disable the `ShimReferenceTagger` and free memory associated with tracking tagged files. */ finalize(): void { this.enabled = false; - for (const sf of this.tagged) { - if (!isExtendedSf(sf)) { - continue; - } - - const extensionData = sfExtensionData(sf); - if (extensionData.originalReferencedFiles !== null) { - sf.referencedFiles = extensionData.originalReferencedFiles! as ts.FileReference[]; - } - } this.tagged.clear(); } } diff --git a/packages/compiler-cli/src/ngtsc/shims/test/reference_tagger_spec.ts b/packages/compiler-cli/src/ngtsc/shims/test/reference_tagger_spec.ts index 6bf2a600b0..807d0996f9 100644 --- a/packages/compiler-cli/src/ngtsc/shims/test/reference_tagger_spec.ts +++ b/packages/compiler-cli/src/ngtsc/shims/test/reference_tagger_spec.ts @@ -12,6 +12,7 @@ import {absoluteFrom as _, AbsoluteFsPath, getSourceFileOrError} from '../../fil import {runInEachFileSystem} from '../../file_system/testing'; import {makeProgram} from '../../testing'; import {ShimAdapter} from '../src/adapter'; +import {retagTsFile, untagTsFile} from '../src/expando'; import {ShimReferenceTagger} from '../src/reference_tagger'; import {TestShimGenerator} from './util'; @@ -67,40 +68,6 @@ runInEachFileSystem(() => { expect(shimSf.referencedFiles).toEqual([]); }); - it('should remove tags during finalization', () => { - const tagger = new ShimReferenceTagger(['test1', 'test2']); - - const fileName = _('/file.ts'); - const sf = makeArbitrarySf(fileName); - - expectReferencedFiles(sf, []); - - tagger.tag(sf); - expectReferencedFiles(sf, ['/file.test1.ts', '/file.test2.ts']); - - tagger.finalize(); - expectReferencedFiles(sf, []); - }); - - it('should not remove references it did not add during finalization', () => { - const tagger = new ShimReferenceTagger(['test1', 'test2']); - const fileName = _('/file.ts'); - const libFileName = _('/lib.d.ts'); - - const sf = makeSf(fileName, ` - /// - export const UNIMPORTANT = true; - `); - - expectReferencedFiles(sf, [libFileName]); - - tagger.tag(sf); - expectReferencedFiles(sf, ['/file.test1.ts', '/file.test2.ts', libFileName]); - - tagger.finalize(); - expectReferencedFiles(sf, [libFileName]); - }); - it('should not tag shims after finalization', () => { const tagger = new ShimReferenceTagger(['test1', 'test2']); tagger.finalize(); @@ -111,6 +78,56 @@ runInEachFileSystem(() => { tagger.tag(sf); expectReferencedFiles(sf, []); }); + + it('should not overwrite original referencedFiles', () => { + const tagger = new ShimReferenceTagger(['test']); + + const fileName = _('/file.ts'); + const sf = makeArbitrarySf(fileName); + sf.referencedFiles = [{ + fileName: _('/other.ts'), + pos: 0, + end: 0, + }]; + + tagger.tag(sf); + expectReferencedFiles(sf, ['/other.ts', '/file.test.ts']); + }); + + it('should always tag against the original referencedFiles', () => { + const tagger1 = new ShimReferenceTagger(['test1']); + const tagger2 = new ShimReferenceTagger(['test2']); + + const fileName = _('/file.ts'); + const sf = makeArbitrarySf(fileName); + + tagger1.tag(sf); + tagger2.tag(sf); + expectReferencedFiles(sf, ['/file.test2.ts']); + }); + + describe('tagging and untagging', () => { + it('should be able to untag references and retag them later', () => { + const tagger = new ShimReferenceTagger(['test']); + + const fileName = _('/file.ts'); + const sf = makeArbitrarySf(fileName); + sf.referencedFiles = [{ + fileName: _('/other.ts'), + pos: 0, + end: 0, + }]; + + tagger.tag(sf); + expectReferencedFiles(sf, ['/other.ts', '/file.test.ts']); + + untagTsFile(sf); + expectReferencedFiles(sf, ['/other.ts']); + + retagTsFile(sf); + expectReferencedFiles(sf, ['/other.ts', '/file.test.ts']); + }); + }); }); }); diff --git a/packages/compiler-cli/src/ngtsc/testing/index.ts b/packages/compiler-cli/src/ngtsc/testing/index.ts index 6e6da32033..354d8a3876 100644 --- a/packages/compiler-cli/src/ngtsc/testing/index.ts +++ b/packages/compiler-cli/src/ngtsc/testing/index.ts @@ -5,4 +5,4 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -export {getDeclaration, makeProgram} from './src/utils'; +export {expectCompleteReuse, getDeclaration, makeProgram} from './src/utils'; diff --git a/packages/compiler-cli/src/ngtsc/testing/src/utils.ts b/packages/compiler-cli/src/ngtsc/testing/src/utils.ts index 2b56b0a877..f2c84d263d 100644 --- a/packages/compiler-cli/src/ngtsc/testing/src/utils.ts +++ b/packages/compiler-cli/src/ngtsc/testing/src/utils.ts @@ -97,6 +97,23 @@ export function walkForDeclaration(name: string, rootNode: ts.Node): ts.Declarat return chosenDecl; } +const COMPLETE_REUSE_FAILURE_MESSAGE = + 'The original program was not reused completely, even though no changes should have been made to its structure'; + +/** + * Extracted from TypeScript's internal enum `StructureIsReused`. + */ +enum TsStructureIsReused { + Not = 0, + SafeModules = 1, + Completely = 2, +} + +export function expectCompleteReuse(oldProgram: ts.Program): void { + // Assert complete reuse using TypeScript's private API. + expect((oldProgram as any).structureIsReused) + .toBe(TsStructureIsReused.Completely, COMPLETE_REUSE_FAILURE_MESSAGE); +} function bindingNameEquals(node: ts.BindingName, name: string): boolean { if (ts.isIdentifier(node)) { diff --git a/packages/compiler-cli/src/ngtsc/tsc_plugin.ts b/packages/compiler-cli/src/ngtsc/tsc_plugin.ts index 0afc0a168b..7c87be7f22 100644 --- a/packages/compiler-cli/src/ngtsc/tsc_plugin.ts +++ b/packages/compiler-cli/src/ngtsc/tsc_plugin.ts @@ -13,6 +13,7 @@ import {NgCompilerOptions, UnifiedModulesHost} from './core/api'; import {NodeJSFileSystem, setFileSystem} from './file_system'; import {PatchedProgramIncrementalBuildStrategy} from './incremental'; import {NOOP_PERF_RECORDER} from './perf'; +import {untagAllTsFiles} from './shims'; import {ReusedProgramStrategy} from './typecheck/src/augmented_program'; // The following is needed to fix a the chicken-and-egg issue where the sync (into g3) script will @@ -80,6 +81,9 @@ export class NgTscPlugin implements TscPlugin { wrapHost( host: ts.CompilerHost&UnifiedModulesHost, inputFiles: readonly string[], options: ts.CompilerOptions): PluginCompilerHost { + // TODO(alxhub): Eventually the `wrapHost()` API will accept the old `ts.Program` (if one is + // available). When it does, its `ts.SourceFile`s need to be re-tagged to enable proper + // incremental compilation. this.options = {...this.ngOptions, ...options} as NgCompilerOptions; this.host = NgCompilerHost.wrap(host, inputFiles, this.options, /* oldProgram */ null); return this.host; @@ -92,6 +96,8 @@ export class NgTscPlugin implements TscPlugin { if (this.host === null || this.options === null) { throw new Error('Lifecycle error: setupCompilation() before wrapHost().'); } + this.host.postProgramCreationCleanup(); + untagAllTsFiles(program); const typeCheckStrategy = new ReusedProgramStrategy( program, this.host, this.options, this.host.shimExtensionPrefixes); this._compiler = new NgCompiler( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts index ce17ab66d1..c3c1f73987 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts @@ -9,6 +9,7 @@ import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../file_system'; +import {retagAllTsFiles, untagAllTsFiles} from '../../shims'; import {TypeCheckingProgramStrategy, UpdateMode} from './api'; import {TypeCheckProgramHost} from './host'; @@ -26,8 +27,10 @@ export class ReusedProgramStrategy implements TypeCheckingProgramStrategy { */ private sfMap = new Map(); + private program: ts.Program = this.originalProgram; + constructor( - private program: ts.Program, private originalHost: ts.CompilerHost, + private originalProgram: ts.Program, private originalHost: ts.CompilerHost, private options: ts.CompilerOptions, private shimExtensionPrefixes: string[]) {} getProgram(): ts.Program { @@ -35,6 +38,17 @@ export class ReusedProgramStrategy implements TypeCheckingProgramStrategy { } updateFiles(contents: Map, updateMode: UpdateMode): void { + if (contents.size === 0) { + // No changes have been requested. Is it safe to skip updating entirely? + // If UpdateMode is Incremental, then yes. If UpdateMode is Complete, then it's safe to skip + // only if there are no active changes already (that would be cleared by the update). + + if (updateMode !== UpdateMode.Complete || this.sfMap.size === 0) { + // No changes would be made to the `ts.Program` anyway, so it's safe to do nothing here. + return; + } + } + if (updateMode === UpdateMode.Complete) { this.sfMap.clear(); } @@ -43,14 +57,25 @@ export class ReusedProgramStrategy implements TypeCheckingProgramStrategy { this.sfMap.set(filePath, ts.createSourceFile(filePath, text, ts.ScriptTarget.Latest, true)); } - const host = - new TypeCheckProgramHost(this.sfMap, this.originalHost, this.shimExtensionPrefixes); + const host = new TypeCheckProgramHost( + this.sfMap, this.originalProgram, this.originalHost, this.shimExtensionPrefixes); + const oldProgram = this.program; + + // Retag the old program's `ts.SourceFile`s with shim tags, to allow TypeScript to reuse the + // most data. + retagAllTsFiles(oldProgram); + this.program = ts.createProgram({ host, rootNames: this.program.getRootFileNames(), options: this.options, - oldProgram: this.program, + oldProgram, }); host.postProgramCreationCleanup(); + + // And untag them afterwards. We explicitly untag both programs here, because the oldProgram + // may still be used for emit and needs to not contain tags. + untagAllTsFiles(this.program); + untagAllTsFiles(oldProgram); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts index 8fb081415a..57a74db930 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts @@ -35,8 +35,8 @@ export class TypeCheckProgramHost implements ts.CompilerHost { readonly resolveModuleNames?: ts.CompilerHost['resolveModuleNames']; constructor( - sfMap: Map, private delegate: ts.CompilerHost, - private shimExtensionPrefixes: string[]) { + sfMap: Map, private originalProgram: ts.Program, + private delegate: ts.CompilerHost, private shimExtensionPrefixes: string[]) { this.sfMap = sfMap; if (delegate.getDirectories !== undefined) { @@ -52,8 +52,15 @@ export class TypeCheckProgramHost implements ts.CompilerHost { fileName: string, languageVersion: ts.ScriptTarget, onError?: ((message: string) => void)|undefined, shouldCreateNewSourceFile?: boolean|undefined): ts.SourceFile|undefined { - const delegateSf = - this.delegate.getSourceFile(fileName, languageVersion, onError, shouldCreateNewSourceFile)!; + // Try to use the same `ts.SourceFile` as the original program, if possible. This guarantees + // that program reuse will be as efficient as possible. + let delegateSf: ts.SourceFile|undefined = this.originalProgram.getSourceFile(fileName); + if (delegateSf === undefined) { + // Something went wrong and a source file is being requested that's not in the original + // program. Just in case, try to retrieve it from the delegate. + delegateSf = this.delegate.getSourceFile( + fileName, languageVersion, onError, shouldCreateNewSourceFile)!; + } if (delegateSf === undefined) { return undefined; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel index b9974dee6d..ce34bc6aa5 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel @@ -16,6 +16,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/incremental", "//packages/compiler-cli/src/ngtsc/reflection", + "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/testing", "//packages/compiler-cli/src/ngtsc/typecheck", "//packages/compiler-cli/src/ngtsc/util", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts new file mode 100644 index 0000000000..1d14e84ea6 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts @@ -0,0 +1,98 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import * as ts from 'typescript'; + +import {absoluteFrom, AbsoluteFsPath, getSourceFileOrError} from '../../file_system'; +import {runInEachFileSystem} from '../../file_system/testing'; +import {sfExtensionData, ShimReferenceTagger} from '../../shims'; +import {expectCompleteReuse, makeProgram} from '../../testing'; +import {UpdateMode} from '../src/api'; +import {ReusedProgramStrategy} from '../src/augmented_program'; + +import {createProgramWithNoTemplates} from './test_utils'; + +runInEachFileSystem(() => { + describe('template type-checking program', () => { + it('should not be created if no components need to be checked', () => { + const {program, templateTypeChecker, programStrategy} = createProgramWithNoTemplates(); + templateTypeChecker.refresh(); + // expect() here would create a really long error message, so this is checked manually. + if (programStrategy.getProgram() !== program) { + fail('Template type-checking created a new ts.Program even though it had no changes.'); + } + }); + + it('should have complete reuse if no structural changes are made to shims', () => { + const {program, host, options, typecheckPath} = makeSingleFileProgramWithTypecheckShim(); + const programStrategy = new ReusedProgramStrategy(program, host, options, ['ngtypecheck']); + + // Update /main.ngtypecheck.ts without changing its shape. Verify that the old program was + // reused completely. + programStrategy.updateFiles( + new Map([[typecheckPath, 'export const VERSION = 2;']]), UpdateMode.Complete); + + expectCompleteReuse(program); + }); + + it('should have complete reuse if no structural changes are made to input files', () => { + const {program, host, options, mainPath} = makeSingleFileProgramWithTypecheckShim(); + const programStrategy = new ReusedProgramStrategy(program, host, options, ['ngtypecheck']); + + // Update /main.ts without changing its shape. Verify that the old program was reused + // completely. + programStrategy.updateFiles( + new Map([[mainPath, 'export const STILL_NOT_A_COMPONENT = true;']]), UpdateMode.Complete); + + expectCompleteReuse(program); + }); + }); +}); + +function makeSingleFileProgramWithTypecheckShim(): { + program: ts.Program, + host: ts.CompilerHost, + options: ts.CompilerOptions, + mainPath: AbsoluteFsPath, + typecheckPath: AbsoluteFsPath, +} { + const mainPath = absoluteFrom('/main.ts'); + const typecheckPath = absoluteFrom('/main.ngtypecheck.ts'); + const {program, host, options} = makeProgram([ + { + name: mainPath, + contents: 'export const NOT_A_COMPONENT = true;', + }, + { + name: typecheckPath, + contents: 'export const VERSION = 1;', + } + ]); + + const sf = getSourceFileOrError(program, mainPath); + const typecheckSf = getSourceFileOrError(program, typecheckPath); + + // To ensure this test is validating the correct behavior, the initial conditions of the + // input program must be such that: + // + // 1) /main.ts was previously tagged with a reference to its ngtypecheck shim. + // 2) /main.ngtypecheck.ts is marked as a shim itself. + + // Condition 1: + const tagger = new ShimReferenceTagger(['ngtypecheck']); + tagger.tag(sf); + tagger.finalize(); + + // Condition 2: + sfExtensionData(typecheckSf).fileShim = { + extension: 'ngtypecheck', + generatedFrom: mainPath, + }; + + return {program, host, options, mainPath, typecheckPath}; +} 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 364c6b9c04..cdc76566f0 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -235,10 +235,18 @@ export function tcb( return res.replace(/\s+/g, ' '); } -export function typecheck( - template: string, source: string, declarations: TestDeclaration[] = [], - additionalSources: {name: AbsoluteFsPath; contents: string}[] = [], - config: Partial = {}, opts: ts.CompilerOptions = {}): ts.Diagnostic[] { +export interface TemplateTestEnvironment { + sf: ts.SourceFile; + program: ts.Program; + templateTypeChecker: TemplateTypeChecker; + programStrategy: ReusedProgramStrategy; +} + +function setupTemplateTypeChecking( + source: string, additionalSources: {name: AbsoluteFsPath; contents: string}[], + config: Partial, opts: ts.CompilerOptions, + makeTypeCheckAdapterFn: (program: ts.Program, sf: ts.SourceFile) => + ProgramTypeCheckAdapter): TemplateTestEnvironment { const typeCheckFilePath = absoluteFrom('/main.ngtypecheck.ts'); const files = [ typescriptLibDts(), @@ -266,48 +274,65 @@ export function typecheck( ]); const fullConfig = {...ALL_ENABLED_CONFIG, ...config}; - const templateUrl = 'synthetic.html'; - const templateFile = new ParseSourceFile(template, templateUrl); - const {nodes, errors} = parseTemplate(template, templateUrl); - if (errors !== undefined) { - throw new Error('Template parse errors: \n' + errors.join('\n')); - } - - const {matcher, pipes} = prepareDeclarations(declarations, decl => { - let declFile = sf; - if (decl.file !== undefined) { - declFile = program.getSourceFile(decl.file)!; - if (declFile === undefined) { - throw new Error(`Unable to locate ${decl.file} for ${decl.type} ${decl.name}`); - } - } - return getClass(declFile, decl.name); - }); - const binder = new R3TargetBinder(matcher); - const boundTarget = binder.bind({template: nodes}); - const clazz = new Reference(getClass(sf, 'TestComponent')); - - const sourceMapping: TemplateSourceMapping = { - type: 'external', - template, - templateUrl, - componentClass: clazz.node, - // Use the class's name for error mappings. - node: clazz.node.name, - }; - - const checkAdapter = createTypeCheckAdapter((ctx: TypeCheckContext) => { - ctx.addTemplate(clazz, boundTarget, pipes, [], sourceMapping, templateFile); - }); - + const checkAdapter = makeTypeCheckAdapterFn(program, sf); const programStrategy = new ReusedProgramStrategy(program, host, options, []); const templateTypeChecker = new TemplateTypeChecker( program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost, host, NOOP_INCREMENTAL_BUILD); + + return {program, sf, templateTypeChecker, programStrategy}; +} + +export function typecheck( + template: string, source: string, declarations: TestDeclaration[] = [], + additionalSources: {name: AbsoluteFsPath; contents: string}[] = [], + config: Partial = {}, opts: ts.CompilerOptions = {}): ts.Diagnostic[] { + const {sf, templateTypeChecker} = + setupTemplateTypeChecking(source, additionalSources, config, opts, (program, sf) => { + const templateUrl = 'synthetic.html'; + const templateFile = new ParseSourceFile(template, templateUrl); + const {nodes, errors} = parseTemplate(template, templateUrl); + if (errors !== undefined) { + throw new Error('Template parse errors: \n' + errors.join('\n')); + } + + const {matcher, pipes} = prepareDeclarations(declarations, decl => { + let declFile = sf; + if (decl.file !== undefined) { + declFile = program.getSourceFile(decl.file)!; + if (declFile === undefined) { + throw new Error(`Unable to locate ${decl.file} for ${decl.type} ${decl.name}`); + } + } + return getClass(declFile, decl.name); + }); + const binder = new R3TargetBinder(matcher); + const boundTarget = binder.bind({template: nodes}); + const clazz = new Reference(getClass(sf, 'TestComponent')); + + const sourceMapping: TemplateSourceMapping = { + type: 'external', + template, + templateUrl, + componentClass: clazz.node, + // Use the class's name for error mappings. + node: clazz.node.name, + }; + + return createTypeCheckAdapter((ctx: TypeCheckContext) => { + ctx.addTemplate(clazz, boundTarget, pipes, [], sourceMapping, templateFile); + }); + }); + templateTypeChecker.refresh(); return templateTypeChecker.getDiagnosticsForFile(sf); } +export function createProgramWithNoTemplates(): TemplateTestEnvironment { + return setupTemplateTypeChecking( + 'export const NOT_A_COMPONENT = true;', [], {}, {}, () => createTypeCheckAdapter(() => {})); +} + function createTypeCheckAdapter(fn: (ctx: TypeCheckContext) => void): ProgramTypeCheckAdapter { let called = false; return { diff --git a/packages/compiler-cli/test/ngtsc/BUILD.bazel b/packages/compiler-cli/test/ngtsc/BUILD.bazel index 62467c93d9..96e2a67c9e 100644 --- a/packages/compiler-cli/test/ngtsc/BUILD.bazel +++ b/packages/compiler-cli/test/ngtsc/BUILD.bazel @@ -12,6 +12,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/indexer", "//packages/compiler-cli/src/ngtsc/routing", + "//packages/compiler-cli/src/ngtsc/testing", "//packages/compiler-cli/src/ngtsc/util", "//packages/compiler-cli/test:test_utils", "//packages/compiler-cli/test/helpers", diff --git a/packages/compiler-cli/test/ngtsc/env.ts b/packages/compiler-cli/test/ngtsc/env.ts index c32ee50623..281ec58a88 100644 --- a/packages/compiler-cli/test/ngtsc/env.ts +++ b/packages/compiler-cli/test/ngtsc/env.ts @@ -128,6 +128,13 @@ export class NgtscTestEnvironment { return this.oldProgram.getTsProgram(); } + getReuseTsProgram(): ts.Program { + if (this.oldProgram === null) { + throw new Error('No ts.Program has been created yet.'); + } + return (this.oldProgram as NgtscProgram).getReuseTsProgram(); + } + /** * Older versions of the CLI do not provide the `CompilerHost.getModifiedResourceFiles()` method. * This results in the `changedResources` set being `null`. diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 4179b649f0..6d084e5baa 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -9,8 +9,9 @@ import * as ts from 'typescript'; import {ErrorCode, ngErrorCode} from '../../src/ngtsc/diagnostics'; -import {absoluteFrom as _, getFileSystem} from '../../src/ngtsc/file_system'; +import {absoluteFrom as _, getFileSystem, getSourceFileOrError} from '../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; +import {expectCompleteReuse} from '../../src/ngtsc/testing'; import {loadStandardTestFiles} from '../helpers/src/mock_file_loading'; import {NgtscTestEnvironment} from './env'; @@ -1862,18 +1863,26 @@ export declare class AnimationEvent { expect(env.driveDiagnostics()).toEqual([]); }); - it('should not leave references to shims after execution', () => { - // This test verifies that proper cleanup is performed for the technique being used to - // include shim files in the ts.Program, and that none are left in the referencedFiles of - // any ts.SourceFile after compilation. + it('should not leave referencedFiles in a tagged state', () => { env.enableMultipleCompilations(); env.driveMain(); - for (const sf of env.getTsProgram().getSourceFiles()) { - for (const ref of sf.referencedFiles) { - expect(ref.fileName).not.toContain('.ngtypecheck.ts'); - } - } + const sf = getSourceFileOrError(env.getTsProgram(), _('/test.ts')); + expect(sf.referencedFiles.map(ref => ref.fileName)).toEqual([]); + }); + + it('should allow for complete program reuse during incremental compilations', () => { + env.enableMultipleCompilations(); + + env.write('other.ts', `export const VERSION = 1;`); + + env.driveMain(); + const firstProgram = env.getReuseTsProgram(); + + env.write('other.ts', `export const VERSION = 2;`); + env.driveMain(); + + expectCompleteReuse(firstProgram); }); }); });