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

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.

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<ts.ClassDeclaration>();
 
-  private sourceManager = new TcbSourceManager();
+  private sourceManager = new TemplateSourceManager();
 
   private domSchemaChecker = new RegistryDomSchemaChecker(this.sourceManager);
 

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;
+  toParseSourceSpan(id: TemplateId, span: AbsoluteSourceSpan): ParseSourceSpan|null;
-}
-
-export function absoluteSourceSpanToSourceLocation(
-    id: string, span: AbsoluteSourceSpan): SourceLocation {
-  return {id, ...span};
 }
 
 /**
@@ -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.
+      // the unique id that is attached with the TCB's function declaration.
-      return toSourceLocation(sourceSpan, node, sourceFile);
+      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(
+function getTemplateId(node: ts.Node, sourceFile: ts.SourceFile): TemplateId|null {
-    sourceSpan: AbsoluteSourceSpan, node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|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 =
+  return ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => {
-      ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => {
+    if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
-        if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
+      return null;
-          return null;
+    }
-        }
+    const commentText = sourceFile.text.substring(pos + 2, end - 2);
-        const commentText = sourceFile.text.substring(pos, end);
+    return commentText;
-        return commentText.substring(2, commentText.length - 2);
+  }) as TemplateId || null;
-      }) || null;
-  if (id === null) {
-    return null;
-  }
-
-  return {
-    id,
-    start: sourceSpan.start,
-    end: sourceSpan.end,
-  };
 }
 
 const parseSpanComment = /^\/\*(\d+),(\d+)\*\/$/;

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<ts.Diagnostic> { 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);

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<ts.Diagnostic> { 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.toParseSourceSpan(templateId, ast.nameSpan);
-    const sourceSpan = this.resolver.sourceLocationToSpan(location);
     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.toParseSourceSpan(templateId, assignment.sourceSpan);
-    const sourceSpan = this.resolver.sourceLocationToSpan(location);
     if (sourceSpan === null) {
       throw new Error(`Assertion failure: no SourceLocation found for property binding.`);
     }

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 {TemplateId, TemplateSourceMapping} from './api';
-import {SourceLocation, TcbSourceResolver} from './diagnostics';
+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 {
+export class TemplateSourceManager implements TemplateSourceResolver {
-  private nextTcbId: number = 1;
+  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<string, TemplateSource>();
+  private templateSources = new Map<TemplateId, TemplateSource>();
 
-  captureSource(mapping: TemplateSourceMapping, file: ParseSourceFile): string {
+  captureSource(mapping: TemplateSourceMapping, file: ParseSourceFile): TemplateId {
-    const id = `tcb${this.nextTcbId++}`;
+    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 {
+  toParseSourceSpan(id: TemplateId, span: AbsoluteSourceSpan): ParseSourceSpan|null {
-    if (!this.templateSources.has(location.id)) {
+    if (!this.templateSources.has(id)) {
       return null;
     }
-    const templateSource = this.templateSources.get(location.id) !;
+    const templateSource = this.templateSources.get(id) !;
-    return templateSource.toParseSourceSpan(location.start, location.end);
+    return templateSource.toParseSourceSpan(span.start, span.end);
   }
 }

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<any>,
+      private templateId: TemplateId, private boundTarget: BoundTarget<any>,
       private oob: OutOfBandDiagnosticRecorder) {
     super();
   }
@@ -35,7 +36,8 @@ export class ExpressionSemanticVisitor extends RecursiveAstVisitor {
   }
 
   static visit(
-      ast: AST, id: string, boundTarget: BoundTarget<any>, oob: OutOfBandDiagnosticRecorder): void {
+      ast: AST, id: TemplateId, boundTarget: BoundTarget<any>,
+      oob: OutOfBandDiagnosticRecorder): void {
     ast.visit(new ExpressionSemanticVisitor(id, boundTarget, oob));
   }
 }

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 {TemplateId, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api';
-import {addParseSpanInfo, addSourceId, wrapForDiagnostics} from './diagnostics';
+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<TypeCheckableDirectiveMeta>,
       private pipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>,
       readonly schemas: SchemaMetadata[]) {}

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,