From c90eb5450d8e2089145f1c9506fb787760e5d682 Mon Sep 17 00:00:00 2001
From: Alex Rickabaugh <alx+alxhub@alxandria.net>
Date: Tue, 25 Aug 2020 09:46:40 -0400
Subject: [PATCH] refactor(compiler-cli): make template parsing errors into
 diagnostics (#38576)

Previously, the compiler was not able to display template parsing errors as
true `ts.Diagnostic`s that point inside the template. Instead, it would
throw an actual `Error`, and "crash" with a stack trace containing the
template errors.

Not only is this a poor user experience, but it causes the Language Service
to also crash as the user is editing a template (in actuality the LS has to
work around this bug).

With this commit, such parsing errors are converted to true template
diagnostics with appropriate span information to be displayed contextually
along with all other diagnostics. This majorly improves the user experience
and unblocks the Language Service from having to deal with the compiler
"crashing" to report errors.

PR Close #38576
---
 .../src/ngtsc/annotations/BUILD.bazel         |  1 +
 .../src/ngtsc/annotations/src/component.ts    | 25 ++++++++--
 .../src/ngtsc/diagnostics/src/error_code.ts   |  5 ++
 .../compiler-cli/test/ngtsc/ngtsc_spec.ts     | 48 +++++++++++++++++++
 .../src/render3/r3_template_transform.ts      |  6 ---
 packages/compiler/test/render3/view/util.ts   |  9 +++-
 6 files changed, 84 insertions(+), 10 deletions(-)

diff --git a/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel b/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel
index b2687e727d..7e089a796f 100644
--- a/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel
+++ b/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel
@@ -23,6 +23,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/shims:api",
         "//packages/compiler-cli/src/ngtsc/transform",
         "//packages/compiler-cli/src/ngtsc/typecheck/api",
+        "//packages/compiler-cli/src/ngtsc/typecheck/diagnostics",
         "//packages/compiler-cli/src/ngtsc/util",
         "@npm//@types/node",
         "@npm//typescript",
diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts
index 93f9061144..964aaebf87 100644
--- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts
+++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts
@@ -10,7 +10,7 @@ import {compileComponentFromMetadata, ConstantPool, CssSelector, DEFAULT_INTERPO
 import * as ts from 'typescript';
 
 import {CycleAnalyzer} from '../../cycles';
-import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
+import {ErrorCode, FatalDiagnosticError, ngErrorCode} from '../../diagnostics';
 import {absoluteFrom, relative} from '../../file_system';
 import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports';
 import {DependencyTracker} from '../../incremental/api';
@@ -22,6 +22,7 @@ import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from
 import {ComponentScopeReader, LocalModuleScopeRegistry} from '../../scope';
 import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform';
 import {TemplateSourceMapping, TypeCheckContext} from '../../typecheck/api';
+import {getTemplateId, makeTemplateDiagnostic} from '../../typecheck/diagnostics';
 import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300';
 import {SubsetOfKeys} from '../../util/src/typescript';
 
@@ -254,9 +255,26 @@ export class ComponentDecoratorHandler implements
       }
     }
 
+    let diagnostics: ts.Diagnostic[]|undefined = undefined;
+
     if (template.errors !== undefined) {
-      throw new Error(
-          `Errors parsing template: ${template.errors.map(e => e.toString()).join(', ')}`);
+      // If there are any template parsing errors, convert them to `ts.Diagnostic`s for display.
+      const id = getTemplateId(node);
+      diagnostics = template.errors.map(error => {
+        const span = error.span;
+
+        if (span.start.offset === span.end.offset) {
+          // Template errors can contain zero-length spans, if the error occurs at a single point.
+          // However, TypeScript does not handle displaying a zero-length diagnostic very well, so
+          // increase the ending offset by 1 for such errors, to ensure the position is shown in the
+          // diagnostic.
+          span.end.offset++;
+        }
+
+        return makeTemplateDiagnostic(
+            id, template.sourceMapping, span, ts.DiagnosticCategory.Error,
+            ngErrorCode(ErrorCode.TEMPLATE_PARSE_ERROR), error.msg);
+      });
     }
 
     // Figure out the set of styles. The ordering here is important: external resources (styleUrls)
@@ -335,6 +353,7 @@ export class ComponentDecoratorHandler implements
         providersRequiringFactory,
         viewProvidersRequiringFactory,
       },
