Skip to content

Commit

Permalink
fix(ivy): capture template source mapping details during preanalysis (#…
Browse files Browse the repository at this point in the history 
…32544)

Prior to this change, the template source mapping details were always
built during the analysis phase, under the assumption that pre-analysed
templates would always correspond with external templates. This has
turned out to be a false assumption, as inline templates are also
pre-analyzed to be able to preload any stylesheets included in the
template.

This commit fixes the bug by capturing the template source mapping
details at the moment the template is parsed, which is either during the
preanalysis phase when preloading is available, or during the analysis
phase when preloading is not supported.

Tests have been added to exercise the template error mapping in
asynchronous compilations where preloading is enabled, similar to how
the CLI performs compilations.

Fixes #32538

PR Close #32544
  • Loading branch information
@JoostK @matsko
JoostK authored and matsko committed Sep 9, 2019
1 parent a391aeb commit a64eded
Show file tree
Hide file tree
Showing 5 changed files with 141 additions and 112 deletions.
150 changes: 72 additions & 78 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
View file
Expand Up @@ -180,67 +180,32 @@ export class ComponentDecoratorHandler implements
let templateSourceMapping: TemplateSourceMapping;
if (this.preanalyzeTemplateCache.has(node)) {
// The template was parsed in preanalyze. Use it and delete it to save memory.
const template = this.preanalyzeTemplateCache.get(node) !;
const preanalyzed = this.preanalyzeTemplateCache.get(node) !;
this.preanalyzeTemplateCache.delete(node);

parseTemplate = template.parseTemplate;

// A pre-analyzed template is always an external mapping.
templateSourceMapping = {
type: 'external',
componentClass: node,
node: component.get('templateUrl') !,
template: template.template,
templateUrl: template.templateUrl,
};
parseTemplate = preanalyzed.parseTemplate;
templateSourceMapping = preanalyzed.templateSourceMapping;
} else {
// The template was not already parsed. Either there's a templateUrl, or an inline template.
if (component.has('templateUrl')) {
const templateUrlExpr = component.get('templateUrl') !;
const evalTemplateUrl = this.evaluator.evaluate(templateUrlExpr);
if (typeof evalTemplateUrl !== 'string') {
const templateUrl = this.evaluator.evaluate(templateUrlExpr);
if (typeof templateUrl !== 'string') {
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, templateUrlExpr, 'templateUrl must be a string');
}
const templateUrl = this.resourceLoader.resolve(evalTemplateUrl, containingFile);
const templateStr = this.resourceLoader.load(templateUrl);
this.resourceDependencies.recordResourceDependency(node.getSourceFile(), templateUrl);

parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate(
component, templateStr, sourceMapUrl(templateUrl), /* templateRange */ undefined,
/* escapedString */ false, options);
templateSourceMapping = {
type: 'external',
componentClass: node,
node: templateUrlExpr,
template: templateStr,
templateUrl: templateUrl,
};
const resourceUrl = this.resourceLoader.resolve(templateUrl, containingFile);
const external =
this._extractExternalTemplate(node, component, templateUrlExpr, resourceUrl);

parseTemplate = external.parseTemplate;
templateSourceMapping = external.templateSourceMapping;
} else {
// Expect an inline template to be present.
const inlineTemplate = this._extractInlineTemplate(component, containingFile);
if (inlineTemplate === null) {
throw new FatalDiagnosticError(
ErrorCode.COMPONENT_MISSING_TEMPLATE, decorator.node,
'component is missing a template');
}
const {templateStr, templateUrl, templateRange, escapedString} = inlineTemplate;
parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate(
component, templateStr, templateUrl, templateRange, escapedString, options);
if (escapedString) {
templateSourceMapping = {
type: 'direct',
node:
component.get('template') !as(ts.StringLiteral | ts.NoSubstitutionTemplateLiteral),
};
} else {
templateSourceMapping = {
type: 'indirect',
node: component.get('template') !,
componentClass: node,
template: templateStr,
};
}
const inline = this._extractInlineTemplate(node, decorator, component, containingFile);

parseTemplate = inline.parseTemplate;
templateSourceMapping = inline.templateSourceMapping;
}
}
const template = parseTemplate();
Expand Down Expand Up @@ -589,7 +554,7 @@ export class ComponentDecoratorHandler implements
}

