Skip to content

Commit 3850237

Browse files
laa-odoolaa-odoo
committed
[REF] evaluation : remove the meta argument concept
This commit simplifies the code by removing the need for meta-concepts. Reference coordinates are now directly accessible from FunctionResultObject-typed arguments. Task: 5428577
1 parent 94f59e6 commit 3850237

File tree

10 files changed

+29
-211
lines changed

10 files changed

+29
-211
lines changed

‎packages/o-spreadsheet-engine/src/formulas/compiler.ts‎

Lines changed: 6 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -138,11 +138,9 @@ function compileTokensOrThrow(tokens: Token[]): CompiledFormula {
138138
const currentArg = args[i];
139139
const argTypes = argDefinition.type || [];
140140

141-
// detect when an argument need to be evaluated as a meta argument
142-
const isMeta = argTypes.includes("META") || argTypes.includes("RANGE<META>");
143141
const hasRange = argTypes.some((t) => isRangeType(t));
144142

145-
compiledArgs.push(compileAST(currentArg, isMeta, hasRange));
143+
compiledArgs.push(compileAST(currentArg, hasRange));
146144
}
147145

148146
return compiledArgs;
@@ -152,21 +150,9 @@ function compileTokensOrThrow(tokens: Token[]): CompiledFormula {
152150
* This function compiles all the information extracted by the parser into an
153151
* executable code for the evaluation of the cells content. It uses a cache to
154152
* not reevaluate identical code structures.
155-
*
156-
* The function is sensitive to parameter “isMeta”. This
157-
* parameter may vary when compiling function arguments:
158-
* isMeta: In some cases the function arguments expects information on the
159-
* cell/range other than the associated value(s). For example the COLUMN
160-
* function needs to receive as argument the coordinates of a cell rather
161-
* than its value. For this we have meta arguments.
162153
*/
163-
function compileAST(ast: AST, isMeta = false, hasRange = false): FunctionCode {
154+
function compileAST(ast: AST, hasRange = false): FunctionCode {
164155
const code = new FunctionCodeBuilder(scope);
165-
if (ast.type !== "REFERENCE" && !(ast.type === "BIN_OPERATION" && ast.value === ":")) {
166-
if (isMeta) {
167-
throw new BadExpressionError(_t("Argument must be a reference to a cell or range."));
168-
}
169-
}
170156
if (ast.debug) {
171157
code.append("debugger;");
172158
code.append(`ctx["debug"] = true;`);
@@ -180,9 +166,7 @@ function compileTokensOrThrow(tokens: Token[]): CompiledFormula {
180166
return code.return(`this.literalValues.strings[${stringCount++}]`);
181167
case "REFERENCE":
182168
return code.return(
183-
`${ast.value.includes(":") || hasRange ? `range` : `ref`}(deps[${dependencyCount++}], ${
184-
isMeta ? "true" : "false"
185-
})`
169+
`${ast.value.includes(":") || hasRange ? `range` : `ref`}(deps[${dependencyCount++}])`
186170
);
187171
case "FUNCALL":
188172
const args = compileFunctionArgs(ast).map((arg) => arg.assignResultToVariable());
@@ -208,14 +192,14 @@ function compileTokensOrThrow(tokens: Token[]): CompiledFormula {
208192
}
209193
case "UNARY_OPERATION": {
210194
const fnName = UNARY_OPERATOR_MAP[ast.value];
211-
const operand = compileAST(ast.operand, false, false).assignResultToVariable();
195+
const operand = compileAST(ast.operand, false).assignResultToVariable();
212196
code.append(operand);
213197
return code.return(`ctx['${fnName}'](${operand.returnExpression})`);
214198
}
215199
case "BIN_OPERATION": {
216200
const fnName = OPERATOR_MAP[ast.value];
217-
const left = compileAST(ast.left, false, false).assignResultToVariable();
218-
const right = compileAST(ast.right, false, false).assignResultToVariable();
201+
const left = compileAST(ast.left, false).assignResultToVariable();
202+
const right = compileAST(ast.right, false).assignResultToVariable();
219203
code.append(left);
220204
code.append(right);
221205
return code.return(

‎packages/o-spreadsheet-engine/src/functions/arguments.ts‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@ const ARG_TYPES: ArgType[] = [
2222
"RANGE<DATE>",
2323
"RANGE<NUMBER>",
2424
"RANGE<STRING>",
25-
"META",
26-
"RANGE<META>",
2725
];
2826

2927
export function arg(
@@ -289,8 +287,6 @@ export function _argTargeting(
289287
// Argument validation
290288
//------------------------------------------------------------------------------
291289

292-
const META_TYPES: ArgType[] = ["META", "RANGE<META>"];
293-
294290
export function validateArguments(descr: FunctionDescription) {
295291
if (descr.nbrArgRepeating && descr.nbrOptionalNonRepeatingArgs >= descr.nbrArgRepeating) {
296292
throw new Error(`Function ${descr.name} has more optional arguments than repeatable ones.`);
@@ -299,15 +295,6 @@ export function validateArguments(descr: FunctionDescription) {
299295
let foundRepeating = false;
300296
let consecutiveRepeating = false;
301297
for (const current of descr.args) {
302-
if (
303-
current.type.some((t) => META_TYPES.includes(t)) &&
304-
current.type.some((t) => !META_TYPES.includes(t))
305-
) {
306-
throw new Error(
307-
`Function ${descr.name} has a mix of META and non-META types in the same argument: ${current.type}.`
308-
);
309-
}
310-
311298
if (current.repeating) {
312299
if (!consecutiveRepeating && foundRepeating) {
313300
throw new Error(

‎packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/compilation_parameters.ts‎

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
import { functionRegistry } from "../../../functions/function_registry";
2-
import { getFullReference } from "../../../helpers";
3-
import { toXC } from "../../../helpers/coordinates";
4-
import { intersection, isZoneValid, zoneToXc } from "../../../helpers/zones";
2+
import { intersection, isZoneValid } from "../../../helpers/zones";
53
import { _t } from "../../../translation";
64
import { EvaluatedCell } from "../../../types/cells";
75
import { EvaluationError, InvalidReferenceError } from "../../../types/errors";
@@ -67,23 +65,14 @@ class CompilationParametersBuilder {
6765
* Returns the value of the cell(s) used in reference
6866
*
6967
* @param range the references used
70-
* @param isMeta if a reference is supposed to be used in a `meta` parameter as described in the
71-
* function for which this parameter is used, we just return the string of the parameter.
72-
* The `compute` of the formula's function must process it completely
7368
*/
74-
private refFn(range: Range, isMeta: boolean): FunctionResultObject {
69+
private refFn(range: Range): FunctionResultObject {
7570
const rangeError = this.getRangeError(range);
7671
if (rangeError) {
7772
return rangeError;
7873
}
7974
// the compiler guarantees only single cell ranges reach this part of the code
8075
const position = { sheetId: range.sheetId, col: range.zone.left, row: range.zone.top };
81-
if (isMeta) {
82-
this.computeCell(position); // ensure the cell is computed if the function using the meta parameter uses the value
83-
// Use zoneToXc of zone instead of getRangeString to avoid sending unbounded ranges
84-
const sheetName = this.getters.getSheetName(range.sheetId);
85-
return { value: getFullReference(sheetName, zoneToXc(range.zone)) };
86-
}
8776
return { ...this.computeCell(position), position };
8877
}
8978

@@ -95,7 +84,7 @@ class CompilationParametersBuilder {
9584
* Note that each col is possibly sparse: it only contain the values of cells
9685
* that are actually present in the grid.
9786
*/
98-
private range(range: Range, isMeta: boolean): Matrix<FunctionResultObject> {
87+
private range(range: Range): Matrix<FunctionResultObject> {
9988
const rangeError = this.getRangeError(range);
10089
if (rangeError) {
10190
return [[rangeError]];
@@ -111,15 +100,14 @@ class CompilationParametersBuilder {
111100
return [[]];
112101
}
113102
const { top, left, bottom, right } = zone;
114-
const cacheKey = `${sheetId}-${top}-${left}-${bottom}-${right}-${isMeta}`;
103+
const cacheKey = `${sheetId}-${top}-${left}-${bottom}-${right}`;
115104
if (cacheKey in this.rangeCache) {
116105
return this.rangeCache[cacheKey];
117106
}
118107

119108
const height = _zone.bottom - _zone.top + 1;
120109
const width = _zone.right - _zone.left + 1;
121110
const matrix: Matrix<FunctionResultObject> = new Array(width);
122-
const sheetName = this.getters.getSheetName(range.sheetId);
123111

124112
// Performance issue: nested loop is faster than a map here
125113
for (let col = _zone.left; col <= _zone.right; col++) {
@@ -128,9 +116,7 @@ class CompilationParametersBuilder {
128116
for (let row = _zone.top; row <= _zone.bottom; row++) {
129117
const rowIndex = row - _zone.top;
130118
const computedCell = this.computeCell({ sheetId, col, row });
131-
matrix[colIndex][rowIndex] = isMeta
132-
? { value: getFullReference(sheetName, toXC(col, row)) }
133-
: { ...computedCell, position: { sheetId, col, row } };
119+
matrix[colIndex][rowIndex] = { ...computedCell, position: { sheetId, col, row } };
134120
}
135121
}
136122

‎packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluator.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ export class Evaluator {
118118
this.formulaDependencies().addDependencies(position, dependencies);
119119
for (const range of dependencies) {
120120
// ensure that all ranges are computed
121-
this.compilationParams.ensureRange(range, false);
121+
this.compilationParams.ensureRange(range);
122122
}
123123
}
124124

‎packages/o-spreadsheet-engine/src/types/functions.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,7 @@ export type ArgType =
1515
| "RANGE<NUMBER>"
1616
| "RANGE<DATE>"
1717
| "RANGE<STRING>"
18-
| "RANGE<ANY>"
19-
| "META"
20-
| "RANGE<META>";
18+
| "RANGE<ANY>";
2119

2220
export interface ArgDefinition {
2321
acceptMatrix?: boolean;

‎packages/o-spreadsheet-engine/src/types/misc.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,12 +174,11 @@ export interface Border {
174174

175175
export type ReferenceDenormalizer = (
176176
range: Range,
177-
isMeta: boolean,
178177
functionName: string,
179178
paramNumber: number
180179
) => FunctionResultObject;
181180

182-
export type EnsureRange = (range: Range, isMeta: boolean) => Matrix<FunctionResultObject>;
181+
export type EnsureRange = (range: Range) => Matrix<FunctionResultObject>;
183182

184183
export type GetSymbolValue = (symbolName: string) => Arg;
185184

‎tests/evaluation/__snapshots__/compiler.test.ts.snap‎

Lines changed: 12 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -53,24 +53,6 @@ return ctx['ADD'](_1, _2);
5353
}"
5454
`;
5555

56-
exports[`compile functions with meta arguments function call requesting meta parameter 1`] = `
57-
"function anonymous(deps,ref,range,getSymbolValue,ctx
58-
) {
59-
// =USEMETAARG(|C|)
60-
const _1 = range(deps[0], true);
61-
return ctx['USEMETAARG'](_1);
62-
}"
63-
`;
64-
65-
exports[`compile functions with meta arguments function call requesting meta parameter 2`] = `
66-
"function anonymous(deps,ref,range,getSymbolValue,ctx
67-
) {
68-
// =USEMETAARG(|C|)
69-
const _1 = range(deps[0], true);
70-
return ctx['USEMETAARG'](_1);
71-
}"
72-
`;
73-
7456
exports[`expression compiler array literal expression 1`] = `
7557
"function anonymous(deps,ref,range,getSymbolValue,ctx
7658
) {
@@ -118,7 +100,7 @@ exports[`expression compiler cells are converted to ranges if function require a
118100
"function anonymous(deps,ref,range,getSymbolValue,ctx
119101
) {
120102
// =sum(|C|)
121-
const _1 = range(deps[0], false);
103+
const _1 = range(deps[0]);
122104
return ctx['SUM'](_1);
123105
}"
124106
`;
@@ -127,10 +109,10 @@ exports[`expression compiler expression with $ref 1`] = `
127109
"function anonymous(deps,ref,range,getSymbolValue,ctx
128110
) {
129111
// =|C|+|C|+|C|
130-
const _1 = ref(deps[0], false);
131-
const _2 = ref(deps[1], false);
112+
const _1 = ref(deps[0]);
113+
const _2 = ref(deps[1]);
132114
const _3 = ctx['ADD'](_1, _2);
133-
const _4 = ref(deps[2], false);
115+
const _4 = ref(deps[2]);
134116
return ctx['ADD'](_3, _4);
135117
}"
136118
`;
@@ -139,7 +121,7 @@ exports[`expression compiler expression with references with a sheet 1`] = `
139121
"function anonymous(deps,ref,range,getSymbolValue,ctx
140122
) {
141123
// =|C|
142-
return ref(deps[0], false);
124+
return ref(deps[0]);
143125
}"
144126
`;
145127

@@ -149,7 +131,7 @@ exports[`expression compiler expressions with a debugger 1`] = `
149131
// =?|C|/|N|
150132
debugger;
151133
ctx["debug"] = true;
152-
const _1 = ref(deps[0], false);
134+
const _1 = ref(deps[0]);
153135
const _2 = this.literalValues.numbers[0];
154136
return ctx['DIVIDE'](_1, _2);
155137
}"
@@ -159,8 +141,8 @@ exports[`expression compiler read some values and functions 1`] = `
159141
"function anonymous(deps,ref,range,getSymbolValue,ctx
160142
) {
161143
// =|C|+sum(|R|)
162-
const _1 = ref(deps[0], false);
163-
const _2 = range(deps[1], false);
144+
const _1 = ref(deps[0]);
145+
const _2 = range(deps[1]);
164146
const _3 = ctx['SUM'](_2);
165147
return ctx['ADD'](_1, _3);
166148
}"
@@ -309,7 +291,7 @@ exports[`expression compiler some arithmetic expressions 15`] = `
309291
"function anonymous(deps,ref,range,getSymbolValue,ctx
310292
) {
311293
// =|C|%
312-
const _1 = ref(deps[0], false);
294+
const _1 = ref(deps[0]);
313295
return ctx['UNARY.PERCENT'](_1);
314296
}"
315297
`;
@@ -318,9 +300,9 @@ exports[`expression compiler with the same reference multiple times 1`] = `
318300
"function anonymous(deps,ref,range,getSymbolValue,ctx
319301
) {
320302
// =SUM(|C|,|C|,|C|)
321-
const _1 = range(deps[0], false);
322-
const _2 = range(deps[1], false);
323-
const _3 = range(deps[2], false);
303+
const _1 = range(deps[0]);
304+
const _2 = range(deps[1]);
305+
const _3 = range(deps[2]);
324306
return ctx['SUM'](_1,_2,_3);
325307
}"
326308
`;

‎tests/evaluation/compiler.test.ts‎

Lines changed: 1 addition & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { functionRegistry } from "@odoo/o-spreadsheet-engine/functions/function_registry";
2-
import { compile, functionCache, Model } from "../../src";
2+
import { compile, functionCache } from "../../src";
33

4-
import { createValidRange } from "../../src/helpers";
54
import { CompiledFormula } from "../../src/types";
65
import { addToRegistry, evaluateCell, evaluateCellFormat } from "../test_helpers/helpers";
76

@@ -264,84 +263,6 @@ describe("compile functions", () => {
264263
});
265264
});
266265

267-
describe("with meta arguments", () => {
268-
beforeEach(() => {
269-
addToRegistry(functionRegistry, "USEMETAARG", {
270-
description: "function with a meta argument",
271-
compute: () => true,
272-
args: [{ name: "arg", description: "", type: ["META", "RANGE<META>"] }],
273-
});
274-
addToRegistry(functionRegistry, "NOTUSEMETAARG", {
275-
description: "any function",
276-
compute: () => true,
277-
args: [{ name: "arg", description: "", type: ["ANY"] }],
278-
});
279-
});
280-
281-
test.each(["=USEMETAARG(A1)", "=USEMETAARG(B2)"])(
282-
"function call requesting meta parameter",
283-
(formula) => {
284-
const compiledFormula = compiledBaseFunction(formula);
285-
expect(compiledFormula.execute.toString()).toMatchSnapshot();
286-
}
287-
);
288-
289-
test("throw error if parameter isn't cell/range reference", () => {
290-
expect(compiledBaseFunction("=USEMETAARG(X8)").isBadExpression).toBe(false);
291-
expect(compiledBaseFunction("=USEMETAARG($X$8)").isBadExpression).toBe(false);
292-
expect(compiledBaseFunction("=USEMETAARG(Sheet42!X8)").isBadExpression).toBe(false);
293-
expect(compiledBaseFunction("=USEMETAARG('Sheet 42'!X8)").isBadExpression).toBe(false);
294-
295-
expect(compiledBaseFunction("=USEMETAARG(D3:Z9)").isBadExpression).toBe(false);
296-
expect(compiledBaseFunction("=USEMETAARG($D$3:$Z$9)").isBadExpression).toBe(false);
297-
expect(compiledBaseFunction("=USEMETAARG(Sheet42!$D$3:$Z$9)").isBadExpression).toBe(false);
298-
expect(compiledBaseFunction("=USEMETAARG('Sheet 42'!D3:Z9)").isBadExpression).toBe(false);
299-
300-
expect(compiledBaseFunction('=USEMETAARG("kikou")').isBadExpression).toBe(true);
301-
expect(compiledBaseFunction('=USEMETAARG("")').isBadExpression).toBe(true);
302-
expect(compiledBaseFunction("=USEMETAARG(TRUE)").isBadExpression).toBe(true);
303-
expect(compiledBaseFunction("=USEMETAARG(SUM(1,2,3))").isBadExpression).toBe(true);
304-
});
305-
306-
test("do not care about the value of the cell / range passed as a reference", () => {
307-
const compiledFormula1 = compileFromCompleteFormula("=USEMETAARG(A1)");
308-
const compiledFormula2 = compileFromCompleteFormula("=USEMETAARG(A1:B2)");
309-
const compiledFormula3 = compileFromCompleteFormula("=NOTUSEMETAARG(A1)");
310-
const compiledFormula4 = compileFromCompleteFormula("=NOTUSEMETAARG(A1:B2)");
311-
312-
const m = new Model();
313-
314-
const refFn = jest.fn();
315-
const ensureRange = jest.fn();
316-
const getSymbolValue = jest.fn();
317-
318-
const ctx = { USEMETAARG: () => {}, NOTUSEMETAARG: () => {} };
319-
320-
const rangeA1 = createValidRange(m.getters, "ABC", "A1")!;
321-
const rangeA1ToB2 = createValidRange(m.getters, "ABC", "A1:B2")!;
322-
323-
compiledFormula1.execute([rangeA1], refFn, ensureRange, getSymbolValue, ctx);
324-
expect(refFn).toHaveBeenCalledTimes(0);
325-
expect(ensureRange).toHaveBeenCalledWith(rangeA1, true);
326-
ensureRange.mockReset();
327-
328-
compiledFormula2.execute([rangeA1ToB2], refFn, ensureRange, getSymbolValue, ctx);
329-
expect(refFn).toHaveBeenCalledTimes(0);
330-
expect(ensureRange).toHaveBeenCalledWith(rangeA1ToB2, true);
331-
ensureRange.mockReset();
332-
333-
compiledFormula3.execute([rangeA1], refFn, ensureRange, getSymbolValue, ctx);
334-
expect(refFn).toHaveBeenCalledWith(rangeA1, false);
335-
expect(ensureRange).toHaveBeenCalledTimes(0);
336-
refFn.mockReset();
337-
338-
compiledFormula4.execute([rangeA1ToB2], refFn, ensureRange, getSymbolValue, ctx);
339-
expect(refFn).toHaveBeenCalledTimes(0);
340-
expect(ensureRange).toHaveBeenCalledWith(rangeA1ToB2, false);
341-
refFn.mockReset();
342-
});
343-
});
344-
345266
test("function cache ignore spaces in functions", () => {
346267
compiledBaseFunction("=SUM(A1)");
347268
expect(Object.keys(functionCache)).toEqual(["=SUM(|C|)"]);

0 commit comments

Comments
 (0)