From de14b2c6709297cadb1923faac6f4586fd78f67b Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 15 Jul 2020 12:53:38 -0700 Subject: [PATCH] refactor(compiler-cli): efficient single-file type checking diagnostics (#38105) Previously, the `TemplateTypeChecker` abstraction allowed fetching diagnostics for a single file, but under the hood would generate type checking code for the entire program to satisfy the request. With this commit, an `OptimizeFor` hint is passed to `getDiagnosticsForFile` which indicates whether the user intends to request diagnostics for the whole program or is truly interested in just the single file. If the latter, the `TemplateTypeChecker` can perform only the work needed to produce diagnostics for just that file, thus returning answers more efficiently. PR Close #38105 --- .../src/ngtsc/core/src/compiler.ts | 5 +- .../src/ngtsc/typecheck/api/checker.ts | 34 ++++- .../src/ngtsc/typecheck/src/checker.ts | 120 +++++++++++++++++- .../ngtsc/typecheck/test/diagnostics_spec.ts | 4 +- .../src/ngtsc/typecheck/test/program_spec.ts | 4 +- .../ngtsc/typecheck/test/type_checker_spec.ts | 92 +++++++++++++- 6 files changed, 241 insertions(+), 18 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 6c63f02846..6e7cd3ff04 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -29,7 +29,7 @@ import {generatedFactoryTransform} from '../../shims'; import {ivySwitchTransform} from '../../switch'; import {aliasTransformFactory, declarationTransformFactory, DecoratorHandler, DtsTransformRegistry, ivyTransformFactory, TraitCompiler} from '../../transform'; import {isTemplateDiagnostic, TemplateTypeCheckerImpl} from '../../typecheck'; -import {TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy} from '../../typecheck/api'; +import {OptimizeFor, TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy} from '../../typecheck/api'; import {getSourceFileOrNull, isDtsPath, resolveModuleName} from '../../util/src/typescript'; import {LazyRoute, NgCompilerAdapter, NgCompilerOptions} from '../api'; @@ -507,7 +507,8 @@ export class NgCompiler { continue; } - diagnostics.push(...compilation.templateTypeChecker.getDiagnosticsForFile(sf)); + diagnostics.push( + ...compilation.templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram)); } const program = this.typeCheckingProgramStrategy.getProgram(); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts index 47a3646b86..7b4ff3e2b5 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts @@ -29,8 +29,16 @@ export interface TemplateTypeChecker { * * This method will fail (throw) if there are components within the `ts.SourceFile` that do not * have TCBs available. + * + * Generating a template type-checking program is expensive, and in some workflows (e.g. checking + * an entire program before emit), it should ideally only be done once. The `optimizeFor` flag + * allows the caller to hint to `getDiagnosticsForFile` (which internally will create a template + * type-checking program if needed) whether the caller is interested in just the results of the + * single file, or whether they plan to query about other files in the program. Based on this + * flag, `getDiagnosticsForFile` will determine how much of the user's program to prepare for + * checking as part of the template type-checking program it creates. */ - getDiagnosticsForFile(sf: ts.SourceFile): ts.Diagnostic[]; + getDiagnosticsForFile(sf: ts.SourceFile, optimizeFor: OptimizeFor): ts.Diagnostic[]; /** * Retrieve the top-level node representing the TCB for the given component. @@ -41,3 +49,27 @@ export interface TemplateTypeChecker { */ getTypeCheckBlock(component: ts.ClassDeclaration): ts.Node|null; } + +/** + * Describes the scope of the caller's interest in template type-checking results. + */ +export enum OptimizeFor { + /** + * Indicates that a consumer of a `TemplateTypeChecker` is only interested in results for a given + * file, and wants them as fast as possible. + * + * Calling `TemplateTypeChecker` methods successively for multiple files while specifying + * `OptimizeFor.SingleFile` can result in significant unnecessary overhead overall. + */ + SingleFile, + + /** + * Indicates that a consumer of a `TemplateTypeChecker` intends to query for results pertaining to + * the entire user program, and so the type-checker should internally optimize for this case. + * + * Initial calls to retrieve type-checking information may take longer, but repeated calls to + * gather information for the whole user program will be significantly faster with this mode of + * optimization. + */ + WholeProgram, +} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index 53fa546853..d6197ac821 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -14,7 +14,7 @@ import {IncrementalBuild} from '../../incremental/api'; import {ReflectionHost} from '../../reflection'; import {isShim} from '../../shims'; import {getSourceFileOrNull} from '../../util/src/typescript'; -import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy, UpdateMode} from '../api'; +import {OptimizeFor, ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy, UpdateMode} from '../api'; import {InliningMode, ShimTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from './context'; import {findTypeCheckBlock, shouldReportDiagnostic, TemplateSourceResolver, translateDiagnostic} from './diagnostics'; @@ -41,8 +41,15 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { * Retrieve type-checking diagnostics from the given `ts.SourceFile` using the most recent * type-checking program. */ - getDiagnosticsForFile(sf: ts.SourceFile): ts.Diagnostic[] { - this.ensureAllShimsForAllFiles(); + getDiagnosticsForFile(sf: ts.SourceFile, optimizeFor: OptimizeFor): ts.Diagnostic[] { + switch (optimizeFor) { + case OptimizeFor.WholeProgram: + this.ensureAllShimsForAllFiles(); + break; + case OptimizeFor.SingleFile: + this.ensureAllShimsForOneFile(sf); + break; + } const sfPath = absoluteFromSourceFile(sf); const fileRecord = this.state.get(sfPath)!; @@ -68,7 +75,7 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { } getTypeCheckBlock(component: ts.ClassDeclaration): ts.Node|null { - this.ensureAllShimsForAllFiles(); + this.ensureAllShimsForOneFile(component.getSourceFile()); const program = this.typeCheckingStrategy.getProgram(); const filePath = absoluteFromSourceFile(component.getSourceFile()); @@ -81,7 +88,7 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { const id = fileRecord.sourceManager.getTemplateId(component); const shimSf = getSourceFileOrNull(program, shimPath); - if (shimSf === null) { + if (shimSf === null || !fileRecord.shimData.has(shimPath)) { throw new Error(`Error: no shim file in program: ${shimPath}`); } @@ -144,6 +151,27 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { this.isComplete = true; } + private ensureAllShimsForOneFile(sf: ts.SourceFile): void { + this.maybeAdoptPriorResultsForFile(sf); + + const sfPath = absoluteFromSourceFile(sf); + + const fileData = this.getFileData(sfPath); + if (fileData.isComplete) { + // All data for this file is present and accounted for already. + return; + } + + const host = new SingleFileTypeCheckingHost(sfPath, fileData, this.typeCheckingStrategy, this); + const ctx = this.newContext(host); + + this.typeCheckAdapter.typeCheck(sf, ctx); + + fileData.isComplete = true; + + this.updateFromContext(ctx); + } + private newContext(host: TypeCheckingHost): TypeCheckContextImpl { const inlining = this.typeCheckingStrategy.supportsInlineOperations ? InliningMode.InlineOps : InliningMode.Error; @@ -152,6 +180,30 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { host, inlining); } + /** + * Remove any shim data that depends on inline operations applied to the type-checking program. + * + * This can be useful if new inlines need to be applied, and it's not possible to guarantee that + * they won't overwrite or corrupt existing inlines that are used by such shims. + */ + clearAllShimDataUsingInlines(): void { + for (const fileData of this.state.values()) { + if (!fileData.hasInlines) { + continue; + } + + for (const [shimFile, shimData] of fileData.shimData.entries()) { + if (shimData.hasInlines) { + fileData.shimData.delete(shimFile); + } + } + + fileData.hasInlines = false; + fileData.isComplete = false; + this.isComplete = false; + } + } + private updateFromContext(ctx: TypeCheckContextImpl): void { const updates = ctx.finalize(); this.typeCheckingStrategy.updateFiles(updates, UpdateMode.Incremental); @@ -240,3 +292,61 @@ class WholeProgramTypeCheckingHost implements TypeCheckingHost { this.impl.getFileData(sfPath).isComplete = true; } } + +/** + * Drives a `TypeCheckContext` to generate type-checking code efficiently for a single input file. + */ +class SingleFileTypeCheckingHost implements TypeCheckingHost { + private seenInlines = false; + + constructor( + private sfPath: AbsoluteFsPath, private fileData: FileTypeCheckingData, + private strategy: TypeCheckingProgramStrategy, private impl: TemplateTypeCheckerImpl) {} + + private assertPath(sfPath: AbsoluteFsPath): void { + if (this.sfPath !== sfPath) { + throw new Error(`AssertionError: querying TypeCheckingHost outside of assigned file`); + } + } + + getSourceManager(sfPath: AbsoluteFsPath): TemplateSourceManager { + this.assertPath(sfPath); + return this.fileData.sourceManager; + } + + shouldCheckComponent(node: ts.ClassDeclaration): boolean { + if (this.sfPath !== absoluteFromSourceFile(node.getSourceFile())) { + return false; + } + const shimPath = this.strategy.shimPathForComponent(node); + + // Only need to generate a TCB for the class if no shim exists for it currently. + return !this.fileData.shimData.has(shimPath); + } + + recordShimData(sfPath: AbsoluteFsPath, data: ShimTypeCheckingData): void { + this.assertPath(sfPath); + + // Previous type-checking state may have required the use of inlines (assuming they were + // supported). If the current operation also requires inlines, this presents a problem: + // generating new inlines may invalidate any old inlines that old state depends on. + // + // Rather than resolve this issue by tracking specific dependencies on inlines, if the new state + // relies on inlines, any old state that relied on them is simply cleared. This happens when the + // first new state that uses inlines is encountered. + if (data.hasInlines && !this.seenInlines) { + this.impl.clearAllShimDataUsingInlines(); + this.seenInlines = true; + } + + this.fileData.shimData.set(data.path, data); + if (data.hasInlines) { + this.fileData.hasInlines = true; + } + } + + recordComplete(sfPath: AbsoluteFsPath): void { + this.assertPath(sfPath); + this.fileData.isComplete = true; + } +} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts index 2aa048eeff..081296f637 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts @@ -10,7 +10,7 @@ import * as ts from 'typescript'; import {absoluteFrom, getSourceFileOrError} from '../../file_system'; import {runInEachFileSystem, TestFile} from '../../file_system/testing'; -import {TypeCheckingConfig} from '../api'; +import {OptimizeFor, TypeCheckingConfig} from '../api'; import {ngForDeclaration, ngForDts, setup, TestDeclaration} from './test_utils'; @@ -472,7 +472,7 @@ function diagnose( ], {config, options}); const sf = getSourceFileOrError(program, sfPath); - const diagnostics = templateTypeChecker.getDiagnosticsForFile(sf); + const diagnostics = templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram); return diagnostics.map(diag => { const text = typeof diag.messageText === 'string' ? diag.messageText : diag.messageText.messageText; 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 f367e1687e..601808d55e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/program_spec.ts @@ -12,7 +12,7 @@ import {absoluteFrom, AbsoluteFsPath, getSourceFileOrError} from '../../file_sys import {runInEachFileSystem} from '../../file_system/testing'; import {sfExtensionData, ShimReferenceTagger} from '../../shims'; import {expectCompleteReuse, makeProgram} from '../../testing'; -import {UpdateMode} from '../api'; +import {OptimizeFor, UpdateMode} from '../api'; import {ReusedProgramStrategy} from '../src/augmented_program'; import {setup} from './test_utils'; @@ -28,7 +28,7 @@ runInEachFileSystem(() => { }]); const sf = getSourceFileOrError(program, fileName); - templateTypeChecker.getDiagnosticsForFile(sf); + templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram); // 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.'); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts index 2c1ee6b751..b331c058eb 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts @@ -9,8 +9,9 @@ import {ErrorCode, ngErrorCode} from '../../diagnostics'; import {absoluteFrom, absoluteFromSourceFile, getSourceFileOrError} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; +import {OptimizeFor} from '../api'; -import {getClass, setup} from './test_utils'; +import {getClass, setup, TestDeclaration} from './test_utils'; runInEachFileSystem(() => { describe('TemplateTypeChecker', () => { @@ -22,14 +23,43 @@ runInEachFileSystem(() => { {fileName: file2, templates: {'Cmp2': ''}} ]); - templateTypeChecker.getDiagnosticsForFile(getSourceFileOrError(program, file1)); + templateTypeChecker.getDiagnosticsForFile( + getSourceFileOrError(program, file1), OptimizeFor.WholeProgram); const ttcProgram1 = programStrategy.getProgram(); - templateTypeChecker.getDiagnosticsForFile(getSourceFileOrError(program, file2)); + templateTypeChecker.getDiagnosticsForFile( + getSourceFileOrError(program, file2), OptimizeFor.WholeProgram); const ttcProgram2 = programStrategy.getProgram(); expect(ttcProgram1).toBe(ttcProgram2); }); + it('should not batch diagnostic operations when requested in SingleFile mode', () => { + const file1 = absoluteFrom('/file1.ts'); + const file2 = absoluteFrom('/file2.ts'); + const {program, templateTypeChecker, programStrategy} = setup([ + {fileName: file1, templates: {'Cmp1': '
'}}, + {fileName: file2, templates: {'Cmp2': ''}} + ]); + + templateTypeChecker.getDiagnosticsForFile( + getSourceFileOrError(program, file1), OptimizeFor.SingleFile); + const ttcProgram1 = programStrategy.getProgram(); + + // ttcProgram1 should not contain a type check block for Cmp2. + const ttcSf2Before = getSourceFileOrError(ttcProgram1, absoluteFrom('/file2.ngtypecheck.ts')); + expect(ttcSf2Before.text).not.toContain('Cmp2'); + + templateTypeChecker.getDiagnosticsForFile( + getSourceFileOrError(program, file2), OptimizeFor.SingleFile); + const ttcProgram2 = programStrategy.getProgram(); + + // ttcProgram2 should now contain a type check block for Cmp2. + const ttcSf2After = getSourceFileOrError(ttcProgram2, absoluteFrom('/file2.ngtypecheck.ts')); + expect(ttcSf2After.text).toContain('Cmp2'); + + expect(ttcProgram1).not.toBe(ttcProgram2); + }); + it('should allow access to the type-check block of a component', () => { const file1 = absoluteFrom('/file1.ts'); const file2 = absoluteFrom('/file2.ts'); @@ -45,6 +75,56 @@ runInEachFileSystem(() => { expect(block!.getText()).toContain(`document.createElement("div")`); }); + it('should clear old inlines when necessary', () => { + const file1 = absoluteFrom('/file1.ts'); + const file2 = absoluteFrom('/file2.ts'); + const dirFile = absoluteFrom('/dir.ts'); + const dirDeclaration: TestDeclaration = { + name: 'TestDir', + selector: '[dir]', + file: dirFile, + type: 'directive', + }; + const {program, templateTypeChecker, programStrategy} = setup([ + { + fileName: file1, + templates: {'CmpA': '
'}, + declarations: [dirDeclaration], + }, + { + fileName: file2, + templates: {'CmpB': '
'}, + declarations: [dirDeclaration], + }, + { + fileName: dirFile, + source: ` + // A non-exported interface used as a type bound for a generic directive causes + // an inline type constructor to be required. + interface NotExported {} + export class TestDir {}`, + templates: {}, + }, + ]); + const sf1 = getSourceFileOrError(program, file1); + const cmpA = getClass(sf1, 'CmpA'); + const sf2 = getSourceFileOrError(program, file2); + const cmpB = getClass(sf2, 'CmpB'); + // Prime the TemplateTypeChecker by asking for a TCB from file1. + expect(templateTypeChecker.getTypeCheckBlock(cmpA)).not.toBeNull(); + + // Next, ask for a TCB from file2. This operation should clear data on TCBs generated for + // file1. + expect(templateTypeChecker.getTypeCheckBlock(cmpB)).not.toBeNull(); + console.error(templateTypeChecker.getTypeCheckBlock(cmpB)?.getSourceFile().text); + + // This can be detected by asking for a TCB again from file1. Since no data should be + // available for file1, this should cause another type-checking program step. + const prevTtcProgram = programStrategy.getProgram(); + expect(templateTypeChecker.getTypeCheckBlock(cmpA)).not.toBeNull(); + expect(programStrategy.getProgram()).not.toBe(prevTtcProgram); + }); + describe('when inlining is unsupported', () => { it('should not produce errors for components that do not require inlining', () => { const fileName = absoluteFrom('/main.ts'); @@ -70,7 +150,7 @@ runInEachFileSystem(() => { ], {inlining: false}); const sf = getSourceFileOrError(program, fileName); - const diags = templateTypeChecker.getDiagnosticsForFile(sf); + const diags = templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram); expect(diags.length).toBe(0); }); @@ -84,7 +164,7 @@ runInEachFileSystem(() => { }], {inlining: false}); const sf = getSourceFileOrError(program, fileName); - const diags = templateTypeChecker.getDiagnosticsForFile(sf); + const diags = templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram); expect(diags.length).toBe(1); expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TCB_REQUIRED)); }); @@ -117,7 +197,7 @@ runInEachFileSystem(() => { ], {inlining: false}); const sf = getSourceFileOrError(program, fileName); - const diags = templateTypeChecker.getDiagnosticsForFile(sf); + const diags = templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram); expect(diags.length).toBe(1); expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TYPE_CTOR_REQUIRED));