[FIX] qweb: t-foreach needs to hold on scope

Have a t-on within a t-foreach
the t-on has an expression

Before this commit, when triggering the t-on, the expression was falsely evaluated
In details, the expression took scope from outside the foreach loop
as we protect it to have similar results than an actual for loop

But, at triggering time, the scope protection had already been terminated
meaning the info of the iteration of the loop the handler has been built in
had already disappeared

This commit fixes that

closes #594

src/qweb/compilation_context.ts

@@ -1,4 +1,4 @@
-import { compileExpr, QWebVar } from "./expression_parser";
+import { compileExpr , compileExprToArray , QWebVar } from "./expression_parser";
 
 export const INTERP_REGEXP = /\{\{.*?\}\}/g;
 //------------------------------------------------------------------------------
@@ -156,6 +156,22 @@ export class CompilationContext {
     this.rootContext.shouldDefineScope = true;
     return compileExpr(expr, this.variables);
   }
+  captureExpression(expr: string): string {
+    this.rootContext.shouldDefineScope = true;
+    const argId = this.generateID();
+    const tokens = compileExprToArray(expr, this.variables);
+    const done = new Set();
+    return tokens.map((tok) => {
+      if (tok.varName) {
+        if (!done.has(tok.varName)) {
+          done.add(tok.varName);
+          this.addLine(`const ${tok.varName}_${argId} = ${tok.value};`);
+        }
+        tok.value = `${tok.varName}_${argId}`;
+      }
+      return tok.value;
+    }).join("");
+  }
 
   /**
    * Perform string interpolation on the given string. Note that if the whole
@@ -187,4 +203,4 @@ export class CompilationContext {
     this.rootContext.protectedScopeNumber--;
     this.addLine(`scope = _origScope${protectID};`);
   }
-}
+}

src/qweb/expression_parser.ts

@@ -66,6 +66,7 @@ interface Token {
   value: string;
   originalValue?: string;
   size?: number;
+  varName?: string;
 }
 
 const STATIC_TOKEN_MAP: { [key: string]: TKind } = {
@@ -234,7 +235,7 @@ export function tokenize(expr: string): Token[] {
  * the arrow operator, then we add the current (or some previous tokens) token to
  * the list of variables so it does not get replaced by a lookup in the context
  */
-export function compileExpr(expr: string, scope: { [key: string]: QWebVar }): string {
+export function compileExprToArray(expr: string, scope: { [key: string]: QWebVar }): Token[] {
   scope = Object.create(scope);
   const tokens = tokenize(expr);
   for (let i = 0; i < tokens.length; i++) {
@@ -269,6 +270,7 @@ export function compileExpr(expr: string, scope: { [key: string]: QWebVar }): st
     }
 
     if (isVar) {
+      token.varName = token.value;
       if (token.value in scope && "id" in scope[token.value]) {
         token.value = scope[token.value].expr!;
       } else {
@@ -277,5 +279,9 @@ export function compileExpr(expr: string, scope: { [key: string]: QWebVar }): st
       }
     }
   }
-  return tokens.map(t => t.value).join("");
+  return tokens;
+}
+
+export function compileExpr(expr: string, scope: { [key: string]: QWebVar }): string {
+  return compileExprToArray(expr, scope).map(t => t.value).join("");
 }

src/qweb/extensions.ts

