[FIX] evaluation: behavior for invalid range arguments

When user provides an invalid sheet name in range arguments of a function,
it performs computations on data of specified range from active sheet by default
and returns misleading results.

This commit changes the behavior to throw an error instead of performing any
kind of computations.

Task ID : 3619144

closes #3511

X-original-commit: 6bf6976
Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com>

src/plugins/ui/evaluation.ts

@@ -247,12 +247,9 @@ export class EvaluationPlugin extends UIPlugin {
      * that are actually present in the grid.
      */
     function range(range: Range): MatrixArg {
+      assertRangeValid(range);
       const sheetId = range.sheetId;
 
-      if (!isZoneValid(range.zone)) {
-        throw new InvalidReferenceError();
-      }
-
       // Performance issue: Avoid fetching data on positions that are out of the spreadsheet
       // e.g. A1:ZZZ9999 in a sheet with 10 cols and 10 rows should ignore everything past J10 and return a 10x10 array
       const sheetZone = getters.getSheetZone(sheetId);
@@ -299,15 +296,12 @@ export class EvaluationPlugin extends UIPlugin {
       functionName: string,
       paramNumber?: number
     ): PrimitiveArg {
+      assertRangeValid(range);
       if (isMeta) {
         // Use zoneToXc of zone instead of getRangeString to avoid sending unbounded ranges
         return { value: zoneToXc(range.zone) };
       }
 
-      if (!isZoneValid(range.zone)) {
-        throw new InvalidReferenceError();
-      }
-
       // if the formula definition could have accepted a range, we would pass through the _range function and not here
       if (range.zone.bottom !== range.zone.top || range.zone.left !== range.zone.right) {
         throw new Error(
@@ -324,11 +318,16 @@ export class EvaluationPlugin extends UIPlugin {
         );
       }
 
+      return readCell(range);
+    }
+
+    function assertRangeValid(range: Range): void {
+      if (!isZoneValid(range.zone)) {
+        throw new InvalidReferenceError();
+      }
       if (range.invalidSheetName) {
         throw new Error(_lt("Invalid sheet name: %s", range.invalidSheetName));
       }
-
-      return readCell(range);
     }
     return [refFn, range, evalContext];
   }

tests/formulas/compiler.test.ts

@@ -290,7 +290,7 @@ describe("compile functions", () => {
       setCellContent(m, "B4", "=NUMBEREXPECTED(A1:A2)");
       setCellContent(m, "B5", "=STRINGEXPECTED(A1:A2)");
       setCellContent(m, "B6", "=ANYEXPECTED(A1:A$2)");
-      setCellContent(m, "B7", "=ANYEXPECTED(sheet2!A1:A$2)");
+      setCellContent(m, "B7", "=ANYEXPECTED(sheet1!A1:A$2)");
       setCellContent(m, "B8", "=A2:A3");
       setCellContent(m, "B9", "=+A2:A3");
       setCellContent(m, "B10", "=A1+A2:A3");

tests/functions/module_lookup.test.ts

@@ -20,14 +20,18 @@ describe("COLUMN formula", () => {
     expect(evaluateCell("A1", { A1: "=COLUMN(G2)" })).toBe(7);
     expect(evaluateCell("A1", { A1: "=COLUMN(ABC2)" })).toBe(731);
     expect(evaluateCell("A1", { A1: "=COLUMN($ABC$2)" })).toBe(731);
-    expect(evaluateCell("A1", { A1: "=COLUMN(Sheet42!$ABC$2)" })).toBe(731);
+    expect(evaluateCell("A1", { A1: "=COLUMN(Sheet1!$ABC$2)" })).toBe(731);
   });
 
   test("functional tests on range arguments", () => {
     expect(evaluateCell("A1", { A1: "=COLUMN(B3:C40)" })).toBe(2);
     expect(evaluateCell("A1", { A1: "=COLUMN(D3:Z9)" })).toBe(4);
     expect(evaluateCell("A1", { A1: "=COLUMN($D$3:$Z$9)" })).toBe(4);
-    expect(evaluateCell("A1", { A1: "=COLUMN(Sheet42!$D$3:$Z$9)" })).toBe(4);
+    expect(evaluateCell("A1", { A1: "=COLUMN(Sheet1!$D$3:$Z$9)" })).toBe(4);
+  });
+
+  test("functional tests on range arguments with invalid sheet name", () => {
+    expect(evaluateCell("A1", { A1: "=COLUMN(Sheet42!ABC2)" })).toBe("#ERROR"); // @compatibility: on google sheets, return #REF!
   });
 });
 
@@ -41,14 +45,18 @@ describe("COLUMNS formula", () => {
     expect(evaluateCell("A1", { A1: "=COLUMNS(H2)" })).toBe(1);
     expect(evaluateCell("A1", { A1: "=COLUMNS(ABC2)" })).toBe(1);
     expect(evaluateCell("A1", { A1: "=COLUMNS($ABC$2)" })).toBe(1);
-    expect(evaluateCell("A1", { A1: "=COLUMNS(Sheet42!$ABC$2)" })).toBe(1);
+    expect(evaluateCell("A1", { A1: "=COLUMNS(Sheet1!$ABC$2)" })).toBe(1);
   });
 
   test("functional tests on range arguments", () => {
     expect(evaluateCell("A1", { A1: "=COLUMNS(B3:C40)" })).toBe(2);
     expect(evaluateCell("A1", { A1: "=COLUMNS(D3:Z9)" })).toBe(23);
     expect(evaluateCell("A1", { A1: "=COLUMNS($D$3:$Z$9)" })).toBe(23);
-    expect(evaluateCell("A1", { A1: "=COLUMNS(Sheet42!$D$3:$Z$9)" })).toBe(23);
+    expect(evaluateCell("A1", { A1: "=COLUMNS(Sheet1!$D$3:$Z$9)" })).toBe(23);
+  });
+
+  test("functional tests on range arguments with invalid sheet name", () => {
+    expect(evaluateCell("A1", { A1: "=COLUMNS(Sheet42!ABC2)" })).toBe("#ERROR"); // @compatibility: on google sheets, return #REF!
   });
 });
 
