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
This commit is contained in:
Alex Rickabaugh 2020-07-15 12:53:38 -07:00 committed by Misko Hevery
parent c573d91285
commit de14b2c670
6 changed files with 241 additions and 18 deletions

View File

@ -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();

View File

@ -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,
}

View File

@ -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;
}
}

View File

@ -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;

View File

@ -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.');

View File

@ -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': '<span></span>'}}
]);
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': '<div></div>'}},
{fileName: file2, templates: {'Cmp2': '<span></span>'}}
]);
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': '<div dir></div>'},
declarations: [dirDeclaration],
},
{
fileName: file2,
templates: {'CmpB': '<div dir></div>'},
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<T extends NotExported> {}`,
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));