From af95dddd7ea4d90cb1354412174beb502274bbd3 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 10 Dec 2019 11:39:45 -0800 Subject: [PATCH] perf(ivy): eagerly parse the template twice during analysis (#34334) A quirk of the Angular template parser is that when parsing templates in the "default" mode, with options specified by the user, the source mapping information in the template AST may be inaccurate. As a result, the compiler parses the template twice: once for "emit" and once to produce an AST with accurate sourcemaps for diagnostic production. Previously, only the first parse was performed during analysis. The second parse occurred during the template type-checking phase, just in time to produce the template type-checking file. However, with the reuse of analysis results during incremental builds, it makes more sense to do the diagnostic parse eagerly during analysis so that the work isn't unnecessarily repeated in subsequent builds. This commit refactors the `ComponentDecoratorHandler` to do both parses eagerly, which actually cleans up some complexity around template parsing as well. PR Close #34334 --- .../ngcc/src/analysis/decoration_analyzer.ts | 2 +- .../src/ngtsc/annotations/src/component.ts | 186 +++++++++--------- 2 files changed, 92 insertions(+), 96 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index db7b8c2c7f..fca42c24d6 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -93,8 +93,8 @@ export class DecorationAnalyzer { /* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat, this.moduleResolver, this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, NOOP_DEPENDENCY_TRACKER, /* annotateForClosureCompiler */ false), + // See the note in ngtsc about why this cast is needed. // clang-format off - // See the note in ngtsc about why this cast is needed. new DirectiveDecoratorHandler( this.reflectionHost, this.evaluator, this.fullRegistry, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, /* annotateForClosureCompiler */ false) as DecoratorHandler, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index d1420f8e92..28a6dc8684 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -51,10 +51,8 @@ export interface ComponentAnalysisData { meta: Omit; baseClass: Reference|'dynamic'|null; guards: ReturnType; - parsedTemplate: ParsedTemplate; - templateSourceMapping: TemplateSourceMapping; + template: ParsedTemplateWithSource; metadataStmt: Statement|null; - parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; } export type ComponentResolutionData = Pick; @@ -83,7 +81,7 @@ export class ComponentDecoratorHandler implements * any potential tags which might need to be loaded. This cache ensures that work is not * thrown away, and the parsed template is reused during the analyze phase. */ - private preanalyzeTemplateCache = new Map(); + private preanalyzeTemplateCache = new Map(); readonly precedence = HandlerPrecedence.PRIMARY; readonly name = ComponentDecoratorHandler.name; @@ -200,16 +198,13 @@ export class ComponentDecoratorHandler implements // the preanalyzeTemplateCache. // Extract a closure of the template parsing code so that it can be reparsed with different // options if needed, like in the indexing pipeline. - let parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; - // Track the origin of the template to determine how the ParseSourceSpans should be interpreted. - let templateSourceMapping: TemplateSourceMapping; + let template: ParsedTemplateWithSource; if (this.preanalyzeTemplateCache.has(node)) { // The template was parsed in preanalyze. Use it and delete it to save memory. const preanalyzed = this.preanalyzeTemplateCache.get(node) !; this.preanalyzeTemplateCache.delete(node); - parseTemplate = preanalyzed.parseTemplate; - templateSourceMapping = preanalyzed.templateSourceMapping; + template = preanalyzed; } else { // The template was not already parsed. Either there's a templateUrl, or an inline template. if (component.has('templateUrl')) { @@ -220,20 +215,12 @@ export class ComponentDecoratorHandler implements ErrorCode.VALUE_HAS_WRONG_TYPE, templateUrlExpr, 'templateUrl must be a string'); } const resourceUrl = this.resourceLoader.resolve(templateUrl, containingFile); - const external = - this._extractExternalTemplate(node, component, templateUrlExpr, resourceUrl); - - parseTemplate = external.parseTemplate; - templateSourceMapping = external.templateSourceMapping; + template = this._extractExternalTemplate(node, component, templateUrlExpr, resourceUrl); } else { // Expect an inline template to be present. - const inline = this._extractInlineTemplate(node, decorator, component, containingFile); - - parseTemplate = inline.parseTemplate; - templateSourceMapping = inline.templateSourceMapping; + template = this._extractInlineTemplate(node, decorator, component, containingFile); } } - const template = parseTemplate(); if (template.errors !== undefined) { throw new Error( @@ -293,7 +280,9 @@ export class ComponentDecoratorHandler implements baseClass: readBaseClass(node, this.reflector, this.evaluator), meta: { ...metadata, - template, + template: { + nodes: template.emitNodes, + }, encapsulation, interpolation: template.interpolation, styles: styles || [], @@ -308,7 +297,7 @@ export class ComponentDecoratorHandler implements metadataStmt: generateSetClassMetadataCall( node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), - parsedTemplate: template, parseTemplate, templateSourceMapping, + template, }, }; if (changeDetection !== null) { @@ -336,14 +325,6 @@ export class ComponentDecoratorHandler implements index( context: IndexingContext, node: ClassDeclaration, analysis: Readonly) { - // The component template may have been previously parsed without preserving whitespace or with - // `leadingTriviaChar`s, both of which may manipulate the AST into a form not representative of - // the source code, making it unsuitable for indexing. The template is reparsed with preserving - // options to remedy this. - const template = analysis.parseTemplate({ - preserveWhitespaces: true, - leadingTriviaChars: [], - }); const scope = this.scopeReader.getScopeForComponent(node); const selector = analysis.meta.selector; const matcher = new SelectorMatcher(); @@ -355,15 +336,15 @@ export class ComponentDecoratorHandler implements } } const binder = new R3TargetBinder(matcher); - const boundTemplate = binder.bind({template: template.nodes}); + const boundTemplate = binder.bind({template: analysis.template.diagNodes}); context.addComponent({ declaration: node, selector, boundTemplate, templateMeta: { - isInline: template.isInline, - file: template.file, + isInline: analysis.template.isInline, + file: analysis.template.file, }, }); } @@ -374,20 +355,6 @@ export class ComponentDecoratorHandler implements return; } - // There are issues with parsing the template under certain configurations (namely with - // `preserveWhitespaces: false`) which cause inaccurate positional information within the - // template AST, particularly within interpolation expressions. - // - // To work around this, the template is re-parsed with settings that guarantee the spans are as - // accurate as possible. This is only a temporary solution until the whitespace removal step can - // be rewritten as a transform against the expression AST instead of against the HTML AST. - // - // TODO(alxhub): remove this when whitespace removal no longer corrupts span information. - const template = meta.parseTemplate({ - preserveWhitespaces: true, - leadingTriviaChars: [], - }); - const matcher = new SelectorMatcher(); const pipes = new Map>>(); let schemas: SchemaMetadata[] = []; @@ -410,9 +377,10 @@ export class ComponentDecoratorHandler implements schemas = scope.schemas; } - const bound = new R3TargetBinder(matcher).bind({template: template.nodes}); + const bound = new R3TargetBinder(matcher).bind({template: meta.template.diagNodes}); ctx.addTemplate( - new Reference(node), bound, pipes, schemas, meta.templateSourceMapping, template.file); + new Reference(node), bound, pipes, schemas, meta.template.sourceMapping, + meta.template.file); } resolve(node: ClassDeclaration, analysis: Readonly): @@ -613,56 +581,48 @@ export class ComponentDecoratorHandler implements // URLs to resolve. if (templatePromise !== undefined) { return templatePromise.then(() => { - const {parseTemplate, templateSourceMapping} = + const template = this._extractExternalTemplate(node, component, templateUrlExpr, resourceUrl); - const template = parseTemplate(); - this.preanalyzeTemplateCache.set( - node, {...template, parseTemplate, templateSourceMapping}); + this.preanalyzeTemplateCache.set(node, template); return template; }); } else { return Promise.resolve(null); } } else { - const {parseTemplate, templateSourceMapping} = - this._extractInlineTemplate(node, decorator, component, containingFile); - const template = parseTemplate(); - this.preanalyzeTemplateCache.set(node, {...template, parseTemplate, templateSourceMapping}); + const template = this._extractInlineTemplate(node, decorator, component, containingFile); + this.preanalyzeTemplateCache.set(node, template); return Promise.resolve(template); } } private _extractExternalTemplate( node: ClassDeclaration, component: Map, templateUrlExpr: ts.Expression, - resourceUrl: string): { - parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; - templateSourceMapping: TemplateSourceMapping - } { + resourceUrl: string): ParsedTemplateWithSource { const templateStr = this.resourceLoader.load(resourceUrl); if (this.depTracker !== null) { this.depTracker.addResourceDependency(node.getSourceFile(), resourceUrl); } - const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate( - component, templateStr, sourceMapUrl(resourceUrl), - /* templateRange */ undefined, - /* escapedString */ false, options); - const templateSourceMapping: TemplateSourceMapping = { - type: 'external', - componentClass: node, - node: templateUrlExpr, - template: templateStr, - templateUrl: resourceUrl, - }; - return {parseTemplate, templateSourceMapping}; + const template = this._parseTemplate( + component, templateStr, sourceMapUrl(resourceUrl), /* templateRange */ undefined, + /* escapedString */ false); + + return { + ...template, + sourceMapping: { + type: 'external', + componentClass: node, + node: templateUrlExpr, + template: templateStr, + templateUrl: resourceUrl, + }, + }; } private _extractInlineTemplate( node: ClassDeclaration, decorator: Decorator, component: Map, - containingFile: string): { - parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; - templateSourceMapping: TemplateSourceMapping - } { + containingFile: string): ParsedTemplateWithSource { if (!component.has('template')) { throw new FatalDiagnosticError( ErrorCode.COMPONENT_MISSING_TEMPLATE, Decorator.nodeForError(decorator), @@ -673,7 +633,7 @@ export class ComponentDecoratorHandler implements let templateStr: string; let templateUrl: string = ''; let templateRange: LexerRange|undefined = undefined; - let templateSourceMapping: TemplateSourceMapping; + let sourceMapping: TemplateSourceMapping; let escapedString = false; // We only support SourceMaps for inline templates that are simple string literals. if (ts.isStringLiteral(templateExpr) || ts.isNoSubstitutionTemplateLiteral(templateExpr)) { @@ -684,7 +644,7 @@ export class ComponentDecoratorHandler implements templateStr = templateExpr.getSourceFile().text; templateUrl = containingFile; escapedString = true; - templateSourceMapping = { + sourceMapping = { type: 'direct', node: templateExpr as(ts.StringLiteral | ts.NoSubstitutionTemplateLiteral), }; @@ -695,7 +655,7 @@ export class ComponentDecoratorHandler implements ErrorCode.VALUE_HAS_WRONG_TYPE, templateExpr, 'template must be a string'); } templateStr = resolvedTemplate; - templateSourceMapping = { + sourceMapping = { type: 'indirect', node: templateExpr, componentClass: node, @@ -703,16 +663,15 @@ export class ComponentDecoratorHandler implements }; } - const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate( - component, templateStr, templateUrl, templateRange, escapedString, options); + const template = + this._parseTemplate(component, templateStr, templateUrl, templateRange, escapedString); - return {parseTemplate, templateSourceMapping}; + return {...template, sourceMapping}; } private _parseTemplate( component: Map, templateStr: string, templateUrl: string, - templateRange: LexerRange|undefined, escapedString: boolean, - options: ParseTemplateOptions = {}): ParsedTemplate { + templateRange: LexerRange|undefined, escapedString: boolean): ParsedTemplate { let preserveWhitespaces: boolean = this.defaultPreserveWhitespaces; if (component.has('preserveWhitespaces')) { const expr = component.get('preserveWhitespaces') !; @@ -737,14 +696,41 @@ export class ComponentDecoratorHandler implements interpolation = InterpolationConfig.fromArray(value as[string, string]); } + const {errors, nodes: emitNodes, styleUrls, styles} = parseTemplate(templateStr, templateUrl, { + preserveWhitespaces, + interpolationConfig: interpolation, + range: templateRange, escapedString, + enableI18nLegacyMessageIdFormat: this.enableI18nLegacyMessageIdFormat, + }); + + // Unfortunately, the primary parse of the template above may not contain accurate source map + // information. If used directly, it would result in incorrect code locations in template + // errors, etc. There are two main problems: + // + // 1. `preserveWhitespaces: false` annihilates the correctness of template source mapping, as + // the whitespace transformation changes the contents of HTML text nodes before they're + // parsed into Angular expressions. + // 2. By default, the template parser strips leading trivia characters (like spaces, tabs, and + // newlines). This also destroys source mapping information. + // + // In order to guarantee the correctness of diagnostics, templates are parsed a second time with + // the above options set to preserve source mappings. + + const {nodes: diagNodes} = parseTemplate(templateStr, templateUrl, { + preserveWhitespaces: true, + interpolationConfig: interpolation, + range: templateRange, escapedString, + enableI18nLegacyMessageIdFormat: this.enableI18nLegacyMessageIdFormat, + leadingTriviaChars: [], + }); + return { interpolation, - ...parseTemplate(templateStr, templateUrl, { - preserveWhitespaces, - interpolationConfig: interpolation, - range: templateRange, escapedString, - enableI18nLegacyMessageIdFormat: this.enableI18nLegacyMessageIdFormat, ...options, - }), + emitNodes, + diagNodes, + styleUrls, + styles, + errors, template: templateStr, templateUrl, isInline: component.has('template'), file: new ParseSourceFile(templateStr, templateUrl), @@ -838,9 +824,20 @@ export interface ParsedTemplate { errors?: ParseError[]|undefined; /** - * The actual parsed template nodes. + * The template AST, parsed according to the user's specifications. + */ + emitNodes: TmplAstNode[]; + + /** + * The template AST, parsed in a manner which preserves source map information for diagnostics. + * + * Not useful for emit. + */ + diagNodes: TmplAstNode[]; + + /** + * */ - nodes: TmplAstNode[]; /** * Any styleUrls extracted from the metadata. @@ -863,7 +860,6 @@ export interface ParsedTemplate { file: ParseSourceFile; } -interface PreanalyzedTemplate extends ParsedTemplate { - parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; - templateSourceMapping: TemplateSourceMapping; +export interface ParsedTemplateWithSource extends ParsedTemplate { + sourceMapping: TemplateSourceMapping; }