From 9d99f8936b060fbdd3c4689a62cd61eae35cfedc Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Thu, 20 Jul 2023 09:38:40 +0200
Subject: [PATCH] [IMP] compiler: improve error message when failing to compile
 template

Previously, when a template failed to compile because of a syntax error
(typically because we don't do any syntax checking when compiling
expression, allowing invalid expressions to be transpiled successfully),
the error reporting was very minimal: you would only get the error
message itself (eg: "Unexpected token") with the stack information of
the error pointing to the call to `new Function` in owl, which is not
very useful.

This commit catches the compilation error and completes it with
information about the template name when available, and also adds the
generated code to the error message, allowing the user to just
copy/paste it in their web console or code editor to get a more precise
location for the error.
---
 src/compiler/index.ts                         | 13 +++-
 tests/compiler/validation.test.ts             | 18 ++++++
 .../__snapshots__/error_handling.test.ts.snap | 12 ++++
 tests/components/error_handling.test.ts       | 60 +++++++++++++++++++
 tests/misc/portal.test.ts                     |  2 +-
 5 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/src/compiler/index.ts b/src/compiler/index.ts
index 6a0b6379..d79e9534 100644
--- a/src/compiler/index.ts
+++ b/src/compiler/index.ts
@@ -2,6 +2,7 @@ import type { TemplateSet } from "../runtime/template_set";
 import type { BDom } from "../runtime/blockdom";
 import { CodeGenerator, Config } from "./code_generator";
 import { parse } from "./parser";
+import { OwlError } from "../runtime";
 
 export type Template = (context: any, vnode: any, key?: string) => BDom;
 
@@ -27,5 +28,15 @@ export function compile(
   const codeGenerator = new CodeGenerator(ast, { ...options, hasSafeContext });
   const code = codeGenerator.generateCode();
   // template function
-  return new Function("app, bdom, helpers", code) as TemplateFunction;
+  try {
+    return new Function("app, bdom, helpers", code) as TemplateFunction;
+  } catch (originalError: any) {
+    const { name } = options;
+    const nameStr = name ? `template "${name}"` : "anonymous template";
+    const err = new OwlError(
+      `Failed to compile ${nameStr}: ${originalError.message}\n\ngenerated code:\nfunction(app, bdom, helpers) {\n${code}\n}`
+    );
+    err.cause = originalError;
+    throw err;
+  }
 }
diff --git a/tests/compiler/validation.test.ts b/tests/compiler/validation.test.ts
index c57925d4..e90eaf9e 100644
--- a/tests/compiler/validation.test.ts
+++ b/tests/compiler/validation.test.ts
@@ -37,4 +37,22 @@ describe("basic validation", () => {
     const template = `<div t-best-beer="rochefort 10">test</div>`;
     expect(() => renderToString(template)).toThrow("Unknown QWeb directive: 't-best-beer'");
   });
+
+  test("compilation error", () => {
+    const template = `<div t-att-class="a b">test</div>`;
+    expect(() => renderToString(template))
+      .toThrow(`Failed to compile anonymous template: Unexpected identifier
+
+generated code:
+function(app, bdom, helpers) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div block-attribute-0="class">test</div>\`);
+  
+  return function template(ctx, node, key = "") {
+    let attr1 = ctx['a']ctx['b'];
+    return block1([attr1]);
+  }
+}`);
+  });
 });
diff --git a/tests/components/__snapshots__/error_handling.test.ts.snap b/tests/components/__snapshots__/error_handling.test.ts.snap
index b4417a9f..e326292d 100644
--- a/tests/components/__snapshots__/error_handling.test.ts.snap
+++ b/tests/components/__snapshots__/error_handling.test.ts.snap
@@ -12,6 +12,18 @@ exports[`basics display a nice error if a component is not a component 1`] = `
 }"
 `;
 
+exports[`basics display a nice error if a non-root component template fails to compile 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, []);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return comp1({}, key + \`__1\`, node, this, null);
+  }
+}"
+`;
+
 exports[`basics display a nice error if it cannot find component (in dev mode) 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts
index a24d4f9e..a444125c 100644
--- a/tests/components/error_handling.test.ts
+++ b/tests/components/error_handling.test.ts
@@ -143,6 +143,66 @@ describe("basics", () => {
     );
   });
 
+  test("display a nice error if the root component template fails to compile", async () => {
+    // This is a special case: mount throws synchronously and we don't have any
+    // node which can handle the error, hence the different structure of this test
+    class Comp extends Component {
+      static template = xml`<div t-att-class="a b">test</div>`;
+    }
+    const app = new App(Comp);
+    let error: Error;
+    try {
+      await app.mount(fixture);
+    } catch (e) {
+      error = e as Error;
+    }
+    const expectedErrorMessage = `Failed to compile anonymous template: Unexpected identifier
+
+generated code:
+function(app, bdom, helpers) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div block-attribute-0="class">test</div>\`);
+  
+  return function template(ctx, node, key = "") {
+    let attr1 = ctx['a']ctx['b'];
+    return block1([attr1]);
+  }
+}`;
+    expect(error!).toBeDefined();
+    expect(error!.message).toBe(expectedErrorMessage);
+  });
+
+  test("display a nice error if a non-root component template fails to compile", async () => {
+    class Child extends Component {
+      static template = xml`<div t-att-class="a b">test</div>`;
+    }
+    class Parent extends Component {
+      static components = { Child };
+      static template = xml`<Child/>`;
+    }
+    const expectedErrorMessage = `Failed to compile anonymous template: Unexpected identifier
+
+generated code:
+function(app, bdom, helpers) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div block-attribute-0="class">test</div>\`);
+  
+  return function template(ctx, node, key = "") {
+    let attr1 = ctx['a']ctx['b'];
+    return block1([attr1]);
+  }
+}`;
+    const app = new App(Parent as typeof Component);
+    let error: Error;
+    const mountProm = app.mount(fixture).catch((e: Error) => (error = e));
+    await expect(nextAppError(app)).resolves.toThrow(expectedErrorMessage);
+    await mountProm;
+    expect(error!).toBeDefined();
+    expect(error!.message).toBe(expectedErrorMessage);
+  });
+
   test("simple catchError", async () => {
     class Boom extends Component {
       static template = xml`<div t-esc="a.b.c"/>`;
diff --git a/tests/misc/portal.test.ts b/tests/misc/portal.test.ts
index b5c230b4..8e5d0e87 100644
--- a/tests/misc/portal.test.ts
+++ b/tests/misc/portal.test.ts
@@ -988,7 +988,7 @@ describe("Portal: Props validation", () => {
       error = e as Error;
     }
     expect(error!).toBeDefined();
-    expect(error!.message).toBe(`Unexpected token ','`);
+    expect(error!.message).toContain(`Unexpected token ','`);
   });
 
   test("target must be a valid selector", async () => {