+      diagnostics,
     };
     if (changeDetection !== null) {
       output.analysis!.meta.changeDetection = changeDetection;
diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
index 91eb812384..b1e767dcf2 100644
--- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
+++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
@@ -56,6 +56,11 @@ export enum ErrorCode {
    */
   HOST_BINDING_PARSE_ERROR = 5001,
 
+  /**
+   * Raised when the compiler cannot parse a component's template.
+   */
+  TEMPLATE_PARSE_ERROR = 5002,
+
   /**
    * Raised when an NgModule contains an invalid reference in `declarations`.
    */
diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
index 596de1ef14..74b4fdc601 100644
--- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
+++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
@@ -7378,6 +7378,54 @@ export const Foo = Foo__PRE_R3__;
            const diags = env.driveDiagnostics();
            expect(diags.length).toBe(0);
          });
+
+      describe('template parsing diagnostics', () => {
+        // These tests validate that errors which occur during template parsing are expressed as
+        // diagnostics instead of a compiler crash (which used to be the case). They only assert
+        // that the error is produced with an accurate span - the exact semantics of the errors are
+        // tested separately, via the parser tests.
+        it('should emit a diagnostic for a template parsing error', () => {
+          env.write('test.ts', `
+            import {Component} from '@angular/core';
+            @Component({
+              template: '<div></span>',
+              selector: 'test-cmp',
+            })
+            export class TestCmp {}
+          `);
+          const diags = env.driveDiagnostics();
+          expect(diags.length).toBe(1);
+          expect(getDiagnosticSourceCode(diags[0])).toBe('</span>');
+        });
+
+        it('should emit a diagnostic for an expression parsing error', () => {
+          env.write('test.ts', `
+            import {Component} from '@angular/core';
+            @Component({
+              template: '<cmp [input]="x ? y">',
+              selector: 'test-cmp',
+            })
+            export class TestCmp {}
+          `);
+          const diags = env.driveDiagnostics();
+          expect(diags.length).toBe(1);
+          expect(getDiagnosticSourceCode(diags[0])).toBe('x ? y');
+        });
+
+        it('should use a single character span for an unexpected EOF parsing error', () => {
+          env.write('test.ts', `
+              import {Component} from '@angular/core';
+              @Component({
+                template: '<cmp [input]="x>',
+                selector: 'test-cmp',
+              })
+              export class TestCmp {}
+            `);
+          const diags = env.driveDiagnostics();
+          expect(diags.length).toBe(1);
+          expect(getDiagnosticSourceCode(diags[0])).toBe('\'');
+        });
+      });
     });
   });
 
diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts
index 52a5e51406..16d899a173 100644
--- a/packages/compiler/src/render3/r3_template_transform.ts
+++ b/packages/compiler/src/render3/r3_template_transform.ts
@@ -62,12 +62,6 @@ export function htmlAstToRender3Ast(
 
   // Errors might originate in either the binding parser or the html to ivy transformer
   const allErrors = bindingParser.errors.concat(transformer.errors);
-  const errors: ParseError[] = allErrors.filter(e => e.level === ParseErrorLevel.ERROR);
-
-  if (errors.length > 0) {
-    const errorString = errors.join('\n');
-    throw syntaxError(`Template parse errors:\n${errorString}`, errors);
-  }
 
   return {
     nodes: ivyNodes,
diff --git a/packages/compiler/test/render3/view/util.ts b/packages/compiler/test/render3/view/util.ts
index d6347ad573..af3ed25c11 100644
--- a/packages/compiler/test/render3/view/util.ts
+++ b/packages/compiler/test/render3/view/util.ts
@@ -101,7 +101,14 @@ export function parseR3(
       ['onEvent'], ['onEvent']);
   const bindingParser =
       new BindingParser(expressionParser, DEFAULT_INTERPOLATION_CONFIG, schemaRegistry, null, []);
-  return htmlAstToRender3Ast(htmlNodes, bindingParser);
+  const r3Result = htmlAstToRender3Ast(htmlNodes, bindingParser);
+
+  if (r3Result.errors.length > 0) {
+    const msg = r3Result.errors.map(e => e.toString()).join('\n');
+    throw new Error(msg);
+  }
+
+  return r3Result;
 }
 
 export function processI18nMeta(