feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics (angular#3…

…1952)

Historically, the Angular Compiler has produced both native TypeScript
diagnostics (called ts.Diagnostics) and its own internal Diagnostic format
(called an api.Diagnostic). This was done because TypeScript ts.Diagnostics
cannot be produced for files not in the ts.Program, and template type-
checking diagnostics are naturally produced for external .html template
files.

This design isn't optimal for several reasons:

1) Downstream tooling (such as the CLI) must support multiple formats of
diagnostics, adding to the maintenance burden.

2) ts.Diagnostics have gotten a lot better in recent releases, with support
for suggested changes, highlighting of the code in question, etc. None of
these changes have been of any benefit for api.Diagnostics, which have
continued to be reported in a very primitive fashion.

3) A future plugin model will not support anything but ts.Diagnostics, so
generating api.Diagnostics is a blocker for ngtsc-as-a-plugin.

4) The split complicates both the typings and the testing of ngtsc.

To fix this issue, this commit changes template type-checking to produce
ts.Diagnostics instead. Instead of reporting a special kind of diagnostic
for external template files, errors in a template are always reported in
a ts.Diagnostic that highlights the portion of the template which contains
the error. When this template text is distinct from the source .ts file
(for example, when the template is parsed from an external resource file),
additional contextual information links the error back to the originating
component.

A template error can thus be reported in 3 separate ways, depending on how
the template was configured:

1) For inline template strings which can be directly mapped to offsets in
the TS code, ts.Diagnostics point to real ranges in the source.

This is the case if an inline template is used with a string literal or a
"no-substitution" string. For example:

```typescript
@component({..., template: `
<p>Bar: {{baz}}</p>
`})
export class TestCmp {
  bar: string;
}
```

The above template contains an error (no 'baz' property of `TestCmp`). The
error produced by TS will look like:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts:2:11 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. Did you mean 'bar'?
```

2) For template strings which cannot be directly mapped to offsets in the
TS code, a logical offset into the template string will be included in
the error message. For example:

```typescript
const SOME_TEMPLATE = '<p>Bar: {{baz}}</p>';

@component({..., template: SOME_TEMPLATE})
export class TestCmp {
  bar: string;
}
```

Because the template is a reference to another variable and is not an
inline string constant, the compiler will not be able to use "absolute"
positions when parsing the template. As a result, errors will report logical
offsets into the template string:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts (TestCmp template):2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:3:28
    @component({..., template: TEMPLATE})
                               ~~~~~~~~

    Error occurs in the template of component TestCmp.
```

This error message uses logical offsets into the template string, and also
gives a reference to the `TEMPLATE` expression from which the template was
parsed. This helps in locating the component which contains the error.

3) For external templates (templateUrl), the error message is delivered
within the HTML template file (testcmp.html) instead, and additional
information contextualizes the error on the templateUrl expression from
which the template file was determined:

```
<p>Bar: {{baz}}</p>
          ~~~

testcmp.html:2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:10:31
    @component({..., templateUrl: './testcmp.html'})
                                  ~~~~~~~~~~~~~~~~

    Error occurs in the template of component TestCmp.
```

PR Close angular#31952

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

@@ -20,7 +20,7 @@ import {EnumValue, PartialEvaluator} from '../../partial_evaluator';
 import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
 import {ComponentScopeReader, LocalModuleScopeRegistry} from '../../scope';
 import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform';
-import {TypeCheckContext} from '../../typecheck';
+import {TemplateSourceMapping, TypeCheckContext} from '../../typecheck';
 import {NoopResourceDependencyRecorder, ResourceDependencyRecorder} from '../../util/src/resource_recorder';
 import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300';
 
@@ -34,7 +34,8 @@ const EMPTY_ARRAY: any[] = [];
 
 export interface ComponentHandlerData {
   meta: R3ComponentMetadata;
-  parsedTemplate: {nodes: TmplAstNode[]; file: ParseSourceFile};
+  parsedTemplate: ParsedTemplate;
+  templateSourceMapping: TemplateSourceMapping;
   metadataStmt: Statement|null;
   parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate;
 }
@@ -63,7 +64,7 @@ export class ComponentDecoratorHandler implements
    * any potential <link> 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<ts.Declaration, ParsedTemplate>();
+  private preanalyzeTemplateCache = new Map<ts.Declaration, PreanalyzedTemplate>();
 
   readonly precedence = HandlerPrecedence.PRIMARY;
 
@@ -174,18 +175,22 @@ export class ComponentDecoratorHandler implements
     // 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;
     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) !;
       this.preanalyzeTemplateCache.delete(node);
 
