From 024e3b30ac4d87bb2350c9502d07ac5a6278fcc9 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 15 Dec 2019 15:12:14 +0100 Subject: [PATCH] refactor(ivy): cleanup translation of source spans in type checker (#34417) This commit cleans up the template type checker regarding how diagnostics are produced. PR Close #34417 --- .../src/ngtsc/typecheck/src/api.ts | 4 +- .../src/ngtsc/typecheck/src/context.ts | 4 +- .../src/ngtsc/typecheck/src/diagnostics.ts | 74 ++++++++----------- .../src/ngtsc/typecheck/src/dom.ts | 9 ++- .../src/ngtsc/typecheck/src/oob.ts | 25 +++---- .../src/ngtsc/typecheck/src/source.ts | 30 ++++---- .../ngtsc/typecheck/src/template_semantics.ts | 6 +- .../ngtsc/typecheck/src/type_check_block.ts | 8 +- .../src/ngtsc/typecheck/test/test_utils.ts | 5 +- 9 files changed, 80 insertions(+), 85 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts index 977943bf02..d5ccb7785e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts @@ -25,6 +25,8 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta { hasNgTemplateContextGuard: boolean; } +export type TemplateId = string & {__brand: 'TemplateId'}; + /** * Metadata required in addition to a component class in order to generate a type check block (TCB) * for that component. @@ -35,7 +37,7 @@ export interface TypeCheckBlockMetadata { * * This can be used to map errors back to the `ts.ClassDeclaration` for the component. */ - id: string; + id: TemplateId; /** * Semantic information about the template of the component. diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 92aa179984..57d8e63933 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -20,7 +20,7 @@ import {DomSchemaChecker, RegistryDomSchemaChecker} from './dom'; import {Environment} from './environment'; import {TypeCheckProgramHost} from './host'; import {OutOfBandDiagnosticRecorder, OutOfBandDiagnosticRecorderImpl} from './oob'; -import {TcbSourceManager} from './source'; +import {TemplateSourceManager} from './source'; import {generateTypeCheckBlock, requiresInlineTypeCheckBlock} from './type_check_block'; import {TypeCheckFile} from './type_check_file'; import {generateInlineTypeCtor, requiresInlineTypeCtor} from './type_constructor'; @@ -55,7 +55,7 @@ export class TypeCheckContext { */ private typeCtorPending = new Set(); - private sourceManager = new TcbSourceManager(); + private sourceManager = new TemplateSourceManager(); private domSchemaChecker = new RegistryDomSchemaChecker(this.sourceManager); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts index c1cc9df1f7..3ed958274b 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts @@ -10,34 +10,26 @@ import * as ts from 'typescript'; import {getTokenAtPosition} from '../../util/src/typescript'; -import {ExternalTemplateSourceMapping, TemplateSourceMapping} from './api'; +import {ExternalTemplateSourceMapping, TemplateId, TemplateSourceMapping} from './api'; -export interface SourceLocation { - id: string; - start: number; - end: number; -} /** * Adapter interface which allows the template type-checking diagnostics code to interpret offsets * in a TCB and map them back to original locations in the template. */ -export interface TcbSourceResolver { +export interface TemplateSourceResolver { /** * For the given template id, retrieve the original source mapping which describes how the offsets * in the template should be interpreted. */ - getSourceMapping(id: string): TemplateSourceMapping; + getSourceMapping(id: TemplateId): TemplateSourceMapping; /** - * Convert a location extracted from a TCB into a `ParseSourceSpan` if possible. + * Convert an absolute source span associated with the given template id into a full + * `ParseSourceSpan`. The returned parse span has line and column numbers in addition to only + * absolute offsets and gives access to the original template source. */ - sourceLocationToSpan(location: SourceLocation): ParseSourceSpan|null; -} - -export function absoluteSourceSpanToSourceLocation( - id: string, span: AbsoluteSourceSpan): SourceLocation { - return {id, ...span}; + toParseSourceSpan(id: TemplateId, span: AbsoluteSourceSpan): ParseSourceSpan|null; } /** @@ -70,10 +62,10 @@ export function addParseSpanInfo(node: ts.Node, span: AbsoluteSourceSpan | Parse } /** - * Adds a synthetic comment to the function declaration that contains the source location + * Adds a synthetic comment to the function declaration that contains the template id * of the class declaration. */ -export function addSourceId(tcb: ts.FunctionDeclaration, id: string): void { +export function addTemplateId(tcb: ts.FunctionDeclaration, id: TemplateId): void { ts.addSyntheticLeadingComment(tcb, ts.SyntaxKind.MultiLineCommentTrivia, id, true); } @@ -105,7 +97,7 @@ export function shouldReportDiagnostic(diagnostic: ts.Diagnostic): boolean { * file from being reported as type-check errors. */ export function translateDiagnostic( - diagnostic: ts.Diagnostic, resolver: TcbSourceResolver): ts.Diagnostic|null { + diagnostic: ts.Diagnostic, resolver: TemplateSourceResolver): ts.Diagnostic|null { if (diagnostic.file === undefined || diagnostic.start === undefined) { return null; } @@ -118,7 +110,7 @@ export function translateDiagnostic( } // Now use the external resolver to obtain the full `ParseSourceFile` of the template. - const span = resolver.sourceLocationToSpan(sourceLocation); + const span = resolver.toParseSourceSpan(sourceLocation.id, sourceLocation.span); if (span === null) { return null; } @@ -217,10 +209,15 @@ export function makeTemplateDiagnostic( } } +interface SourceLocation { + id: TemplateId; + span: AbsoluteSourceSpan; +} + function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|null { // Search for comments until the TCB's function declaration is encountered. while (node !== undefined && !ts.isFunctionDeclaration(node)) { - const sourceSpan = + const span = ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => { if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { return null; @@ -228,10 +225,14 @@ function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLoc const commentText = sourceFile.text.substring(pos, end); return parseSourceSpanComment(commentText); }) || null; - if (sourceSpan !== null) { + if (span !== null) { // Once the positional information has been extracted, search further up the TCB to extract - // the file information that is attached with the TCB's function declaration. - return toSourceLocation(sourceSpan, node, sourceFile); + // the unique id that is attached with the TCB's function declaration. + const id = getTemplateId(node, sourceFile); + if (id === null) { + return null; + } + return {id, span}; } node = node.parent; @@ -240,8 +241,7 @@ function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLoc return null; } -function toSourceLocation( - sourceSpan: AbsoluteSourceSpan, node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|null { +function getTemplateId(node: ts.Node, sourceFile: ts.SourceFile): TemplateId|null { // Walk up to the function declaration of the TCB, the file information is attached there. let tcb = node; while (!ts.isFunctionDeclaration(tcb)) { @@ -253,23 +253,13 @@ function toSourceLocation( } } - const id = - ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => { - if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { - return null; - } - const commentText = sourceFile.text.substring(pos, end); - return commentText.substring(2, commentText.length - 2); - }) || null; - if (id === null) { - return null; - } - - return { - id, - start: sourceSpan.start, - end: sourceSpan.end, - }; + return ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => { + if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { + return null; + } + const commentText = sourceFile.text.substring(pos + 2, end - 2); + return commentText; + }) as TemplateId || null; } const parseSpanComment = /^\/\*(\d+),(\d+)\*\/$/; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts index c50a727bc7..48733b5d9e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts @@ -11,7 +11,8 @@ import * as ts from 'typescript'; import {ErrorCode} from '../../diagnostics'; -import {TcbSourceResolver, makeTemplateDiagnostic} from './diagnostics'; +import {TemplateId} from './api'; +import {TemplateSourceResolver, makeTemplateDiagnostic} from './diagnostics'; const REGISTRY = new DomElementSchemaRegistry(); const REMOVE_XHTML_REGEX = /^:xhtml:/; @@ -67,9 +68,9 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker { get diagnostics(): ReadonlyArray { return this._diagnostics; } - constructor(private resolver: TcbSourceResolver) {} + constructor(private resolver: TemplateSourceResolver) {} - checkElement(id: string, element: TmplAstElement, schemas: SchemaMetadata[]): void { + checkElement(id: TemplateId, element: TmplAstElement, schemas: SchemaMetadata[]): void { // HTML elements inside an SVG `foreignObject` are declared in the `xhtml` namespace. // We need to strip it before handing it over to the registry because all HTML tag names // in the registry are without a namespace. @@ -97,7 +98,7 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker { } checkProperty( - id: string, element: TmplAstElement, name: string, span: ParseSourceSpan, + id: TemplateId, element: TmplAstElement, name: string, span: ParseSourceSpan, schemas: SchemaMetadata[]): void { if (!REGISTRY.hasProperty(element.name, name, schemas)) { const mapping = this.resolver.getSourceMapping(id); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index 8b22433bbb..58096e4fdb 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -6,12 +6,13 @@ * found in the LICENSE file at https://angular.io/license */ -import {AbsoluteSourceSpan, BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler'; +import {BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler'; import * as ts from 'typescript'; import {ErrorCode, ngErrorCode} from '../../diagnostics'; -import {TcbSourceResolver, absoluteSourceSpanToSourceLocation, makeTemplateDiagnostic} from './diagnostics'; +import {TemplateId} from './api'; +import {TemplateSourceResolver, makeTemplateDiagnostic} from './diagnostics'; @@ -35,7 +36,7 @@ export interface OutOfBandDiagnosticRecorder { * reference. * @param ref the `TmplAstReference` which could not be matched to a directive. */ - missingReferenceTarget(templateId: string, ref: TmplAstReference): void; + missingReferenceTarget(templateId: TemplateId, ref: TmplAstReference): void; /** * Reports usage of a `| pipe` expression in the template for which the named pipe could not be @@ -45,20 +46,20 @@ export interface OutOfBandDiagnosticRecorder { * pipe. * @param ast the `BindingPipe` invocation of the pipe which could not be found. */ - missingPipe(templateId: string, ast: BindingPipe): void; + missingPipe(templateId: TemplateId, ast: BindingPipe): void; illegalAssignmentToTemplateVar( - templateId: string, assignment: PropertyWrite, target: TmplAstVariable): void; + templateId: TemplateId, assignment: PropertyWrite, target: TmplAstVariable): void; } export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder { private _diagnostics: ts.Diagnostic[] = []; - constructor(private resolver: TcbSourceResolver) {} + constructor(private resolver: TemplateSourceResolver) {} get diagnostics(): ReadonlyArray { return this._diagnostics; } - missingReferenceTarget(templateId: string, ref: TmplAstReference): void { + missingReferenceTarget(templateId: TemplateId, ref: TmplAstReference): void { const mapping = this.resolver.getSourceMapping(templateId); const value = ref.value.trim(); @@ -68,12 +69,11 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor ngErrorCode(ErrorCode.MISSING_REFERENCE_TARGET), errorMsg)); } - missingPipe(templateId: string, ast: BindingPipe): void { + missingPipe(templateId: TemplateId, ast: BindingPipe): void { const mapping = this.resolver.getSourceMapping(templateId); const errorMsg = `No pipe found with name '${ast.name}'.`; - const location = absoluteSourceSpanToSourceLocation(templateId, ast.nameSpan); - const sourceSpan = this.resolver.sourceLocationToSpan(location); + const sourceSpan = this.resolver.toParseSourceSpan(templateId, ast.nameSpan); if (sourceSpan === null) { throw new Error( `Assertion failure: no SourceLocation found for usage of pipe '${ast.name}'.`); @@ -84,13 +84,12 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor } illegalAssignmentToTemplateVar( - templateId: string, assignment: PropertyWrite, target: TmplAstVariable): void { + templateId: TemplateId, assignment: PropertyWrite, target: TmplAstVariable): void { const mapping = this.resolver.getSourceMapping(templateId); const errorMsg = `Cannot use variable '${assignment.name}' as the left-hand side of an assignment expression. Template variables are read-only.`; - const location = absoluteSourceSpanToSourceLocation(templateId, assignment.sourceSpan); - const sourceSpan = this.resolver.sourceLocationToSpan(location); + const sourceSpan = this.resolver.toParseSourceSpan(templateId, assignment.sourceSpan); if (sourceSpan === null) { throw new Error(`Assertion failure: no SourceLocation found for property binding.`); } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/source.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/source.ts index fa2f8d4e0e..22ea319fff 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/source.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/source.ts @@ -6,10 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ -import {ParseLocation, ParseSourceFile, ParseSourceSpan} from '@angular/compiler'; +import {AbsoluteSourceSpan, ParseLocation, ParseSourceFile, ParseSourceSpan} from '@angular/compiler'; -import {TemplateSourceMapping} from './api'; -import {SourceLocation, TcbSourceResolver} from './diagnostics'; +import {TemplateId, TemplateSourceMapping} from './api'; +import {TemplateSourceResolver} from './diagnostics'; import {computeLineStartsMap, getLineAndCharacterFromPosition} from './line_mappings'; /** @@ -44,35 +44,35 @@ export class TemplateSource { /** * Assigns IDs to templates and keeps track of their origins. * - * Implements `TcbSourceResolver` to resolve the source of a template based on these IDs. + * Implements `TemplateSourceResolver` to resolve the source of a template based on these IDs. */ -export class TcbSourceManager implements TcbSourceResolver { - private nextTcbId: number = 1; +export class TemplateSourceManager implements TemplateSourceResolver { + private nextTemplateId: number = 1; /** * This map keeps track of all template sources that have been type-checked by the id that is * attached to a TCB's function declaration as leading trivia. This enables translation of * diagnostics produced for TCB code to their source location in the template. */ - private templateSources = new Map(); + private templateSources = new Map(); - captureSource(mapping: TemplateSourceMapping, file: ParseSourceFile): string { - const id = `tcb${this.nextTcbId++}`; + captureSource(mapping: TemplateSourceMapping, file: ParseSourceFile): TemplateId { + const id = `tcb${this.nextTemplateId++}` as TemplateId; this.templateSources.set(id, new TemplateSource(mapping, file)); return id; } - getSourceMapping(id: string): TemplateSourceMapping { + getSourceMapping(id: TemplateId): TemplateSourceMapping { if (!this.templateSources.has(id)) { - throw new Error(`Unexpected unknown TCB ID: ${id}`); + throw new Error(`Unexpected unknown template ID: ${id}`); } return this.templateSources.get(id) !.mapping; } - sourceLocationToSpan(location: SourceLocation): ParseSourceSpan|null { - if (!this.templateSources.has(location.id)) { + toParseSourceSpan(id: TemplateId, span: AbsoluteSourceSpan): ParseSourceSpan|null { + if (!this.templateSources.has(id)) { return null; } - const templateSource = this.templateSources.get(location.id) !; - return templateSource.toParseSourceSpan(location.start, location.end); + const templateSource = this.templateSources.get(id) !; + return templateSource.toParseSourceSpan(span.start, span.end); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts index b9c0235a60..5cdfd53518 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_semantics.ts @@ -8,6 +8,7 @@ import {AST, BoundTarget, ImplicitReceiver, PropertyWrite, RecursiveAstVisitor, TmplAstVariable} from '@angular/compiler'; +import {TemplateId} from './api'; import {OutOfBandDiagnosticRecorder} from './oob'; /** @@ -15,7 +16,7 @@ import {OutOfBandDiagnosticRecorder} from './oob'; */ export class ExpressionSemanticVisitor extends RecursiveAstVisitor { constructor( - private templateId: string, private boundTarget: BoundTarget, + private templateId: TemplateId, private boundTarget: BoundTarget, private oob: OutOfBandDiagnosticRecorder) { super(); } @@ -35,7 +36,8 @@ export class ExpressionSemanticVisitor extends RecursiveAstVisitor { } static visit( - ast: AST, id: string, boundTarget: BoundTarget, oob: OutOfBandDiagnosticRecorder): void { + ast: AST, id: TemplateId, boundTarget: BoundTarget, + oob: OutOfBandDiagnosticRecorder): void { ast.visit(new ExpressionSemanticVisitor(id, boundTarget, oob)); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index 6d52d5afbd..b358ef3300 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -12,8 +12,8 @@ import * as ts from 'typescript'; import {Reference} from '../../imports'; import {ClassDeclaration} from '../../reflection'; -import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api'; -import {addParseSpanInfo, addSourceId, wrapForDiagnostics} from './diagnostics'; +import {TemplateId, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api'; +import {addParseSpanInfo, addTemplateId, wrapForDiagnostics} from './diagnostics'; import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; import {NULL_AS_ANY, astToTypescript} from './expression'; @@ -77,7 +77,7 @@ export function generateTypeCheckBlock( /* parameters */ paramList, /* type */ undefined, /* body */ body); - addSourceId(fnDecl, meta.id); + addTemplateId(fnDecl, meta.id); return fnDecl; } @@ -579,7 +579,7 @@ export class Context { constructor( readonly env: Environment, readonly domSchemaChecker: DomSchemaChecker, - readonly oobRecorder: OutOfBandDiagnosticRecorder, readonly id: string, + readonly oobRecorder: OutOfBandDiagnosticRecorder, readonly id: TemplateId, readonly boundTarget: BoundTarget, private pipes: Map>>, readonly schemas: SchemaMetadata[]) {} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index f52b16becb..f14c2c91f8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -15,7 +15,7 @@ import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, import {ClassDeclaration, TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; import {makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; -import {TemplateSourceMapping, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig} from '../src/api'; +import {TemplateId, TemplateSourceMapping, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig} from '../src/api'; import {TypeCheckContext} from '../src/context'; import {DomSchemaChecker} from '../src/dom'; import {Environment} from '../src/environment'; @@ -197,7 +197,8 @@ export function tcb( const binder = new R3TargetBinder(matcher); const boundTarget = binder.bind({template: nodes}); - const meta: TypeCheckBlockMetadata = {boundTarget, pipes, id: 'tcb', schemas: []}; + const id = 'tcb' as TemplateId; + const meta: TypeCheckBlockMetadata = {id, boundTarget, pipes, schemas: []}; config = config || { applyTemplateContextGuards: true,