@@ -338,14 +346,18 @@ describe("ROW formula", () => {
     expect(evaluateCell("A1", { A1: "=ROW(H2)" })).toBe(2);
     expect(evaluateCell("A1", { A1: "=ROW(A234)" })).toBe(234);
     expect(evaluateCell("A1", { A1: "=ROW($A$234)" })).toBe(234);
-    expect(evaluateCell("A1", { A1: "=ROW(Sheet42!$A$234)" })).toBe(234);
+    expect(evaluateCell("A1", { A1: "=ROW(Sheet1!$A$234)" })).toBe(234);
   });
 
   test("functional tests on range arguments", () => {
     expect(evaluateCell("A1", { A1: "=ROW(B3:C40)" })).toBe(3);
     expect(evaluateCell("A1", { A1: "=ROW(D3:Z9)" })).toBe(3);
     expect(evaluateCell("A1", { A1: "=ROW($D$3:$Z$9)" })).toBe(3);
-    expect(evaluateCell("A1", { A1: "=ROW(Sheet42!$D$3:$Z$9)" })).toBe(3);
+    expect(evaluateCell("A1", { A1: "=ROW(Sheet1!$D$3:$Z$9)" })).toBe(3);
+  });
+
+  test("functional tests on range arguments with invalid sheet name", () => {
+    expect(evaluateCell("A1", { A1: "=ROW(Sheet42!A234)" })).toBe("#ERROR"); // @compatibility: on google sheets, return #REF!
   });
 });
 
@@ -359,14 +371,18 @@ describe("ROWS formula", () => {
     expect(evaluateCell("A1", { A1: "=ROWS(H2)" })).toBe(1);
     expect(evaluateCell("A1", { A1: "=ROWS(ABC2)" })).toBe(1);
     expect(evaluateCell("A1", { A1: "=ROWS($ABC$2)" })).toBe(1);
-    expect(evaluateCell("A1", { A1: "=ROWS(Sheet42!$ABC$2)" })).toBe(1);
+    expect(evaluateCell("A1", { A1: "=ROWS(Sheet1!$ABC$2)" })).toBe(1);
   });
 
   test("functional tests on range arguments", () => {
     expect(evaluateCell("A1", { A1: "=ROWS(B3:C40)" })).toBe(38);
     expect(evaluateCell("A1", { A1: "=ROWS(D3:Z9)" })).toBe(7);
     expect(evaluateCell("A1", { A1: "=ROWS($D$3:$Z$9)" })).toBe(7);
-    expect(evaluateCell("A1", { A1: "=ROWS(Sheet42!$D$3:$Z$9)" })).toBe(7);
+    expect(evaluateCell("A1", { A1: "=ROWS(Sheet1!$D$3:$Z$9)" })).toBe(7);
+  });
+
+  test("functional tests on range arguments with invalid sheet name", () => {
+    expect(evaluateCell("A1", { A1: "=ROWS(Sheet42!ABC2)" })).toBe("#ERROR"); // @compatibility: on google sheets, return #REF!
   });
 });