From 01cc9497227aad523d317cb2d587f17bdab8bc87 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 8 Oct 2020 14:25:51 -0700 Subject: [PATCH] refactor(compiler-cli): cache Symbols in the TemplateTypeCheckerImpl (#39278) This commit introduces caching of `Symbol`s produced by the template type- checking infrastructure, in the same way that autocompletion results are now cached. PR Close #39278 --- .../src/ngtsc/typecheck/src/checker.ts | 33 +++++++++-- .../typecheck/src/template_symbol_builder.ts | 59 ++++++++++++------- ...ecker__get_symbol_of_template_node_spec.ts | 27 +++++++++ 3 files changed, 92 insertions(+), 27 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index ad8895d4de..b3a415691f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -42,6 +42,14 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { * `ts.Program` changes, the `TemplateTypeCheckerImpl` as a whole is destroyed and replaced. */ private completionCache = new Map(); + /** + * Stores the `SymbolBuilder` which creates symbols for each component class. + * + * Must be invalidated whenever the component's template or the `ts.Program` changes. Invalidation + * on template changes is performed within this `TemplateTypeCheckerImpl` instance. When the + * `ts.Program` changes, the `TemplateTypeCheckerImpl` as a whole is destroyed and replaced. + */ + private symbolBuilderCache = new Map(); private isComplete = false; @@ -67,6 +75,7 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { // but the `TemplateTypeCheckerImpl` does not track the class for components with overrides. As // a quick workaround, clear the entire cache instead. this.completionCache.clear(); + this.symbolBuilderCache.clear(); } getTemplate(component: ts.ClassDeclaration): TmplAstNode[]|null { @@ -146,8 +155,9 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { fileRecord.isComplete = false; this.isComplete = false; - // Overriding a component's template invalidates its autocompletion results. + // Overriding a component's template invalidates its cached results. this.completionCache.delete(component); + this.symbolBuilderCache.delete(component); return {nodes}; } @@ -400,15 +410,28 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { } getSymbolOfNode(node: AST|TmplAstNode, component: ts.ClassDeclaration): Symbol|null { + const builder = this.getOrCreateSymbolBuilder(component); + if (builder === null) { + return null; + } + return builder.getSymbol(node); + } + + private getOrCreateSymbolBuilder(component: ts.ClassDeclaration): SymbolBuilder|null { + if (this.symbolBuilderCache.has(component)) { + return this.symbolBuilderCache.get(component)!; + } + const {tcb, data, shimPath} = this.getLatestComponentState(component); if (tcb === null || data === null) { return null; } - const typeChecker = this.typeCheckingStrategy.getProgram().getTypeChecker(); - - return new SymbolBuilder(typeChecker, shimPath, tcb, data, this.componentScopeReader) - .getSymbol(node); + const builder = new SymbolBuilder( + shimPath, tcb, data, this.componentScopeReader, + () => this.typeCheckingStrategy.getProgram().getTypeChecker()); + this.symbolBuilderCache.set(component, builder); + return builder; } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index 20f28b58a8..999351cd9f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts @@ -21,41 +21,55 @@ import {isAccessExpression} from './ts_util'; import {TcbDirectiveOutputsOp} from './type_check_block'; /** - * A class which extracts information from a type check block. - * This class is essentially used as just a closure around the constructor parameters. + * Generates and caches `Symbol`s for various template structures for a given component. + * + * The `SymbolBuilder` internally caches the `Symbol`s it creates, and must be destroyed and + * replaced if the component's template changes. */ export class SymbolBuilder { + private symbolCache = new Map(); + constructor( - private readonly typeChecker: ts.TypeChecker, private readonly shimPath: AbsoluteFsPath, private readonly typeCheckBlock: ts.Node, private readonly templateData: TemplateData, private readonly componentScopeReader: ComponentScopeReader, + // The `ts.TypeChecker` depends on the current type-checking program, and so must be requested + // on-demand instead of cached. + private readonly getTypeChecker: () => ts.TypeChecker, ) {} getSymbol(node: TmplAstTemplate|TmplAstElement): TemplateSymbol|ElementSymbol|null; getSymbol(node: TmplAstReference|TmplAstVariable): ReferenceSymbol|VariableSymbol|null; getSymbol(node: AST|TmplAstNode): Symbol|null; getSymbol(node: AST|TmplAstNode): Symbol|null { + if (this.symbolCache.has(node)) { + return this.symbolCache.get(node)!; + } + + let symbol: Symbol|null = null; if (node instanceof TmplAstBoundAttribute || node instanceof TmplAstTextAttribute) { // TODO(atscott): input and output bindings only return the first directive match but should // return a list of bindings for all of them. - return this.getSymbolOfInputBinding(node); + symbol = this.getSymbolOfInputBinding(node); } else if (node instanceof TmplAstBoundEvent) { - return this.getSymbolOfBoundEvent(node); + symbol = this.getSymbolOfBoundEvent(node); } else if (node instanceof TmplAstElement) { - return this.getSymbolOfElement(node); + symbol = this.getSymbolOfElement(node); } else if (node instanceof TmplAstTemplate) { - return this.getSymbolOfAstTemplate(node); + symbol = this.getSymbolOfAstTemplate(node); } else if (node instanceof TmplAstVariable) { - return this.getSymbolOfVariable(node); + symbol = this.getSymbolOfVariable(node); } else if (node instanceof TmplAstReference) { - return this.getSymbolOfReference(node); + symbol = this.getSymbolOfReference(node); } else if (node instanceof AST) { - return this.getSymbolOfTemplateExpression(node); + symbol = this.getSymbolOfTemplateExpression(node); + } else { + // TODO(atscott): TmplAstContent, TmplAstIcu } - // TODO(atscott): TmplAstContent, TmplAstIcu - return null; + + this.symbolCache.set(node, symbol); + return symbol; } private getSymbolOfAstTemplate(template: TmplAstTemplate): TemplateSymbol|null { @@ -171,7 +185,8 @@ export class SymbolBuilder { return null; } - const tsSymbol = this.typeChecker.getSymbolAtLocation(outputFieldAccess.argumentExpression); + const tsSymbol = + this.getTypeChecker().getSymbolAtLocation(outputFieldAccess.argumentExpression); if (tsSymbol === undefined) { return null; } @@ -183,7 +198,7 @@ export class SymbolBuilder { } const positionInShimFile = this.getShimPositionForNode(outputFieldAccess); - const tsType = this.typeChecker.getTypeAtLocation(node); + const tsType = this.getTypeChecker().getTypeAtLocation(node); return { kind: SymbolKind.Output, bindings: [{ @@ -240,7 +255,7 @@ export class SymbolBuilder { {isComponent, selector}: TypeCheckableDirectiveMeta): DirectiveSymbol|null { // In either case, `_t1["index"]` or `_t1.index`, `node.expression` is _t1. // The retrieved symbol for _t1 will be the variable declaration. - const tsSymbol = this.typeChecker.getSymbolAtLocation(node.expression); + const tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.expression); if (tsSymbol === undefined || tsSymbol.declarations.length === 0) { return null; } @@ -304,8 +319,8 @@ export class SymbolBuilder { // initializers as invalid for symbol retrieval. const originalDeclaration = ts.isParenthesizedExpression(node.initializer) && ts.isAsExpression(node.initializer.expression) ? - this.typeChecker.getSymbolAtLocation(node.name) : - this.typeChecker.getSymbolAtLocation(node.initializer); + this.getTypeChecker().getSymbolAtLocation(node.name) : + this.getTypeChecker().getSymbolAtLocation(node.initializer); if (originalDeclaration === undefined || originalDeclaration.valueDeclaration === undefined) { return null; } @@ -384,7 +399,7 @@ export class SymbolBuilder { kind: SymbolKind.Expression, // Rather than using the type of only the `whenTrue` part of the expression, we should // still get the type of the whole conditional expression to include `|undefined`. - tsType: this.typeChecker.getTypeAtLocation(node) + tsType: this.getTypeChecker().getTypeAtLocation(node) }; } else if (expression instanceof BindingPipe && ts.isCallExpression(node)) { // TODO(atscott): Create a PipeSymbol to include symbol for the Pipe class @@ -403,15 +418,15 @@ export class SymbolBuilder { let tsSymbol: ts.Symbol|undefined; if (ts.isPropertyAccessExpression(node)) { - tsSymbol = this.typeChecker.getSymbolAtLocation(node.name); + tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.name); } else if (ts.isElementAccessExpression(node)) { - tsSymbol = this.typeChecker.getSymbolAtLocation(node.argumentExpression); + tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.argumentExpression); } else { - tsSymbol = this.typeChecker.getSymbolAtLocation(node); + tsSymbol = this.getTypeChecker().getSymbolAtLocation(node); } const positionInShimFile = this.getShimPositionForNode(node); - const type = this.typeChecker.getTypeAtLocation(node); + const type = this.getTypeChecker().getTypeAtLocation(node); return { // If we could not find a symbol, fall back to the symbol on the type for the node. // Some nodes won't have a "symbol at location" but will have a symbol for the type. diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts index 1deb4dc9a9..326c9c6136 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts @@ -39,6 +39,33 @@ runInEachFileSystem(() => { assertElementSymbol(symbol.host); }); + it('should invalidate symbols when template overrides change', () => { + const fileName = absoluteFrom('/main.ts'); + const templateString = `
`; + const {templateTypeChecker, program} = setup( + [ + { + fileName, + templates: {'Cmp': templateString}, + source: `export class Cmp {}`, + }, + ], + ); + const sf = getSourceFileOrError(program, fileName); + const cmp = getClass(sf, 'Cmp'); + const {attributes: beforeAttributes} = getAstElements(templateTypeChecker, cmp)[0]; + const beforeSymbol = templateTypeChecker.getSymbolOfNode(beforeAttributes[0], cmp)!; + + // Replace the
with a . + templateTypeChecker.overrideComponentTemplate(cmp, ''); + + const {attributes: afterAttributes} = getAstElements(templateTypeChecker, cmp)[0]; + const afterSymbol = templateTypeChecker.getSymbolOfNode(afterAttributes[0], cmp)!; + + // After the override, the symbol cache should have been invalidated. + expect(beforeSymbol).not.toBe(afterSymbol); + }); + it('should get a symbol for text attributes corresponding with a directive input', () => { const fileName = absoluteFrom('/main.ts'); const dirFile = absoluteFrom('/dir.ts');