From 521111644ce69aa2887c1143e5cacd9e69c05d39 Mon Sep 17 00:00:00 2001 From: Mathieu Duckerts-Antoine Date: Wed, 10 Sep 2025 15:17:25 +0200 Subject: [PATCH] [FIX] parser, code_generator: wrapped children recognition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several directives (t-key, t-log, t-translation, …) are represented by wrapper ASTs that contain another AST or null. Because these wrappers don't share the same type as their children, various AST type checks were broken. This commit addresses those issues by: - Commuting Translation / TranslationContext and Multi ASTs so that Multi children are spread as expected (see parseChildren). - Parsing the t-key directive before t-esc / t-out (in line with https://github.com/odoo/owl/pull/1685). - Ensuring wrappers around TSet ASTs are recognized as having no representation, so compileMulti properly discards children equivalent to TSet. --- src/compiler/code_generator.ts | 8 +- src/compiler/parser.ts | 131 +++++++++++++----- .../__snapshots__/t_debug_log.test.ts.snap | 42 ++++++ .../compiler/__snapshots__/t_key.test.ts.snap | 15 ++ .../__snapshots__/translation.test.ts.snap | 52 ++++++- tests/compiler/parser.test.ts | 14 +- tests/compiler/t_debug_log.test.ts | 30 ++++ tests/compiler/t_key.test.ts | 6 + tests/compiler/translation.test.ts | 47 +++++++ 9 files changed, 300 insertions(+), 45 deletions(-) diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts index 11839c88..8c696e94 100644 --- a/src/compiler/code_generator.ts +++ b/src/compiler/code_generator.ts @@ -995,7 +995,7 @@ export class CodeGenerator { const isNewBlock = !block || forceNewBlock; let codeIdx = this.target.code.length; if (isNewBlock) { - const n = ast.content.filter((c) => c.type !== ASTType.TSet).length; + const n = ast.content.filter((c) => !c.hasNoRepresentation).length; let result: string | null = null; if (n <= 1) { for (let child of ast.content) { @@ -1009,15 +1009,15 @@ export class CodeGenerator { let index = 0; for (let i = 0, l = ast.content.length; i < l; i++) { const child = ast.content[i]; - const isTSet = child.type === ASTType.TSet; + const forceNewBlock = !child.hasNoRepresentation; const subCtx = createContext(ctx, { block, index, - forceNewBlock: !isTSet, + forceNewBlock, isLast: ctx.isLast && i === l - 1, }); this.compileAST(child, subCtx); - if (!isTSet) { + if (forceNewBlock) { index++; } } diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index 12fb592b..aff0bc86 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -31,12 +31,17 @@ export const enum ASTType { TPortal, } -export interface ASTText { +export interface BaseAST { + type: ASTType; + hasNoRepresentation?: true; +} + +export interface ASTText extends BaseAST { type: ASTType.Text; value: string; } -export interface ASTComment { +export interface ASTComment extends BaseAST { type: ASTType.Comment; value: string; } @@ -52,7 +57,7 @@ interface TModelInfo { specialInitTargetAttr: string | null; } -export interface ASTDomNode { +export interface ASTDomNode extends BaseAST { type: ASTType.DomNode; tag: string; content: AST[]; @@ -65,24 +70,24 @@ export interface ASTDomNode { ns: string | null; } -export interface ASTMulti { +export interface ASTMulti extends BaseAST { type: ASTType.Multi; content: AST[]; } -export interface ASTTEsc { +export interface ASTTEsc extends BaseAST { type: ASTType.TEsc; expr: string; defaultValue: string; } -export interface ASTTOut { +export interface ASTTOut extends BaseAST { type: ASTType.TOut; expr: string; body: AST[] | null; } -export interface ASTTif { +export interface ASTTif extends BaseAST { type: ASTType.TIf; condition: string; content: AST; @@ -90,15 +95,16 @@ export interface ASTTif { tElse: AST | null; } -export interface ASTTSet { +export interface ASTTSet extends BaseAST { type: ASTType.TSet; name: string; value: string | null; // value defined in attribute defaultValue: string | null; // value defined in body, if text body: AST[] | null; // content of body if not text + hasNoRepresentation: true; } -export interface ASTTForEach { +export interface ASTTForEach extends BaseAST { type: ASTType.TForEach; collection: string; elem: string; @@ -111,13 +117,13 @@ export interface ASTTForEach { key: string | null; } -export interface ASTTKey { +export interface ASTTKey extends BaseAST { type: ASTType.TKey; expr: string; content: AST; } -export interface ASTTCall { +export interface ASTTCall extends BaseAST { type: ASTType.TCall; name: string; body: AST[] | null; @@ -132,7 +138,7 @@ interface SlotDefinition { attrsTranslationCtx: Attrs | null; } -export interface ASTComponent { +export interface ASTComponent extends BaseAST { type: ASTType.TComponent; name: string; isDynamic: boolean; @@ -143,7 +149,7 @@ export interface ASTComponent { slots: { [name: string]: SlotDefinition } | null; } -export interface ASTSlot { +export interface ASTSlot extends BaseAST { type: ASTType.TSlot; name: string; attrs: Attrs | null; @@ -152,34 +158,34 @@ export interface ASTSlot { defaultContent: AST | null; } -export interface ASTTCallBlock { +export interface ASTTCallBlock extends BaseAST { type: ASTType.TCallBlock; name: string; } -export interface ASTDebug { +export interface ASTDebug extends BaseAST { type: ASTType.TDebug; content: AST | null; } -export interface ASTLog { +export interface ASTLog extends BaseAST { type: ASTType.TLog; expr: string; content: AST | null; } -export interface ASTTranslation { +export interface ASTTranslation extends BaseAST { type: ASTType.TTranslation; content: AST | null; } -export interface ASTTranslationContext { +export interface ASTTranslationContext extends BaseAST { type: ASTType.TTranslationContext; content: AST | null; translationCtx: string; } -export interface ASTTPortal { +export interface ASTTPortal extends BaseAST { type: ASTType.TPortal; target: string; content: AST; @@ -255,9 +261,9 @@ function parseNode(node: Node, ctx: ParsingContext): AST | null { parseTCallBlock(node, ctx) || parseTTranslation(node, ctx) || parseTTranslationContext(node, ctx) || + parseTKey(node, ctx) || parseTEscNode(node, ctx) || parseTOutNode(node, ctx) || - parseTKey(node, ctx) || parseTSlot(node, ctx) || parseComponent(node, ctx) || parseDOMNode(node, ctx) || @@ -334,20 +340,30 @@ function parseTCustom(node: Element, ctx: ParsingContext): AST | null { function parseTDebugLog(node: Element, ctx: ParsingContext): AST | null { if (node.hasAttribute("t-debug")) { node.removeAttribute("t-debug"); - return { + const content = parseNode(node, ctx); + const ast: ASTDebug = { type: ASTType.TDebug, - content: parseNode(node, ctx), + content, }; + if (content?.hasNoRepresentation) { + ast.hasNoRepresentation = true; + } + return ast; } if (node.hasAttribute("t-log")) { const expr = node.getAttribute("t-log")!; node.removeAttribute("t-log"); - return { + const content = parseNode(node, ctx); + const ast: ASTLog = { type: ASTType.TLog, expr, - content: parseNode(node, ctx), + content, }; + if (content?.hasNoRepresentation) { + ast.hasNoRepresentation = true; + } + return ast; } return null; } @@ -598,11 +614,19 @@ function parseTKey(node: Element, ctx: ParsingContext): AST | null { } const key = node.getAttribute("t-key")!; node.removeAttribute("t-key"); - const body = parseNode(node, ctx); - if (!body) { + const content = parseNode(node, ctx); + if (!content) { return null; } - return { type: ASTType.TKey, expr: key, content: body }; + const ast: ASTTKey = { + type: ASTType.TKey, + expr: key, + content, + }; + if (content.hasNoRepresentation) { + ast.hasNoRepresentation = true; + } + return ast; } // ----------------------------------------------------------------------------- @@ -724,7 +748,7 @@ function parseTSetNode(node: Element, ctx: ParsingContext): AST | null { if (node.textContent !== node.innerHTML) { body = parseChildren(node, ctx); } - return { type: ASTType.TSet, name, value, defaultValue, body }; + return { type: ASTType.TSet, name, value, defaultValue, body, hasNoRepresentation: true }; } // ----------------------------------------------------------------------------- @@ -916,32 +940,55 @@ function parseTSlot(node: Element, ctx: ParsingContext): AST | null { // Translation // ----------------------------------------------------------------------------- +function wrapInTTranslationAST(r: AST | null) { + const ast: ASTTranslation = { type: ASTType.TTranslation, content: r }; + if (r?.hasNoRepresentation) { + ast.hasNoRepresentation = true; + } + return ast; +} + function parseTTranslation(node: Element, ctx: ParsingContext): AST | null { if (node.getAttribute("t-translation") !== "off") { return null; } node.removeAttribute("t-translation"); - return { - type: ASTType.TTranslation, - content: parseNode(node, ctx), - }; + const result = parseNode(node, ctx); + if (result?.type === ASTType.Multi) { + const children = result.content.map(wrapInTTranslationAST); + return makeASTMulti(children); + } + return wrapInTTranslationAST(result); } // ----------------------------------------------------------------------------- // Translation Context // ----------------------------------------------------------------------------- +function wrapInTTranslationContextAST(r: AST | null, translationCtx: string) { + const ast: ASTTranslationContext = { + type: ASTType.TTranslationContext, + content: r, + translationCtx, + }; + if (r?.hasNoRepresentation) { + ast.hasNoRepresentation = true; + } + return ast; +} + function parseTTranslationContext(node: Element, ctx: ParsingContext): AST | null { const translationCtx = node.getAttribute("t-translation-context"); if (!translationCtx) { return null; } node.removeAttribute("t-translation-context"); - return { - type: ASTType.TTranslationContext, - content: parseNode(node, ctx), - translationCtx, - }; + const result = parseNode(node, ctx); + if (result?.type === ASTType.Multi) { + const children = result.content.map((c) => wrapInTTranslationContextAST(c, translationCtx)); + return makeASTMulti(children); + } + return wrapInTTranslationContextAST(result, translationCtx); } // ----------------------------------------------------------------------------- @@ -990,6 +1037,14 @@ function parseChildren(node: Element, ctx: ParsingContext): AST[] { return children; } +function makeASTMulti(children: AST[]) { + const ast: ASTMulti = { type: ASTType.Multi, content: children }; + if (children.every((c) => c.hasNoRepresentation)) { + ast.hasNoRepresentation = true; + } + return ast; +} + /** * Parse all the child nodes of a given node and return an ast if possible. * In the case there are multiple children, they are wrapped in a astmulti. @@ -1002,7 +1057,7 @@ function parseChildNodes(node: Element, ctx: ParsingContext): AST | null { case 1: return children[0]; default: - return { type: ASTType.Multi, content: children }; + return makeASTMulti(children); } } diff --git a/tests/compiler/__snapshots__/t_debug_log.test.ts.snap b/tests/compiler/__snapshots__/t_debug_log.test.ts.snap index 58899b6a..dde4b454 100644 --- a/tests/compiler/__snapshots__/t_debug_log.test.ts.snap +++ b/tests/compiler/__snapshots__/t_debug_log.test.ts.snap @@ -49,6 +49,27 @@ exports[`debugging t-debug on sub template 2`] = ` }" `; +exports[`debugging t-debug: interaction with t-set 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + let { isBoundary, withDefault, setContextValue } = helpers; + + let block1 = createBlock(\`\`); + + return function template(ctx, node, key = \\"\\") { + ctx = Object.create(ctx); + ctx[isBoundary] = 1 + debugger; + setContextValue(ctx, \\"foo\\", 42); + debugger; + setContextValue(ctx, \\"bar\\", 49); + let txt1 = ctx['foo']+ctx['bar']; + return block1([txt1]); + } +}" +`; + exports[`debugging t-log 1`] = ` "function anonymous(app, bdom, helpers ) { @@ -66,3 +87,24 @@ exports[`debugging t-log 1`] = ` } }" `; + +exports[`debugging t-log: interaction with t-set 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + let { isBoundary, withDefault, setContextValue } = helpers; + + let block1 = createBlock(\`\`); + + return function template(ctx, node, key = \\"\\") { + ctx = Object.create(ctx); + ctx[isBoundary] = 1 + console.log(ctx['foo']); + setContextValue(ctx, \\"foo\\", 42); + console.log(ctx['bar']); + setContextValue(ctx, \\"bar\\", 49); + let txt1 = ctx['foo']+ctx['bar']; + return block1([txt1]); + } +}" +`; diff --git a/tests/compiler/__snapshots__/t_key.test.ts.snap b/tests/compiler/__snapshots__/t_key.test.ts.snap index e51d1dc0..2063d12c 100644 --- a/tests/compiler/__snapshots__/t_key.test.ts.snap +++ b/tests/compiler/__snapshots__/t_key.test.ts.snap @@ -103,3 +103,18 @@ exports[`t-key t-key on sub dom node pushes a child block in its parent 2`] = ` } }" `; + +exports[`t-key t-key: interaction with t-esc 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + let block1 = createBlock(\`