private _preloadAndParseTemplate(
node: ts.Declaration, decorator: Decorator, component: Map<string, ts.Expression>,
node: ClassDeclaration, decorator: Decorator, component: Map<string, ts.Expression>,
containingFile: string): Promise<ParsedTemplate|null> {
if (component.has('templateUrl')) {
// Extract the templateUrl and preload it.
Expand All @@ -606,50 +571,64 @@ export class ComponentDecoratorHandler implements
// URLs to resolve.
if (templatePromise !== undefined) {
return templatePromise.then(() => {
const templateStr = this.resourceLoader.load(resourceUrl);
this.resourceDependencies.recordResourceDependency(node.getSourceFile(), resourceUrl);
const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate(
component, templateStr, sourceMapUrl(resourceUrl),
/* templateRange */ undefined,
/* escapedString */ false, options);
const {parseTemplate, templateSourceMapping} =
this._extractExternalTemplate(node, component, templateUrlExpr, resourceUrl);
const template = parseTemplate();
this.preanalyzeTemplateCache.set(node, {...template, parseTemplate});
this.preanalyzeTemplateCache.set(
node, {...template, parseTemplate, templateSourceMapping});
return template;
});
} else {
return Promise.resolve(null);
}
} else {
const inlineTemplate = this._extractInlineTemplate(component, containingFile);
if (inlineTemplate === null) {
throw new FatalDiagnosticError(
ErrorCode.COMPONENT_MISSING_TEMPLATE, decorator.node,
'component is missing a template');
}

const {templateStr, templateUrl, escapedString, templateRange} = inlineTemplate;
const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate(
component, templateStr, templateUrl, templateRange, escapedString, options);
const {parseTemplate, templateSourceMapping} =
this._extractInlineTemplate(node, decorator, component, containingFile);
const template = parseTemplate();
this.preanalyzeTemplateCache.set(node, {...template, parseTemplate});
this.preanalyzeTemplateCache.set(node, {...template, parseTemplate, templateSourceMapping});
return Promise.resolve(template);
}
}

private _extractInlineTemplate(component: Map<string, ts.Expression>, containingFile: string): {
templateStr: string,
templateUrl: string,
templateRange: LexerRange|undefined,
escapedString: boolean
}|null {
// If there is no inline template, then return null.
private _extractExternalTemplate(
node: ClassDeclaration, component: Map<string, ts.Expression>, templateUrlExpr: ts.Expression,
resourceUrl: string): {
parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate;
templateSourceMapping: TemplateSourceMapping
} {
const templateStr = this.resourceLoader.load(resourceUrl);
this.resourceDependencies.recordResourceDependency(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};
}

private _extractInlineTemplate(
node: ClassDeclaration, decorator: Decorator, component: Map<string, ts.Expression>,
containingFile: string): {
parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate;
templateSourceMapping: TemplateSourceMapping
} {
if (!component.has('template')) {
return null;
throw new FatalDiagnosticError(
ErrorCode.COMPONENT_MISSING_TEMPLATE, decorator.node, 'component is missing a template');
}
const templateExpr = component.get('template') !;

let templateStr: string;
let templateUrl: string = '';
let templateRange: LexerRange|undefined = undefined;
let templateSourceMapping: TemplateSourceMapping;
let escapedString = false;
// We only support SourceMaps for inline templates that are simple string literals.
if (ts.isStringLiteral(templateExpr) || ts.isNoSubstitutionTemplateLiteral(templateExpr)) {
Expand All @@ -660,15 +639,29 @@ export class ComponentDecoratorHandler implements
templateStr = templateExpr.getSourceFile().text;
templateUrl = containingFile;
escapedString = true;
templateSourceMapping = {
type: 'direct',
node: templateExpr as(ts.StringLiteral | ts.NoSubstitutionTemplateLiteral),
};
} else {
const resolvedTemplate = this.evaluator.evaluate(templateExpr);
if (typeof resolvedTemplate !== 'string') {
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, templateExpr, 'template must be a string');
}
templateStr = resolvedTemplate;
templateSourceMapping = {
type: 'indirect',
node: templateExpr,
componentClass: node,
template: templateStr,
};
}
return {templateStr, templateUrl, templateRange, escapedString};

const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate(
component, templateStr, templateUrl, templateRange, escapedString, options);

return {parseTemplate, templateSourceMapping};
}

