From b2aa847b681d2c840b93190f36ce6fdbbb5c424d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= Date: Wed, 13 Nov 2024 16:33:14 +0100 Subject: [PATCH] [FIX] evaluation: Ensure dependency invalidation for array formulas How to reproduce: -> see the attached test What is happening? During the evaluation of SUMIFS, it will fetch its ranges. First it will first fetch an empty range (E4:E7) since MUNIT hasn't been evaluated yet. Afterwards, it will fetch H4, which in turn will request C4 and therefore force the evaluation of the spreaded function MUNIT. AT this point, there is a disparity of values because the values of the range (E4:E7) have changed. To address those situations, the evaluator invalidates the dependencies of all the spreaded zone. Which means marking A1 to be recomputed. Unfortunately, A1 is still being evaluated and since #4589 (specifically commit 8d22e86) we remove the cell from the queue after we evaluted it. This revision changes the approach taken in 8d22e86 to allow legitimate invalidations take place while avoiding useless reevaluations. Task: 4252800 --- src/functions/index.ts | 22 +++++++++++++++++-- .../cell_evaluation/evaluator.ts | 5 ++++- .../evaluation_formula_array.test.ts | 19 +++++++++++++--- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/functions/index.ts b/src/functions/index.ts index 9a7e05e739..655ae46e6f 100644 --- a/src/functions/index.ts +++ b/src/functions/index.ts @@ -5,6 +5,7 @@ import { Arg, CellValue, ComputeFunction, + DEFAULT_LOCALE, EvalContext, FPayload, FunctionDescription, @@ -12,8 +13,9 @@ import { isMatrix, } from "../types"; import { BadExpressionError, EvaluationError } from "../types/errors"; -import { addMetaInfoFromArg, validateArguments } from "./arguments"; -import { isEvaluationError, matrixForEach, matrixMap } from "./helpers"; +import { addMetaInfoFromArg, arg, validateArguments } from "./arguments"; +import { toScalar } from "./helper_matrices"; +import { isEvaluationError, matrixForEach, matrixMap, toNumber } from "./helpers"; import * as array from "./module_array"; import * as misc from "./module_custom"; import * as database from "./module_database"; @@ -222,3 +224,19 @@ for (let category of categories) { functionRegistry.add(name, { isExported: false, ...addDescr }); } } + +functionRegistry.add("MFILL", { + description: "Return an n*n matrix filled with n.", + args: [ + arg("n (number)", "number of column of the matrix"), + arg("m (number)", "number of row of the matrix"), + arg("v (number)", "value to fill matrix"), + ], + returns: ["RANGE"], + compute: function (n, m, v): number[][] { + const _n = toNumber(toScalar(n), DEFAULT_LOCALE); + const _m = toNumber(toScalar(m), DEFAULT_LOCALE); + const _v = toNumber(toScalar(v), DEFAULT_LOCALE); + return Array.from({ length: _n }, (_, i) => Array.from({ length: _m }, (_, j) => _v)); + }, +}); diff --git a/src/plugins/ui_core_views/cell_evaluation/evaluator.ts b/src/plugins/ui_core_views/cell_evaluation/evaluator.ts index dcab6066f4..6cee36adf9 100644 --- a/src/plugins/ui_core_views/cell_evaluation/evaluator.ts +++ b/src/plugins/ui_core_views/cell_evaluation/evaluator.ts @@ -228,6 +228,9 @@ export class Evaluator { } for (let i = 0; i < positions.length; ++i) { const position = positions[i]; + if (this.nextPositionsToUpdate.has(position)) { + continue; + } const evaluatedCell = this.computeCell(position); if (evaluatedCell !== EMPTY_CELL) { this.evaluatedCells.set(position, evaluatedCell); @@ -267,7 +270,6 @@ export class Evaluator { return createEvaluatedCell(e); } finally { this.cellsBeingComputed.delete(cellId); - this.nextPositionsToUpdate.delete(position); } } @@ -346,6 +348,7 @@ export class Evaluator { { sheetId, zone: rightPartZone }, { sheetId, zone: leftColumnZone }, ]); + invalidatedPositions.delete(arrayFormulaPosition); this.nextPositionsToUpdate.addMany(invalidatedPositions); } diff --git a/tests/evaluation/evaluation_formula_array.test.ts b/tests/evaluation/evaluation_formula_array.test.ts index 5044c2c55a..486d41d3c2 100644 --- a/tests/evaluation/evaluation_formula_array.test.ts +++ b/tests/evaluation/evaluation_formula_array.test.ts @@ -641,6 +641,19 @@ describe("evaluate formulas that return an array", () => { expect(mockCompute).toHaveBeenCalledTimes(3); }); + test("Formula depending on array formula is reevaluated when the array formula result changes", () => { + const model = new Model(); + setCellContent(model, "A1", "=sumifs(E4:E7,H4:H7,1)"); + setCellContent(model, "C4", "=MUNIT(4)"); + setCellContent(model, "H4", "=C4"); + setCellContent(model, "H6", "=E6"); + expect(getEvaluatedCell(model, "A1").value).toBe(1); + + // Force a reevaluation to avoid the incremental evaluation following each update_cell + model.dispatch("EVALUATE_CELLS"); + expect(getEvaluatedCell(model, "A1").value).toBe(1); + }); + test("Spreaded formulas with range deps invalidate only once the dependencies of themselves", () => { let c = 0; functionRegistry.add("INCREMENTONEVAL", { @@ -696,9 +709,9 @@ describe("evaluate formulas that return an array", () => { setCellContent(model, "A1", "2"); - expect(getEvaluatedCell(model, "B1").value).toBe("#ERROR"); - expect(getEvaluatedCell(model, "B2").value).toBe(null); - expect(getEvaluatedCell(model, "B3").value).toBe(0); + expect(getEvaluatedCell(model, "B1").value).toBe(42); + expect(getEvaluatedCell(model, "B2").value).toBe(42); + expect(getEvaluatedCell(model, "B3").value).toBe(42); expect(getEvaluatedCell(model, "A3").value).toBe("#ERROR"); });