@@ -58,7 +58,7 @@ export function makeHandlerCode(
     ctx.rootContext.shouldDefineUtils = true;
     const comp = `utils.getComponent(context)`;
     if (args) {
-      let argId = ctx.generateID();
+      const argId = ctx.generateID();
       ctx.addLine(`let args${argId} = [${ctx.formatExpression(args)}];`);
       code = `${comp}['${name}'](...args${argId}, e);`;
       putInCache = false;
@@ -67,8 +67,9 @@ export function makeHandlerCode(
     }
   } else {
     // if we get here, then it is an expression
+    // we need to capture every variable in it
     putInCache = false;
-    code = ctx.formatExpression(value);
+    code = ctx.captureExpression(value);
   }
   const modCode = mods.map(mod => modcodes[mod]).join("");
   let handler = `function (e) {if (!context.__owl__.isMounted){return}${modCode}${code}}`;

tests/component/__snapshots__/component.test.ts.snap

@@ -639,6 +639,204 @@ exports[`other directives with t-component slot setted value (with t-set) not ac
 }"
 `;
 
+exports[`other directives with t-component t-on expression captured in t-foreach 1`] = `
+"function anonymous(context, extra
+) {
+    // Template name: \\"__template__1\\"
+    let utils = this.constructor.utils;
+    let scope = Object.create(context);
+    let h = this.h;
+    let c1 = [], p1 = {key:1};
+    let vn1 = h('div', p1, c1);
+    scope.iter = 0;
+    let _2 = scope['arr'];
+    if (!_2) { throw new Error('QWeb error: Invalid loop expression')}
+    let _3 = _4 = _2;
+    if (!(_2 instanceof Array)) {
+        _3 = Object.keys(_2);
+        _4 = Object.values(_2);
+    }
+    let _length3 = _3.length;
+    let _origScope5 = scope;
+    scope = Object.create(scope);
+    for (let i1 = 0; i1 < _length3; i1++) {
+        scope.val_first = i1 === 0
+        scope.val_last = i1 === _length3 - 1
+        scope.val_index = i1
+        scope.val = _3[i1]
+        scope.val_value = _4[i1]
+        let key1 = scope['val'];
+        let c6 = [], p6 = {key:\`\${key1}_6\`};
+        let vn6 = h('div', p6, c6);
+        c1.push(vn6);
+        let c7 = [], p7 = {key:\`\${key1}_7\`,on:{}};
+        let vn7 = h('button', p7, c7);
+        c6.push(vn7);
+        const otherState_8 = scope['otherState'];
+        const iter_8 = scope.iter;
+        p7.on['click'] = function (e) {if (!context.__owl__.isMounted){return}otherState_8.vals.push(iter_8+'_'+iter_8)};
+        c7.push({text: \`expr\`});
+        utils.getScope(scope, 'iter').iter = scope.iter+1;
+    }
+    scope = _origScope5;
+    return vn1;
+}"
+`;
+
+exports[`other directives with t-component t-on expression in t-foreach 1`] = `
+"function anonymous(context, extra
+) {
+    // Template name: \\"__template__1\\"
+    let scope = Object.create(context);
+    let h = this.h;
+    let c1 = [], p1 = {key:1};
+    let vn1 = h('div', p1, c1);
+    let _2 = scope['state'].values;
+    if (!_2) { throw new Error('QWeb error: Invalid loop expression')}
+    let _3 = _4 = _2;
+    if (!(_2 instanceof Array)) {
+        _3 = Object.keys(_2);
+        _4 = Object.values(_2);
+    }
+    let _length3 = _3.length;
+    let _origScope5 = scope;
+    scope = Object.create(scope);
+    for (let i1 = 0; i1 < _length3; i1++) {
+        scope.val_first = i1 === 0
+        scope.val_last = i1 === _length3 - 1
+        scope.val_index = i1
+        scope.val = _3[i1]
+        scope.val_value = _4[i1]
+        let key1 = scope['val'];
+        let c6 = [], p6 = {key:\`\${key1}_6\`};
+        let vn6 = h('div', p6, c6);
+        c1.push(vn6);
+        let _7 = scope['val_index'];
+        if (_7 != null) {
+            c6.push({text: _7});
+        }
+        c6.push({text: \`: \`});
+        let _8 = scope['val']+'';
+        if (_8 != null) {
+            c6.push({text: _8});
+        }
+        let c9 = [], p9 = {key:\`\${key1}_9\`,on:{}};
+        let vn9 = h('button', p9, c9);
+        c6.push(vn9);
+        const otherState_10 = scope['otherState'];
+        const val_10 = scope['val'];
+        p9.on['click'] = function (e) {if (!context.__owl__.isMounted){return}otherState_10.vals.push(val_10)};
+        c9.push({text: \`Expr\`});
+    }
+    scope = _origScope5;
+    return vn1;
+}"
+`;
+
+exports[`other directives with t-component t-on expression in t-foreach with t-set 1`] = `
+"function anonymous(context, extra
+) {
+    // Template name: \\"__template__1\\"
+    let utils = this.constructor.utils;
+    let scope = Object.create(context);
+    let h = this.h;
+    let c1 = [], p1 = {key:1};
+    let vn1 = h('div', p1, c1);
+    scope.bossa = 'nova';
+    let _2 = scope['state'].values;
+    if (!_2) { throw new Error('QWeb error: Invalid loop expression')}
+    let _3 = _4 = _2;
+    if (!(_2 instanceof Array)) {
+        _3 = Object.keys(_2);
+        _4 = Object.values(_2);
+    }
+    let _length3 = _3.length;
+    let _origScope5 = scope;
+    scope = Object.create(scope);
+    for (let i1 = 0; i1 < _length3; i1++) {
+        scope.val_first = i1 === 0
+        scope.val_last = i1 === _length3 - 1
+        scope.val_index = i1
+        scope.val = _3[i1]
+        scope.val_value = _4[i1]
+        let key1 = scope['val'];
+        let c6 = [], p6 = {key:\`\${key1}_6\`};
+        let vn6 = h('div', p6, c6);
+        c1.push(vn6);
+        utils.getScope(scope, 'bossa').bossa = scope.bossa+'_'+scope['val_index'];
+        let _7 = scope['val_index'];
+        if (_7 != null) {
+            c6.push({text: _7});
+        }
+        c6.push({text: \`: \`});
+        let _8 = scope['val']+'';
+        if (_8 != null) {
+            c6.push({text: _8});
+        }
+        let c9 = [], p9 = {key:\`\${key1}_9\`,on:{}};
+        let vn9 = h('button', p9, c9);
+        c6.push(vn9);
+        const otherState_10 = scope['otherState'];
+        const val_10 = scope['val'];
+        const bossa_10 = scope.bossa;
+        p9.on['click'] = function (e) {if (!context.__owl__.isMounted){return}otherState_10.vals.push(val_10+'_'+bossa_10)};
+        c9.push({text: \`Expr\`});
+    }
+    scope = _origScope5;
+    return vn1;
+}"
+`;
+
+exports[`other directives with t-component t-on method call in t-foreach 1`] = `
+"function anonymous(context, extra
+) {
+    // Template name: \\"__template__1\\"
+    let utils = this.constructor.utils;
+    let scope = Object.create(context);
+    let h = this.h;
+    let c1 = [], p1 = {key:1};
+    let vn1 = h('div', p1, c1);
+    let _2 = scope['state'].values;
+    if (!_2) { throw new Error('QWeb error: Invalid loop expression')}
+    let _3 = _4 = _2;
+    if (!(_2 instanceof Array)) {
+        _3 = Object.keys(_2);
+        _4 = Object.values(_2);
+    }
+    let _length3 = _3.length;
+    let _origScope5 = scope;
+    scope = Object.create(scope);
+    for (let i1 = 0; i1 < _length3; i1++) {
+        scope.val_first = i1 === 0
+        scope.val_last = i1 === _length3 - 1
+        scope.val_index = i1
+        scope.val = _3[i1]
+        scope.val_value = _4[i1]
+        let key1 = scope['val'];
+        let c6 = [], p6 = {key:\`\${key1}_6\`};
+        let vn6 = h('div', p6, c6);
+        c1.push(vn6);
+        let _7 = scope['val_index'];
+        if (_7 != null) {
+            c6.push({text: _7});
+        }
+        c6.push({text: \`: \`});
+        let _8 = scope['val']+'';
+        if (_8 != null) {
+            c6.push({text: _8});
+        }
+        let c9 = [], p9 = {key:\`\${key1}_9\`,on:{}};
+        let vn9 = h('button', p9, c9);
+        c6.push(vn9);
+        let args10 = [scope['val']];
+        p9.on['click'] = function (e) {if (!context.__owl__.isMounted){return}utils.getComponent(context)['addVal'](...args10, e);};
+        c9.push({text: \`meth call\`});
+    }
+    scope = _origScope5;
+    return vn1;
+}"
+`;
+
 exports[`other directives with t-component t-on with getter as handler 1`] = `
 "function anonymous(context, extra
 ) {
@@ -853,6 +1051,7 @@ exports[`other directives with t-component t-on with inline statement 1`] = `
         c1.push({text: _2});
     }
     // Component 'Child'
+    const state_5 = scope['state'];
     let w3 = '__4__' in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap['__4__']] : false;
     let props3 = {};
     if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) {
@@ -869,7 +1068,7 @@ exports[`other directives with t-component t-on with inline statement 1`] = `
         if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')}
         w3 = new W3(parent, props3);
         parent.__owl__.cmap['__4__'] = w3.__owl__.id;
-        let fiber = w3.__prepare(extra.fiber, undefined, () => { const vnode = fiber.vnode; pvnode.sel = vnode.sel; vnode.data.hook = {create(_, vn){vn.elm.addEventListener('ev', function (e) {if (!context.__owl__.isMounted){return}scope['state'].counter++});}};});
+        let fiber = w3.__prepare(extra.fiber, undefined, () => { const vnode = fiber.vnode; pvnode.sel = vnode.sel; vnode.data.hook = {create(_, vn){vn.elm.addEventListener('ev', function (e) {if (!context.__owl__.isMounted){return}state_5.counter++});}};});
         let pvnode = h('dummy', {key: '__4__', hook: {remove() {},destroy(vn) {w3.destroy();}}});
         c1.push(pvnode);
         w3.__owl__.pvnode = pvnode;

tests/component/component.test.ts

@@ -2902,6 +2902,115 @@ describe("other directives with t-component", () => {
     await widget.mount(fixture);
     expect(fixture.innerHTML).toBe("<div><p>InLoop: 0</p><p>InLoop: 1</p><p>EndLoop: 1</p></div>");
   });
+
+  test("t-on expression in t-foreach", async () => {
+    class SomeWidget extends Component<any, any> {
+      static template = xml`
+      <div>
+        <div t-foreach="state.values" t-as="val" t-key="val">
+          <t t-esc="val_index"/>: <t t-esc="val + ''"/>
+          <button t-on-click="otherState.vals.push(val)">Expr</button>
+        </div>
+      </div>`;
+
+      state = useState({values: ['a', 'b']});
+      otherState = {vals: []};
+    }
+    const widget = new SomeWidget();
+    await widget.mount(fixture);
+
+    expect(fixture.innerHTML).toBe("<div><div>0: a<button>Expr</button></div><div>1: b<button>Expr</button></div></div>");
+    expect(widget.otherState.vals).toStrictEqual([]);
+    const buttons = fixture.querySelectorAll('button');
+    buttons[0].click();
+    buttons[1].click();
+    expect(widget.otherState.vals).toStrictEqual(['a', 'b']);
+    expect(QWeb.TEMPLATES[SomeWidget.template].fn.toString()).toMatchSnapshot();
+  });
+
+  test("t-on expression in t-foreach with t-set", async () => {
+    class SomeWidget extends Component<any, any> {
+      static template = xml`
+      <div>
+        <t t-set="bossa" t-value="'nova'"/>
+        <div t-foreach="state.values" t-as="val" t-key="val">
+          <t t-set="bossa" t-value="bossa + '_' + val_index" />
+          <t t-esc="val_index"/>: <t t-esc="val + ''"/>
+          <button t-on-click="otherState.vals.push(val + '_' + bossa)">Expr</button>
+        </div>
+      </div>`;
+
+      state = useState({values: ['a', 'b']});
+      otherState = {vals: []};
+    }
+    const widget = new SomeWidget();
+    await widget.mount(fixture);
+
+    expect(fixture.innerHTML).toBe("<div><div>0: a<button>Expr</button></div><div>1: b<button>Expr</button></div></div>");
+    expect(widget.otherState.vals).toStrictEqual([]);
+    const buttons = fixture.querySelectorAll('button');
+    buttons[0].click();
+    buttons[1].click();
+    expect(widget.otherState.vals).toStrictEqual(['a_nova_0', 'b_nova_0_1']);
+    expect(QWeb.TEMPLATES[SomeWidget.template].fn.toString()).toMatchSnapshot();
+  });
+
+  test("t-on method call in t-foreach", async () => {
+    class SomeWidget extends Component<any, any> {
+      static template = xml`
+      <div>
+        <div t-foreach="state.values" t-as="val" t-key="val">
+          <t t-esc="val_index"/>: <t t-esc="val + ''"/>
+          <button t-on-click="addVal(val)">meth call</button>
+        </div>
+      </div>`;
+
+      state = useState({values: ['a', 'b']});
+      otherState = {vals: new Array<string>()};
+
+      addVal(val: string) {
+        this.otherState.vals.push(val);
+      }
+    }
+    const widget = new SomeWidget();
+    await widget.mount(fixture);
+
+
+    expect(fixture.innerHTML).toBe("<div><div>0: a<button>meth call</button></div><div>1: b<button>meth call</button></div></div>");
+    expect(widget.otherState.vals).toStrictEqual([]);
+    const buttons = fixture.querySelectorAll('button');
+    buttons[0].click();
+    buttons[1].click();
+    expect(widget.otherState.vals).toStrictEqual(['a', 'b']);
+    expect(QWeb.TEMPLATES[SomeWidget.template].fn.toString()).toMatchSnapshot();
+  });
+
+  test("t-on expression captured in t-foreach", async () => {
+    class SomeWidget extends Component<any, any> {
+      static template = xml`
+      <div>
+        <t t-set="iter" t-value="0" />
+        <div t-foreach="arr" t-as="val" t-key="val">
+          <button t-on-click="otherState.vals.push(iter + '_' + iter)">expr</button>
+          <t t-set="iter" t-value="iter + 1" />
+        </div>
+      </div>`;
+
+      arr = ['a', 'b']
+      otherState = {vals: new Array<string>()};
+    }
+    const widget = new SomeWidget();
+    await widget.mount(fixture);
+
+
+    expect(fixture.innerHTML).toBe("<div><div><button>expr</button></div><div><button>expr</button></div></div>");
+    expect(widget.otherState.vals).toStrictEqual([]);
+    const buttons = fixture.querySelectorAll('button');
+    buttons[0].click();
+    buttons[1].click();
+    expect(widget.otherState.vals).toStrictEqual(['0_0', '1_1']);
+    expect(QWeb.TEMPLATES[SomeWidget.template].fn.toString()).toMatchSnapshot();
+  });
 });
 
 describe("random stuff/miscellaneous", () => {

tests/qweb/__snapshots__/qweb.test.ts.snap

@@ -2400,7 +2400,8 @@ exports[`t-on t-on with inline statement (function call) 1`] = `
     let h = this.h;
     let c1 = [], p1 = {key:1,on:{}};
     let vn1 = h('button', p1, c1);
-    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}scope['state'].incrementCounter(2)};
+    const state_2 = scope['state'];
+    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}state_2.incrementCounter(2)};
     c1.push({text: \`Click\`});
     return vn1;
 }"
@@ -2414,7 +2415,8 @@ exports[`t-on t-on with inline statement 1`] = `
     let h = this.h;
     let c1 = [], p1 = {key:1,on:{}};
     let vn1 = h('button', p1, c1);
-    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}scope['state'].counter++};
+    const state_2 = scope['state'];
+    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}state_2.counter++};
     c1.push({text: \`Click\`});
     return vn1;
 }"
@@ -2428,7 +2430,8 @@ exports[`t-on t-on with inline statement, part 2 1`] = `
     let h = this.h;
     let c1 = [], p1 = {key:1,on:{}};
     let vn1 = h('button', p1, c1);
-    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}scope['state'].flag=!scope['state'].flag};
+    const state_2 = scope['state'];
+    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}state_2.flag=!state_2.flag};
     c1.push({text: \`Toggle\`});
     return vn1;
 }"
@@ -2442,7 +2445,9 @@ exports[`t-on t-on with inline statement, part 3 1`] = `
     let h = this.h;
     let c1 = [], p1 = {key:1,on:{}};
     let vn1 = h('button', p1, c1);
-    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}scope['state'].n=scope['someFunction'](3)};
+    const state_2 = scope['state'];
+    const someFunction_2 = scope['someFunction'];
+    p1.on['click'] = function (e) {if (!context.__owl__.isMounted){return}state_2.n=someFunction_2(3)};
     c1.push({text: \`Toggle\`});
     return vn1;
 }"