private _parseTemplate(
Expand Down Expand Up @@ -826,4 +819,5 @@ export interface ParsedTemplate {

interface PreanalyzedTemplate extends ParsedTemplate {
parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate;
templateSourceMapping: TemplateSourceMapping;
}
7 changes: 0 additions & 7 deletions packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts
View file
Expand Up @@ -141,13 +141,6 @@ export function translateDiagnostic(
return null;
}

let messageText: string;
if (typeof diagnostic.messageText === 'string') {
messageText = diagnostic.messageText;
} else {
messageText = diagnostic.messageText.messageText;
}

const mapping = resolver.getSourceMapping(sourceLocation.id);
return makeTemplateDiagnostic(
mapping, span, diagnostic.category, diagnostic.code, diagnostic.messageText);
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/perform_compile.ts
View file
Expand Up @@ -283,7 +283,7 @@ export function performCompilation(
return {diagnostics: allDiagnostics, program};
}
}
function defaultGatherDiagnostics(program: api.Program): Diagnostics {
export function defaultGatherDiagnostics(program: api.Program): Diagnostics {
const allDiagnostics: Array<ts.Diagnostic|api.Diagnostic> = [];

function checkDiagnostics(diags: Diagnostics | undefined) {
Expand Down
31 changes: 30 additions & 1 deletion packages/compiler-cli/test/ngtsc/env.ts
View file
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {CustomTransformers, Program} from '@angular/compiler-cli';
import {CustomTransformers, Program, defaultGatherDiagnostics} from '@angular/compiler-cli';
import * as api from '@angular/compiler-cli/src/transformers/api';
import * as ts from 'typescript';

Expand Down Expand Up @@ -100,6 +100,15 @@ export class NgtscTestEnvironment {
setWrapHostForTest(makeWrapHost(this.multiCompileHostExt));
}

/**
* Installs a compiler host that allows for asynchronous reading of resources by implementing the
* `CompilerHost.readResource` method. Note that only asynchronous compilations are affected, as
* synchronous compilations do not use the asynchronous resource loader.
*/
enablePreloading(): void {
setWrapHostForTest(makeWrapHost(new ResourceLoadingCompileHost(this.fs)));
}

flushWrittenFileTracking(): void {
if (this.multiCompileHostExt === null) {
throw new Error(`Not tracking written files - call enableMultipleCompilations()`);
Expand Down Expand Up @@ -192,6 +201,16 @@ export class NgtscTestEnvironment {
return mainDiagnosticsForTest(['-p', this.basePath]) as ts.Diagnostic[];
}

async driveDiagnosticsAsync(): Promise<ReadonlyArray<ts.Diagnostic>> {
const {rootNames, options} = readNgcCommandLineAndConfiguration(['-p', this.basePath]);
const host = createCompilerHost({options});
const program = createProgram({rootNames, host, options});
await program.loadNgStructureAsync();

// ngtsc only produces ts.Diagnostic messages.
return defaultGatherDiagnostics(program as api.Program) as ts.Diagnostic[];
}

driveRoutes(entryPoint?: string): LazyRoute[] {
const {rootNames, options} = readNgcCommandLineAndConfiguration(['-p', this.basePath]);
const host = createCompilerHost({options});
Expand Down Expand Up @@ -251,6 +270,16 @@ class MultiCompileHostExt extends AugmentedCompilerHost implements Partial<ts.Co
invalidate(fileName: string): void { this.cache.delete(fileName); }
}

class ResourceLoadingCompileHost extends AugmentedCompilerHost implements api.CompilerHost {
readResource(fileName: string): Promise<string>|string {
const resource = this.readFile(fileName);
if (resource === undefined) {
throw new Error(`Resource ${fileName} not found`);
}
return resource;
}
}

function makeWrapHost(wrapped: AugmentedCompilerHost): (host: ts.CompilerHost) => ts.CompilerHost {
return (delegate) => {
wrapped.delegate = delegate;
Expand Down
63 changes: 38 additions & 25 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View file
Expand Up @@ -469,9 +469,21 @@ export declare class CommonModule {
});
});

describe('error locations', () => {
it('should be correct for direct templates', () => {
env.write('test.ts', `
// Test both sync and async compilations, see https://github.com/angular/angular/issues/32538
['sync', 'async'].forEach(mode => {
describe(`error locations [${mode}]`, () => {
let driveDiagnostics: () => Promise<ReadonlyArray<ts.Diagnostic>>;
beforeEach(() => {
if (mode === 'async') {
env.enablePreloading();
driveDiagnostics = () => env.driveDiagnosticsAsync();
} else {
driveDiagnostics = () => Promise.resolve(env.driveDiagnostics());
}
});

it('should be correct for direct templates', async() => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
Expand All @@ -484,14 +496,14 @@ export declare class CommonModule {
user: {name: string}[];
}`);

const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/test.ts'));
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
});
const diags = await driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/test.ts'));
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
});

it('should be correct for indirect templates', () => {
env.write('test.ts', `
it('should be correct for indirect templates', async() => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
const TEMPLATE = \`<p>
Expand All @@ -506,18 +518,18 @@ export declare class CommonModule {
user: {name: string}[];
}`);

const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/test.ts') + ' (TestCmp template)');
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0])).toBe('TEMPLATE');
});
const diags = await driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/test.ts') + ' (TestCmp template)');
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0])).toBe('TEMPLATE');
});

it('should be correct for external templates', () => {
env.write('template.html', `<p>
it('should be correct for external templates', async() => {
env.write('template.html', `<p>
{{user.does_not_exist}}
</p>`);
env.write('test.ts', `
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
Expand All @@ -529,12 +541,13 @@ export declare class CommonModule {
user: {name: string}[];
}`);

const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/template.html'));
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0]))
.toBe(`'./template.html'`);
const diags = await driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/template.html'));
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0]))
.toBe(`'./template.html'`);
});
});
});
});
Expand Down

0 comments on commit a64eded

Please sign in to comment.