From 30c82cd17757f41d56afe71f69a90ac812a3201f Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 4 Jul 2021 00:02:57 +0200 Subject: [PATCH] fix(compiler-cli): inline type checking instructions no longer prevent incremental reuse (#42759) Source files that contain directives or components that need an inline type constructor or inline template type-check block would always be considered as affected in incremental rebuilds. The inline operations cause the source file to be updated in the TypeScript program that is created for template type-checking, which becomes the reuse program in a subsequent incremental rebuild. In an incremental rebuild, the source files from the new user program are compared to those from the reuse program. The updated source files are not the same as the original source file from the user program, so the incremental engine would mark the file which needed inline operations as affected. This prevents incremental reuse for these files, causing sub-optimal rebuild performance. This commit attaches the original source file for source files that have been updated with inline operations, such that the incremental engine is able to compare source files using the original source file. Fixes #42543 PR Close #42759 --- .../src/ngtsc/core/src/compiler.ts | 4 +- .../src/ngtsc/incremental/BUILD.bazel | 1 + .../src/ngtsc/incremental/src/incremental.ts | 31 +++++++++++- .../src/ngtsc/program_driver/src/api.ts | 25 +++++++++- .../src/ts_create_program_driver.ts | 12 +++-- .../src/ngtsc/typecheck/src/context.ts | 19 ++++++-- .../src/ngtsc/typecheck/test/program_spec.ts | 15 ++++-- .../test/ngtsc/incremental_spec.ts | 47 +++++++++++++++++++ .../language-service/ivy/language_service.ts | 6 +-- 9 files changed, 140 insertions(+), 20 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index fa95ac48e9..dcc37051f9 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -20,7 +20,7 @@ import {generateAnalysis, IndexedComponent, IndexingContext} from '../../indexer import {ComponentResources, CompoundMetadataReader, CompoundMetadataRegistry, DirectiveMeta, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry, MetadataReader, PipeMeta, ResourceRegistry} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {ActivePerfRecorder, DelegatingPerfRecorder, PerfCheckpoint, PerfEvent, PerfPhase} from '../../perf'; -import {ProgramDriver, UpdateMode} from '../../program_driver'; +import {FileUpdate, ProgramDriver, UpdateMode} from '../../program_driver'; import {DeclarationNode, isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection'; import {AdapterResourceLoader} from '../../resource'; import {entryPointKeyFor, NgModuleRouteAnalyzer} from '../../routing'; @@ -1180,7 +1180,7 @@ class NotifyingProgramDriverWrapper implements ProgramDriver { return this.delegate.getProgram(); } - updateFiles(contents: Map, updateMode: UpdateMode): void { + updateFiles(contents: Map, updateMode: UpdateMode): void { this.delegate.updateFiles(contents, updateMode); this.notifyNewProgram(this.delegate.getProgram()); } diff --git a/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel b/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel index 0e89dfbed2..78f5074011 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel @@ -15,6 +15,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/metadata", "//packages/compiler-cli/src/ngtsc/partial_evaluator", "//packages/compiler-cli/src/ngtsc/perf", + "//packages/compiler-cli/src/ngtsc/program_driver", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/scope", "//packages/compiler-cli/src/ngtsc/transform", diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/incremental.ts b/packages/compiler-cli/src/ngtsc/incremental/src/incremental.ts index 1f466dc968..7ef18d83c2 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/incremental.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/incremental.ts @@ -10,11 +10,13 @@ import * as ts from 'typescript'; import {absoluteFromSourceFile, AbsoluteFsPath, resolve} from '../../file_system'; import {PerfPhase, PerfRecorder} from '../../perf'; +import {MaybeSourceFileWithOriginalFile, NgOriginalFile} from '../../program_driver'; import {ClassRecord, TraitCompiler} from '../../transform'; import {FileTypeCheckingData} from '../../typecheck'; import {toUnredirectedSourceFile} from '../../util/src/typescript'; import {IncrementalBuild} from '../api'; import {SemanticDepGraphUpdater} from '../semantic_graph'; + import {FileDependencyGraph} from './dependency_tracking'; import {AnalyzedIncrementalState, DeltaIncrementalState, IncrementalState, IncrementalStateKind} from './state'; @@ -134,12 +136,12 @@ export class IncrementalCompilation implements IncrementalBuild toUnredirectedSourceFile(sf)); + const oldFilesArray = oldProgram.getSourceFiles().map(toOriginalSourceFile); const oldFiles = new Set(oldFilesArray); const deletedTsFiles = new Set(oldFilesArray.map(sf => absoluteFromSourceFile(sf))); for (const possiblyRedirectedNewFile of program.getSourceFiles()) { - const sf = toUnredirectedSourceFile(possiblyRedirectedNewFile); + const sf = toOriginalSourceFile(possiblyRedirectedNewFile); const sfPath = absoluteFromSourceFile(sf); // Since we're seeing a file in the incoming program with this name, it can't have been // deleted. @@ -373,3 +375,28 @@ export class IncrementalCompilation implements IncrementalBuild, updateMode: UpdateMode): void; + updateFiles(contents: Map, updateMode: UpdateMode): void; /** * Retrieve a string version for a given `ts.SourceFile`, which much change when the contents of diff --git a/packages/compiler-cli/src/ngtsc/program_driver/src/ts_create_program_driver.ts b/packages/compiler-cli/src/ngtsc/program_driver/src/ts_create_program_driver.ts index 4fce91fa2d..5c4c483fb1 100644 --- a/packages/compiler-cli/src/ngtsc/program_driver/src/ts_create_program_driver.ts +++ b/packages/compiler-cli/src/ngtsc/program_driver/src/ts_create_program_driver.ts @@ -12,7 +12,7 @@ import {AbsoluteFsPath} from '../../file_system'; import {copyFileShimData, retagAllTsFiles, ShimReferenceTagger, untagAllTsFiles} from '../../shims'; import {RequiredDelegations, toUnredirectedSourceFile} from '../../util/src/typescript'; -import {ProgramDriver, UpdateMode} from './api'; +import {FileUpdate, MaybeSourceFileWithOriginalFile, NgOriginalFile, ProgramDriver, UpdateMode} from './api'; /** * Delegates all methods of `ts.CompilerHost` to a delegate, with the exception of @@ -153,7 +153,7 @@ export class TsCreateProgramDriver implements ProgramDriver { return this.program; } - updateFiles(contents: Map, updateMode: UpdateMode): void { + 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 @@ -169,8 +169,12 @@ export class TsCreateProgramDriver implements ProgramDriver { this.sfMap.clear(); } - for (const [filePath, text] of contents.entries()) { - this.sfMap.set(filePath, ts.createSourceFile(filePath, text, ts.ScriptTarget.Latest, true)); + for (const [filePath, {newText, originalFile}] of contents.entries()) { + const sf = ts.createSourceFile(filePath, newText, ts.ScriptTarget.Latest, true); + if (originalFile !== null) { + (sf as MaybeSourceFileWithOriginalFile)[NgOriginalFile] = originalFile; + } + this.sfMap.set(filePath, sf); } const host = new UpdatedProgramHost( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 52bf545974..d1ac3e3eac 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -13,6 +13,7 @@ import * as ts from 'typescript'; import {absoluteFromSourceFile, AbsoluteFsPath} from '../../file_system'; import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports'; import {PerfEvent, PerfRecorder} from '../../perf'; +import {FileUpdate} from '../../program_driver'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; import {ImportManager} from '../../translator'; import {TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckContext, TypeCheckingConfig, TypeCtorMetadata} from '../api'; @@ -376,13 +377,16 @@ export class TypeCheckContextImpl implements TypeCheckContext { return code; } - finalize(): Map { + finalize(): Map { // First, build the map of updates to source files. - const updates = new Map(); + const updates = new Map(); for (const originalSf of this.opMap.keys()) { const newText = this.transform(originalSf); if (newText !== null) { - updates.set(absoluteFromSourceFile(originalSf), newText); + updates.set(absoluteFromSourceFile(originalSf), { + newText, + originalFile: originalSf, + }); } } @@ -399,8 +403,13 @@ export class TypeCheckContextImpl implements TypeCheckContext { path: pendingShimData.file.fileName, templates: pendingShimData.templates, }); - updates.set( - pendingShimData.file.fileName, pendingShimData.file.render(false /* removeComments */)); + const sfText = pendingShimData.file.render(false /* removeComments */); + updates.set(pendingShimData.file.fileName, { + newText: sfText, + + // Shim files do not have an associated original file. + originalFile: null, + }); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts index ea15ed67dd..39027c315a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts @@ -10,7 +10,7 @@ import * as ts from 'typescript'; import {absoluteFrom, AbsoluteFsPath, getSourceFileOrError} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; -import {TsCreateProgramDriver, UpdateMode} from '../../program_driver'; +import {FileUpdate, TsCreateProgramDriver, UpdateMode} from '../../program_driver'; import {sfExtensionData, ShimReferenceTagger} from '../../shims'; import {expectCompleteReuse, makeProgram} from '../../testing'; import {OptimizeFor} from '../api'; @@ -42,7 +42,8 @@ runInEachFileSystem(() => { // 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); + new Map([[typecheckPath, createUpdate('export const VERSION = 2;')]]), + UpdateMode.Complete); expectCompleteReuse(programStrategy.getProgram()); }); @@ -54,13 +55,21 @@ runInEachFileSystem(() => { // 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); + new Map([[mainPath, createUpdate('export const STILL_NOT_A_COMPONENT = true;')]]), + UpdateMode.Complete); expectCompleteReuse(programStrategy.getProgram()); }); }); }); +function createUpdate(text: string): FileUpdate { + return { + newText: text, + originalFile: null, + }; +} + function makeSingleFileProgramWithTypecheckShim(): { program: ts.Program, host: ts.CompilerHost, diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index 75780f19a8..8e2827ae90 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -895,6 +895,53 @@ runInEachFileSystem(() => { expect(diags[0].messageText) .toContain(`Type '"gamma"' is not assignable to type 'keyof Keys'.`); }); + + it('should not re-emit files that need inline type constructors', () => { + // Setup a directive that requires an inline type constructor, as it has a generic type + // parameter that refer an interface that has not been exported. The inline operation + // causes an updated dir.ts to be used in the type-check program, which should not + // confuse the incremental engine in undesirably considering dir.ts as affected in + // incremental rebuilds. + env.write('dir.ts', ` + import {Directive, Input} from '@angular/core'; + interface Keys { + alpha: string; + beta: string; + } + @Directive({ + selector: '[dir]' + }) + export class Dir { + @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 {} + `); + env.driveMain(); + + // Trigger a recompile by changing cmp.ts. + env.invalidateCachedFile('cmp.ts'); + + env.flushWrittenFileTracking(); + env.driveMain(); + + // Verify that only cmp.ts was emitted, but not dir.ts as it was not affected. + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).toContain('/cmp.js'); + expect(written).not.toContain('/dir.js'); + }); }); }); }); diff --git a/packages/language-service/ivy/language_service.ts b/packages/language-service/ivy/language_service.ts index 673128a1ab..66573f971d 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -12,7 +12,7 @@ import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; import {ErrorCode, ngErrorCode} from '@angular/compiler-cli/src/ngtsc/diagnostics'; import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system'; import {PerfPhase} from '@angular/compiler-cli/src/ngtsc/perf'; -import {ProgramDriver} from '@angular/compiler-cli/src/ngtsc/program_driver'; +import {FileUpdate, ProgramDriver} from '@angular/compiler-cli/src/ngtsc/program_driver'; import {isNamedClassDeclaration} from '@angular/compiler-cli/src/ngtsc/reflection'; import {TypeCheckShimGenerator} from '@angular/compiler-cli/src/ngtsc/typecheck'; import {OptimizeFor} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; @@ -483,8 +483,8 @@ function createProgramDriver(project: ts.server.Project): ProgramDriver { } return program; }, - updateFiles(contents: Map) { - for (const [fileName, newText] of contents) { + updateFiles(contents: Map) { + for (const [fileName, {newText}] of contents) { const scriptInfo = getOrCreateTypeCheckScriptInfo(project, fileName); const snapshot = scriptInfo.getSnapshot(); const length = snapshot.getLength();