[FIX] evaluator: Prevent incorrect invalidation of spread

This commit solves an issue with the invalidation process of spreaded
formulas. While marking the positions invalidated by a given position,
we would mistakenly mark the latter to be recomputed as well,
effectively creating an infinite loop (position is invalid > invalidate
its dependencies > position is invalidated > etc ...).

This issue was introduced by a bugfix in #3125
and was not discovered as the infinite loop is actually stopped by our
maximum iteration limit.

Avoiding these useless iterations implies a performance improvement of a
factor 30 when reevaluating a spreaded formula.

closes #4129

Task: 3883954
X-original-commit: 47bb1a9
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Co-authored-by: Lucas Lefèvre <lul@odoo.com>

src/plugins/ui_core_views/cell_evaluation/evaluator.ts

@@ -130,7 +130,7 @@ export class Evaluator {
       }
       if (!content) {
         // The previous content could have blocked some array formulas
-        impactedPositions.addMany(this.getArrayFormulasBlockedByOrSpreadingOn(position));
+        impactedPositions.addMany(this.getArrayFormulasBlockedBy(position));
       }
     }
     return impactedPositions;
@@ -187,14 +187,23 @@ export class Evaluator {
     return positions;
   }
 
-  private getArrayFormulasBlockedByOrSpreadingOn(position: CellPosition): Iterable<CellPosition> {
+  /**
+   * Return the position of formulas blocked by the given position
+   * as well as all their dependencies.
+   */
+  private getArrayFormulasBlockedBy(position: CellPosition): Iterable<CellPosition> {
     if (!this.spreadingRelations.hasArrayFormulaResult(position)) {
       return [];
     }
     const arrayFormulas = this.spreadingRelations.getFormulaPositionsSpreadingOn(position);
     const positions = this.createEmptyPositionSet();
     positions.addMany(arrayFormulas);
-    positions.addMany(this.getCellsDependingOn(arrayFormulas));
+    const arrayFormulaPosition = this.getArrayFormulaSpreadingOn(position);
+    if (arrayFormulaPosition) {
+      // ignore the formula spreading on the position. Keep only the blocked ones
+      positions.delete(arrayFormulaPosition);
+    }
+    positions.addMany(this.getCellsDependingOn(positions));
     return positions;
   }
 
@@ -401,7 +410,7 @@ export class Evaluator {
       }
       this.evaluatedCells.delete(child);
       this.nextPositionsToUpdate.addMany(this.getCellsDependingOn([child]));
-      this.nextPositionsToUpdate.addMany(this.getArrayFormulasBlockedByOrSpreadingOn(child));
+      this.nextPositionsToUpdate.addMany(this.getArrayFormulasBlockedBy(child));
     }
     this.spreadingRelations.removeNode(position);
   }

src/plugins/ui_core_views/cell_evaluation/formula_dependency_graph.ts

@@ -54,7 +54,7 @@ export class FormulaDependencyGraph {
   }
 
   /**
-   * Return the cell and all cells that depend on it,
+   * Return all the cells that depend on the provided ranges,
    * in the correct order they should be evaluated.
    * This is called a topological ordering (excluding cycles)
    */

tests/evaluation/evaluation_formula_array.test.ts

@@ -588,8 +588,74 @@ describe("evaluate formulas that return an array", () => {
       setCellContent(model, "C1", "=MFILL(2,1,B1+1)");
       expect(getEvaluatedCell(model, "A1").value).toBe(31);
       expect(getEvaluatedCell(model, "B1").value).toBe(31);
-      expect(getEvaluatedCell(model, "C1").value).toBe(32);
-      expect(getEvaluatedCell(model, "D1").value).toBe(32);
+      expect(getEvaluatedCell(model, "C1").value).toBe(30);
+      expect(getEvaluatedCell(model, "D1").value).toBe(30);
+    });
+
+    test("Spreaded formulas with range deps Do not invalidate themselves on evaluation", () => {
+      let c = 0;
+      functionRegistry.add("INCREMENTONEVAL", {
+        description: "returns the input, but fancy. Like transpose(transpose(range))",
+        args: [arg("range (any, range<any>)", "The matrix to be transposed.")],
+        returns: ["RANGE<ANY>"],
+        compute: function (values) {
+          c++;
+          return 5;
+        },
+        isExported: true,
+      });
+      setCellContent(model, "A1", "0");
+      setCellContent(model, "A2", "1");
+      setCellContent(model, "A3", "2");
+      setCellContent(model, "A4", "3");
+      setCellContent(model, "A5", "=INCREMENTONEVAL(A2:A3)");
+      expect(c).toEqual(1);
+      setCellContent(model, "A5", "=INCREMENTONEVAL(A1:A2)");
+      expect(c).toEqual(2);
+      setCellContent(model, "A5", "=INCREMENTONEVAL(A1:B2)");
+      expect(c).toEqual(3);
+    });
+
+    test("Spreaded formulas with range deps invalidate only once the dependencies of themselves", () => {
+      let c = 0;
+      functionRegistry.add("INCREMENTONEVAL", {
+        description: "",
+        args: [arg("range (any, range<any>)", "")],
+        returns: ["RANGE<ANY>"],
+        compute: function () {
+          c++;
+          return 5;
+        },
+        isExported: false,
+      });
+      setCellContent(model, "A1", "0");
+      setCellContent(model, "A2", "1");
+      setCellContent(model, "A5", "=TRANSPOSE(A1:A2)");
+      setCellContent(model, "B1", "=INCREMENTONEVAL(A5)"); // depends on array formula (main cell)
+      expect(c).toEqual(1);
+      setCellContent(model, "A2", "2");
+      expect(c).toEqual(2);
+    });
+
+    test("Spreaded formulas with range deps invalidate only once the dependencies of result", () => {
+      let c = 0;
+      functionRegistry.add("INCREMENTONEVAL", {
+        description: "",
+        args: [arg("range (any, range<any>)", "")],
+        returns: ["RANGE<ANY>"],
+        compute: function () {
+          c++;
+          return 5;
+        },
+        isExported: false,
+      });
+      setCellContent(model, "A1", "0");
+      setCellContent(model, "A2", "1");
+      setCellContent(model, "A5", "=TRANSPOSE(A1:A2)");
+      setCellContent(model, "B1", "=INCREMENTONEVAL(B5)"); // depends on array formula (not the main cell)
+      expect(c).toEqual(1);
+      setCellContent(model, "A2", "2");
+      expect(c).toEqual(2);
     });
 
     test("have collision when spread size zone change", () => {

tests/test_helpers/helpers.ts

@@ -9,6 +9,7 @@ import {
 } from "@odoo/owl";
 import type { ChartConfiguration } from "chart.js";
 import format from "xml-formatter";
+import { functionCache } from "../../src";
 import { Action } from "../../src/actions/action";
 import { Composer, ComposerProps } from "../../src/components/composer/composer/composer";
 import {
@@ -485,6 +486,9 @@ export function clearFunctions() {
 }
 
 export function restoreDefaultFunctions() {
+  for (let f in functionCache) {
+    delete functionCache[f];
+  }
   clearFunctions();
   Object.keys(functionMapRestore).forEach((k) => {
     functionMap[k] = functionMapRestore[k];