\`); + + return function template(ctx, node, key = \\"\\") { + const tKey_1 = ctx['key']; + let txt1 = ctx['text']; + return toggler(tKey_1, block1([txt1])); + } +}" +`; diff --git a/tests/compiler/__snapshots__/translation.test.ts.snap b/tests/compiler/__snapshots__/translation.test.ts.snap index b7c1ea7c..f954f5b3 100644 --- a/tests/compiler/__snapshots__/translation.test.ts.snap +++ b/tests/compiler/__snapshots__/translation.test.ts.snap @@ -10,8 +10,7 @@ exports[`translation context body of t-sets are translated in context 1`] = ` ctx = Object.create(ctx); ctx[isBoundary] = 1 setContextValue(ctx, \\"label\\", \`traduit\`); - const b2 = text(ctx['label']); - return multi([b2]); + return text(ctx['label']); } }" `; @@ -94,6 +93,23 @@ exports[`translation context slot attrs and text contents are translated in cont }" `; +exports[`translation context t-translation-context with several children 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + let b2; + if (true) { + b2 = text(\`\`); + } + return block1([], [b2]); + } +}" +`; + exports[`translation context translation of attributes in context 1`] = ` "function anonymous(app, bdom, helpers ) { @@ -153,6 +169,21 @@ exports[`translation support body of t-sets inside translation=off are not trans }" `; +exports[`translation support body of t-sets inside translation=off are not translated 2 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + let { isBoundary, withDefault, setContextValue } = helpers; + + return function template(ctx, node, key = \\"\\") { + ctx = Object.create(ctx); + ctx[isBoundary] = 1 + setContextValue(ctx, \\"label\\", \`untranslated\`); + return text(ctx['label']); + } +}" +`; + exports[`translation support body of t-sets with html content are translated 1`] = ` "function anonymous(app, bdom, helpers ) { @@ -264,6 +295,23 @@ exports[`translation support t-set and falsy t-value: t-body are translated 1`] }" `; +exports[`translation support t-translation with several children 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + let b2; + if (true) { + b2 = text(\`\`); + } + return block1([], [b2]); + } +}" +`; + exports[`translation support translation is done on the trimmed text, with extra spaces readded after 1`] = ` "function anonymous(app, bdom, helpers ) { diff --git a/tests/compiler/parser.test.ts b/tests/compiler/parser.test.ts index a7c9d505..c75a5c33 100644 --- a/tests/compiler/parser.test.ts +++ b/tests/compiler/parser.test.ts @@ -692,6 +692,7 @@ describe("qweb parser", () => { value: "value", defaultValue: null, body: null, + hasNoRepresentation: true, }); }); @@ -702,6 +703,7 @@ describe("qweb parser", () => { defaultValue: "ok", value: null, body: null, + hasNoRepresentation: true, }); expect(parse(`
ok
`)).toEqual({ @@ -723,6 +725,7 @@ describe("qweb parser", () => { content: [{ type: ASTType.Text, value: "ok" }], }, ], + hasNoRepresentation: true, }); expect(parse(`
ok
abc`)).toEqual({ @@ -745,6 +748,7 @@ describe("qweb parser", () => { }, { type: ASTType.Text, value: "abc" }, ], + hasNoRepresentation: true, }); }); @@ -758,6 +762,7 @@ describe("qweb parser", () => { defaultValue: "ok", value: null, body: null, + hasNoRepresentation: true, }, tElif: null, tElse: null, @@ -783,7 +788,14 @@ describe("qweb parser", () => { condition: "flag", content: { type: ASTType.Text, value: "1" }, tElif: null, - tElse: { type: ASTType.TSet, name: "ourvar", value: "0", defaultValue: null, body: null }, + tElse: { + type: ASTType.TSet, + name: "ourvar", + value: "0", + defaultValue: null, + body: null, + hasNoRepresentation: true, + }, }, ], }); diff --git a/tests/compiler/t_debug_log.test.ts b/tests/compiler/t_debug_log.test.ts index 97bc8ada..82e10749 100644 --- a/tests/compiler/t_debug_log.test.ts +++ b/tests/compiler/t_debug_log.test.ts @@ -38,4 +38,34 @@ describe("debugging", () => { expect(console.log).toHaveBeenCalledWith(45); console.log = consoleLog; }); + + test("t-log: interaction with t-set", () => { + const consoleLog = console.log; + console.log = jest.fn(); + + const template = ` + + + + + + `; + snapshotTemplate(template); + renderToString(template); + expect(console.log).toHaveBeenCalledWith(undefined); + expect(console.log).toHaveBeenCalledWith(undefined); + console.log = consoleLog; + }); + + test("t-debug: interaction with t-set", () => { + const template = ` + + + + + + `; + snapshotTemplate(template); + renderToString(template); + }); }); diff --git a/tests/compiler/t_key.test.ts b/tests/compiler/t_key.test.ts index 596fbf9c..9355b6fe 100644 --- a/tests/compiler/t_key.test.ts +++ b/tests/compiler/t_key.test.ts @@ -63,4 +63,10 @@ describe("t-key", () => { expect(renderToString(template2, { key: "1" })).toBe("

"); }); + + test("t-key: interaction with t-esc", async () => { + const template = `

`; + + expect(renderToString(template, { key: "1", text: "abc" })).toBe("

abc

"); + }); }); diff --git a/tests/compiler/translation.test.ts b/tests/compiler/translation.test.ts index dd3009f5..9eae3680 100644 --- a/tests/compiler/translation.test.ts +++ b/tests/compiler/translation.test.ts @@ -129,6 +129,21 @@ describe("translation support", () => { expect(fixture.innerHTML).toBe("untranslated"); }); + test("body of t-sets inside translation=off are not translated 2", async () => { + class SomeComponent extends Component { + static template = xml` + + untranslated + + `; + } + + const translateFn = () => "translated"; + + await mount(SomeComponent, fixture, { translateFn }); + expect(fixture.innerHTML).toBe("untranslated"); + }); + test("body of t-sets with html content are translated", async () => { class SomeComponent extends Component { static template = xml` @@ -170,6 +185,22 @@ describe("translation support", () => { await mount(SomeComponent, fixture, { translateFn }); expect(fixture.innerHTML).toBe("translated"); }); + + test("t-translation with several children", async () => { + class SomeComponent extends Component { + static template = xml` +
+ +
+
+ + +
+ `; + } + await mount(SomeComponent, fixture); + expect(fixture.outerHTML).toBe("
"); + }); }); describe("translation context", () => { @@ -293,4 +324,20 @@ describe("translation context", () => { expect(translateFn).toHaveBeenCalledWith("param", "fr"); expect(translateFn).toHaveBeenCalledWith("title", "pt"); }); + + test("t-translation-context with several children", async () => { + class SomeComponent extends Component { + static template = xml` +
+ +
+
+ + +
+ `; + } + await mount(SomeComponent, fixture); + expect(fixture.outerHTML).toBe("
"); + }); });