-      // A pre-analyzed template cannot be reparsed. Pre-analysis is never run with the indexing
-      // pipeline.
-      parseTemplate = (options?: ParseTemplateOptions) => {
-        if (options !== undefined) {
-          throw new Error(`Cannot reparse a pre-analyzed template with new options`);
-        }
-        return template;
+      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,
       };
     } else {
       // The template was not already parsed. Either there's a templateUrl, or an inline template.
@@ -203,6 +208,13 @@ export class ComponentDecoratorHandler implements
         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,
+        };
       } else {
         // Expect an inline template to be present.
         const inlineTemplate = this._extractInlineTemplate(component, containingFile);
@@ -214,6 +226,20 @@ export class ComponentDecoratorHandler implements
         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 template = parseTemplate();
@@ -308,7 +334,7 @@ export class ComponentDecoratorHandler implements
         },
         metadataStmt: generateSetClassMetadataCall(
             node, this.reflector, this.defaultImportRecorder, this.isCore),
-        parsedTemplate: template, parseTemplate,
+        parsedTemplate: template, parseTemplate, templateSourceMapping,
       },
       typeCheck: true,
     };
@@ -354,8 +380,22 @@ export class ComponentDecoratorHandler implements
       return;
     }
 
-    const pipes = new Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>();
+    // 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<DirectiveMeta>();
+    const pipes = new Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>();
 
     const scope = this.scopeReader.getScopeForComponent(node);
     if (scope !== null) {
@@ -372,8 +412,8 @@ export class ComponentDecoratorHandler implements
       }
     }
 
-    const bound = new R3TargetBinder(matcher).bind({template: meta.parsedTemplate.nodes});
-    ctx.addTemplate(new Reference(node), bound, pipes, meta.parsedTemplate.file);
+    const bound = new R3TargetBinder(matcher).bind({template: template.nodes});
+    ctx.addTemplate(new Reference(node), bound, pipes, meta.templateSourceMapping, template.file);
   }
 
   resolve(node: ClassDeclaration, analysis: ComponentHandlerData): ResolveResult {
@@ -561,10 +601,12 @@ export class ComponentDecoratorHandler implements
         return templatePromise.then(() => {
           const templateStr = this.resourceLoader.load(resourceUrl);
           this.resourceDependencies.recordResourceDependency(node.getSourceFile(), resourceUrl);
-          const template = this._parseTemplate(
-              component, templateStr, sourceMapUrl(resourceUrl), /* templateRange */ undefined,
-              /* escapedString */ false);
-          this.preanalyzeTemplateCache.set(node, template);
+          const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate(
+              component, templateStr, sourceMapUrl(resourceUrl),
+              /* templateRange */ undefined,
+              /* escapedString */ false, options);
+          const template = parseTemplate();
+          this.preanalyzeTemplateCache.set(node, {...template, parseTemplate});
           return template;
         });
       } else {
@@ -579,9 +621,10 @@ export class ComponentDecoratorHandler implements
       }
 
       const {templateStr, templateUrl, escapedString, templateRange} = inlineTemplate;
-      const template =
-          this._parseTemplate(component, templateStr, templateUrl, templateRange, escapedString);
-      this.preanalyzeTemplateCache.set(node, template);
+      const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate(
+          component, templateStr, templateUrl, templateRange, escapedString, options);
+      const template = parseTemplate();
+      this.preanalyzeTemplateCache.set(node, {...template, parseTemplate});
       return Promise.resolve(template);
     }
   }
@@ -656,6 +699,7 @@ export class ComponentDecoratorHandler implements
         interpolationConfig: interpolation,
         range: templateRange, escapedString, ...options,
       }),
+      template: templateStr, templateUrl,
       isInline: component.has('template'),
       file: new ParseSourceFile(templateStr, templateUrl),
     };
@@ -713,12 +757,66 @@ function sourceMapUrl(resourceUrl: string): string {
   }
 }
 
-interface ParsedTemplate {
+
+/**
+ * Information about the template which was extracted during parsing.
+ *
+ * This contains the actual parsed template as well as any metadata collected during its parsing,
+ * some of which might be useful for re-parsing the template with different options.
+ */
+export interface ParsedTemplate {
+  /**
+   * The `InterpolationConfig` specified by the user.
+   */
   interpolation: InterpolationConfig;
+
+  /**
+   * A full path to the file which contains the template.
+   *
+   * This can be either the original .ts file if the template is inline, or the .html file if an
+   * external file was used.
+   */
+  templateUrl: string;
+
+  /**
+   * The string contents of the template.
+   *
+   * This is the "logical" template string, after expansion of any escaped characters (for inline
+   * templates). This may differ from the actual template bytes as they appear in the .ts file.
+   */
+  template: string;
+
+  /**
+   * Any errors from parsing the template the first time.
+   */
   errors?: ParseError[]|undefined;
+
+  /**
+   * The actual parsed template nodes.
+   */
   nodes: TmplAstNode[];
+
+  /**
+   * Any styleUrls extracted from the metadata.
+   */
   styleUrls: string[];
+
+  /**
+   * Any inline styles extracted from the metadata.
+   */
   styles: string[];
+
+  /**
+   * Whether the template was inline.
+   */
   isInline: boolean;
+
+  /**
+   * The `ParseSourceFile` for the template.
+   */
   file: ParseSourceFile;
 }
+
+interface PreanalyzedTemplate extends ParsedTemplate {
+  parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate;
+}

packages/compiler-cli/src/ngtsc/program.ts

@@ -175,7 +175,7 @@ export class NgtscProgram implements api.Program {
   }
 
   getNgOptionDiagnostics(cancellationToken?: ts.CancellationToken|
-                         undefined): ReadonlyArray<ts.Diagnostic|api.Diagnostic> {
+                         undefined): ReadonlyArray<ts.Diagnostic> {
     return this.constructionDiagnostics;
   }
 
@@ -197,8 +197,8 @@ export class NgtscProgram implements api.Program {
   }
 
   getNgSemanticDiagnostics(
-      fileName?: string|undefined, cancellationToken?: ts.CancellationToken|
-                                   undefined): ReadonlyArray<ts.Diagnostic|api.Diagnostic> {
+      fileName?: string|undefined,
+      cancellationToken?: ts.CancellationToken|undefined): ReadonlyArray<ts.Diagnostic> {
     const compilation = this.ensureAnalyzed();
     const diagnostics = [...compilation.diagnostics, ...this.getTemplateDiagnostics()];
     if (this.entryPoint !== null && this.exportReferenceGraph !== null) {
@@ -381,7 +381,7 @@ export class NgtscProgram implements api.Program {
     return ((opts && opts.mergeEmitResultsCallback) || mergeEmitResults)(emitResults);
   }
 
-  private getTemplateDiagnostics(): ReadonlyArray<api.Diagnostic|ts.Diagnostic> {
+  private getTemplateDiagnostics(): ReadonlyArray<ts.Diagnostic> {
     // Skip template type-checking if it's disabled.
     if (this.options.ivyTemplateTypeCheck === false &&
         this.options.fullTemplateTypeCheck !== true) {

packages/compiler-cli/src/ngtsc/typecheck/src/api.ts

@@ -29,6 +29,13 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta {
  * for that component.
  */
 export interface TypeCheckBlockMetadata {
+  /**
+   * A unique identifier for the class which gave rise to this TCB.
+   *
+   * This can be used to map errors back to the `ts.ClassDeclaration` for the component.
+   */
+  id: string;
+
   /**
    * Semantic information about the template of the component.
    */
@@ -104,3 +111,47 @@ export interface TypeCheckingConfig {
    */
   checkQueries: false;
 }
+
+
+export type TemplateSourceMapping =
+    DirectTemplateSourceMapping | IndirectTemplateSourceMapping | ExternalTemplateSourceMapping;
+
+/**
+ * A mapping to an inline template in a TS file.
+ *
+ * `ParseSourceSpan`s for this template should be accurate for direct reporting in a TS error
+ * message.
+ */
+export interface DirectTemplateSourceMapping {
+  type: 'direct';
+  node: ts.StringLiteral|ts.NoSubstitutionTemplateLiteral;
+}
+
+/**
+ * A mapping to a template which is still in a TS file, but where the node positions in any
+ * `ParseSourceSpan`s are not accurate for one reason or another.
+ *
+ * This can occur if the template expression was interpolated in a way where the compiler could not
+ * construct a contiguous mapping for the template string. The `node` refers to the `template`
+ * expression.
+ */
+export interface IndirectTemplateSourceMapping {
+  type: 'indirect';
+  componentClass: ClassDeclaration;
+  node: ts.Expression;
+  template: string;
+}
+
+/**
+ * A mapping to a template declared in an external HTML file, where node positions in
+ * `ParseSourceSpan`s represent accurate offsets into the external file.
+ *
+ * In this case, the given `node` refers to the `templateUrl` expression.
+ */
+export interface ExternalTemplateSourceMapping {
+  type: 'external';
+  componentClass: ClassDeclaration;
+  node: ts.Expression;
+  template: string;
+  templateUrl: string;
+}