[IMP] compiler: catch argument shape errors

Steps to reproduce:
- type in a cell `=IFERROR(ADD(A1:A2,1),456)`
=> the cell value is #ERROR instead of 456.

The compiled formulas use to functions to get the values of references
- `ref` returns a single value from a single cell
- `range` returns a matrix of values

Currently, the compiler chooses `ref` or `range` based on the function arg.
e.g. for 'ADD(..., ...)', the compiler would choose `ref` because ADD only
accepts single cell values. If the user typed `ADD(A1:A3, 2)`, `ref` currently
checks the range size and throw an error if it's not a single cell. That
causes the entire compiled function to fail.
Notably: '=IFERROR(ADD(A1:A2,1), 45)' results in an error instead of 45.

One key observation is that `ref` is actually useless from a functional POV.
We could always use `range` no matter the shape of the arguments. There's
already a check for the input sizes for each sub-function (think of
'ADD(TRANSPOSE(...), 5)'. There is a performance issue though because `range`
always builds a 2D matrix. A POC was done to remove `ref` entirely and only
use `range` but it showed 2x increase of the evaluation time of the large
formula dataset.
With this commit, we keep `ref` and use it every time the reference is a single
cell, and (importantly) only in this case.

The logic of errors
===================

There are two types of errors in formulas:
- structural/compilation errors:
    If the formula will *never* be correct, no matter what changes in the spreadsheet,
    for example `=TODAY(456)` will never work because `TODAY` will never accept
    any argument.
    Those errors cannot be caught by IFERROR for a functional reason:
    warn the user the formula is wrong (and will always be) and that he should
    fix it.
- evaluation errors:
   Any other error (divisiion by zero, wrong type). They can be caught by
   IFERROR.
   In the context of this PR: `ADD(A1:A2,1)` is invalid because `ADD` does not
   accepts ranges. But it can become valid if row 2 is deleted.

Task: 3859327
Part-of: #4088
Co-authored-by: Alexis Lacroix <[email protected]>

src/formulas/compiler.ts

@@ -130,12 +130,7 @@ export function compileTokens(tokens: Token[]): CompiledFormula {
           }
         }
 
-        compiledArgs.push(
-          compileAST(currentArg, isMeta, hasRange, {
-            functionName,
-            paramIndex: i + 1,
-          })
-        );
+        compiledArgs.push(compileAST(currentArg, isMeta, hasRange));
       }
 
       return compiledArgs;
@@ -153,15 +148,7 @@ export function compileTokens(tokens: Token[]): CompiledFormula {
      * function needs to receive as argument the coordinates of a cell rather
      * than its value. For this we have meta arguments.
      */
-    function compileAST(
-      ast: AST,
-      isMeta = false,
-      hasRange = false,
-      referenceVerification: {
-        functionName?: string;
-        paramIndex?: number;
-      } = {}
-    ): FunctionCode {
+    function compileAST(ast: AST, isMeta = false, hasRange = false): FunctionCode {
       const code = new FunctionCodeBuilder(scope);
       if (ast.type !== "REFERENCE" && !(ast.type === "BIN_OPERATION" && ast.value === ":")) {
         if (isMeta) {
@@ -184,14 +171,10 @@ export function compileTokens(tokens: Token[]): CompiledFormula {
           );
         case "REFERENCE":
           const referenceIndex = dependencies.indexOf(ast.value);
-          if (hasRange) {
+          if ((!isMeta && ast.value.includes(":")) || hasRange) {
             return code.return(`range(deps[${referenceIndex}])`);
           } else {
-            return code.return(
-              `ref(deps[${referenceIndex}], ${isMeta ? "true" : "false"}, "${
-                referenceVerification.functionName || OPERATOR_MAP["="]
-              }",  ${referenceVerification.paramIndex})`
-            );
+            return code.return(`ref(deps[${referenceIndex}], ${isMeta ? "true" : "false"})`);
           }
         case "FUNCALL":
           const args = compileFunctionArgs(ast).map((arg) => arg.assignResultToVariable());
@@ -200,20 +183,14 @@ export function compileTokens(tokens: Token[]): CompiledFormula {
           return code.return(`ctx['${fnName}'](${args.map((arg) => arg.returnExpression)})`);
         case "UNARY_OPERATION": {
           const fnName = UNARY_OPERATOR_MAP[ast.value];
-          const operand = compileAST(ast.operand, false, false, {
-            functionName: fnName,
-          }).assignResultToVariable();
+          const operand = compileAST(ast.operand, false, false).assignResultToVariable();
           code.append(operand);
           return code.return(`ctx['${fnName}'](${operand.returnExpression})`);
         }
         case "BIN_OPERATION": {
           const fnName = OPERATOR_MAP[ast.value];
-          const left = compileAST(ast.left, false, false, {
-            functionName: fnName,
-          }).assignResultToVariable();
-          const right = compileAST(ast.right, false, false, {
-            functionName: fnName,
-          }).assignResultToVariable();
+          const left = compileAST(ast.left, false, false).assignResultToVariable();
+          const right = compileAST(ast.right, false, false).assignResultToVariable();
           code.append(left);
           code.append(right);
           return code.return(
@@ -259,7 +236,10 @@ function compilationCacheKey(
           return `|N${constantValues.numbers.indexOf(parseNumber(token.value, DEFAULT_LOCALE))}|`;
         case "REFERENCE":
         case "INVALID_REFERENCE":
-          return `|${dependencies.indexOf(token.value)}|`;
+          if (token.value.includes(":")) {
+            return `R|${dependencies.indexOf(token.value)}|`;
+          }
+          return `C|${dependencies.indexOf(token.value)}|`;
         case "SPACE":
           return "";
         default:

src/plugins/ui_core_views/cell_evaluation/compilation_parameters.ts

@@ -69,34 +69,14 @@ class CompilationParametersBuilder {
    *        function for which this parameter is used, we just return the string of the parameter.
    *        The `compute` of the formula's function must process it completely
    */
-  private refFn(
-    range: Range,
-    isMeta: boolean,
-    functionName: string,
-    paramNumber?: number
-  ): FPayload {
+  private refFn(range: Range, isMeta: boolean): FPayload {
     this.assertRangeValid(range);
     if (isMeta) {
       // Use zoneToXc of zone instead of getRangeString to avoid sending unbounded ranges
       const sheetName = this.getters.getSheetName(range.sheetId);
       return { value: getFullReference(sheetName, zoneToXc(range.zone)) };
     }
-
-    // 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 EvaluationError(
-        paramNumber
-          ? _t(
-              "Function %s expects the parameter %s to be a single value or a single cell reference, not a range.",
-              functionName.toString(),
-              paramNumber.toString()
-            )
-          : _t(
-              "Function %s expects its parameters to be single values or single cell references, not ranges.",
-              functionName.toString()
-            )
-      );
-    }
+    // the compiler guarantees only single cell ranges reach this part of the code
     const position = { sheetId: range.sheetId, col: range.zone.left, row: range.zone.top };
     return this.computeCell(position);
   }

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

@@ -1,27 +1,45 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`compile functions check type of arguments simple argument value from a single cell or range reference 1`] = `
+"function anonymous(deps,ref,range,ctx
+) {
+// =SIMPLE_VALUE_EXPECTED(C|0|)
+const _1 = ref(deps[0], false);
+return ctx['SIMPLE_VALUE_EXPECTED'](_1);
+}"
+`;
+
+exports[`compile functions check type of arguments simple argument value from a single cell or range reference 2`] = `
+"function anonymous(deps,ref,range,ctx
+) {
+// =SIMPLE_VALUE_EXPECTED(R|0|)
+const _1 = range(deps[0]);
+return ctx['SIMPLE_VALUE_EXPECTED'](_1);
+}"
+`;
+
 exports[`compile functions with meta arguments function call requesting meta parameter 1`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =USEMETAARG(|0|)
-const _1 = ref(deps[0], true, "USEMETAARG",  1);
+// =USEMETAARG(C|0|)
+const _1 = ref(deps[0], true);
 return ctx['USEMETAARG'](_1);
 }"
 `;
 
 exports[`compile functions with meta arguments function call requesting meta parameter 2`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =USEMETAARG(|0|)
-const _1 = ref(deps[0], true, "USEMETAARG",  1);
+// =USEMETAARG(C|0|)
+const _1 = ref(deps[0], true);
 return ctx['USEMETAARG'](_1);
 }"
 `;
 
 exports[`expression compiler cells are converted to ranges if function require a range 1`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =sum(|0|)
+// =sum(C|0|)
 const _1 = range(deps[0]);
 return ctx['SUM'](_1);
 }"
@@ -30,29 +48,29 @@ return ctx['SUM'](_1);
 exports[`expression compiler expression with $ref 1`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =|0|+|1|+|2|
-const _1 = ref(deps[0], false, "ADD",  undefined);
-const _2 = ref(deps[1], false, "ADD",  undefined);
+// =C|0|+C|1|+C|2|
+const _1 = ref(deps[0], false);
+const _2 = ref(deps[1], false);
 const _3 = ctx['ADD'](_1, _2);
-const _4 = ref(deps[2], false, "ADD",  undefined);
+const _4 = ref(deps[2], false);
 return ctx['ADD'](_3, _4);
 }"
 `;
 
 exports[`expression compiler expression with references with a sheet 1`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =|0|
-return ref(deps[0], false, "EQ",  undefined);
+// =C|0|
+return ref(deps[0], false);
 }"
 `;
 
 exports[`expression compiler expressions with a debugger 1`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =?|0|/|N0|
+// =?C|0|/|N0|
 debugger;
-const _1 = ref(deps[0], false, "DIVIDE",  undefined);
+const _1 = ref(deps[0], false);
 const _2 = { value: this.constantValues.numbers[0] };
 return ctx['DIVIDE'](_1, _2);
 }"
@@ -61,8 +79,8 @@ return ctx['DIVIDE'](_1, _2);
 exports[`expression compiler read some values and functions 1`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =|0|+sum(|1|)
-const _1 = ref(deps[0], false, "ADD",  undefined);
+// =C|0|+sum(R|1|)
+const _1 = ref(deps[0], false);
 const _2 = range(deps[1]);
 const _3 = ctx['SUM'](_2);
 return ctx['ADD'](_1, _3);
@@ -211,16 +229,16 @@ return ctx['UNARY.PERCENT'](_3);
 exports[`expression compiler some arithmetic expressions 15`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =|0|%
-const _1 = ref(deps[0], false, "UNARY.PERCENT",  undefined);
+// =C|0|%
+const _1 = ref(deps[0], false);
 return ctx['UNARY.PERCENT'](_1);
 }"
 `;
 
 exports[`expression compiler with the same reference multiple times 1`] = `
 "function anonymous(deps,ref,range,ctx
 ) {
-// =SUM(|0|,|0|,|2|)
+// =SUM(C|0|,C|0|,C|2|)
 const _1 = range(deps[0]);
 const _2 = range(deps[0]);
 const _3 = range(deps[2]);

tests/evaluation/compiler.test.ts

@@ -289,6 +289,24 @@ describe("compile functions", () => {
       expect(() => compiledBaseFunction("=RANGEEXPECTED(FORMULA_RETURNING_RANGE())")).not.toThrow();
     });
 
+    test("simple argument value from a single cell or range reference", () => {
+      functionRegistry.add("SIMPLE_VALUE_EXPECTED", {
+        description: "does not accept a range",
+        compute: (arg) => {
+          return true;
+        },
+        args: [{ name: "arg1", description: "", type: ["NUMBER"] }],
+        returns: ["ANY"],
+      });
+
+      expect(
+        compiledBaseFunction("=SIMPLE_VALUE_EXPECTED(A1)").execute.toString()
+      ).toMatchSnapshot();
+      expect(
+        compiledBaseFunction("=SIMPLE_VALUE_EXPECTED(A1:A2)").execute.toString()
+      ).toMatchSnapshot();
+    });
+
     test("reject range when expecting only non-range argument", () => {
       for (let typeExpected of ["ANY", "BOOLEAN", "DATE", "NUMBER", "STRING"] as ArgType[]) {
         functionRegistry.add(typeExpected + "EXPECTED", {
@@ -319,43 +337,40 @@ describe("compile functions", () => {
       setCellContent(m, "B14", "=ANYEXPECTED(A1:A1)");
 
       expect(getCellError(m, "B1")).toBe(
-        "Function ANYEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
+        "Function ANYEXPECTED expects the parameter 'arg1' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B2")).toBe(
-        "Function BOOLEANEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
+        "Function BOOLEANEXPECTED expects the parameter 'arg1' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B3")).toBe(
-        "Function DATEEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
+        "Function DATEEXPECTED expects the parameter 'arg1' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B4")).toBe(
-        "Function NUMBEREXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
+        "Function NUMBEREXPECTED expects the parameter 'arg1' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B5")).toBe(
-        "Function STRINGEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
+        "Function STRINGEXPECTED expects the parameter 'arg1' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B6")).toBe(
-        "Function ANYEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
+        "Function ANYEXPECTED expects the parameter 'arg1' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B7")).toBe(
-        "Function ANYEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
-      );
-      expect(getCellError(m, "B8")).toBe(
-        "Function EQ expects its parameters to be single values or single cell references, not ranges."
+        "Function ANYEXPECTED expects the parameter 'arg1' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B9")).toBe(
-        "Function UPLUS expects its parameters to be single values or single cell references, not ranges."
+        "Function UPLUS expects the parameter 'value' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B10")).toBe(
-        "Function ADD expects its parameters to be single values or single cell references, not ranges."
+        "Function ADD expects the parameter 'value2' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B11")).toBe(
-        "Function UMINUS expects its parameters to be single values or single cell references, not ranges."
+        "Function UMINUS expects the parameter 'value' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B12")).toBe(
-        "Function MINUS expects its parameters to be single values or single cell references, not ranges."
+        "Function MINUS expects the parameter 'value2' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B13")).toBe(
-        "Function MULTIPLY expects its parameters to be single values or single cell references, not ranges."
+        "Function MULTIPLY expects the parameter 'factor2' to be a single value or a single cell reference, not a range."
       );
       expect(getCellError(m, "B14")).toBeUndefined();
     });
@@ -419,34 +434,34 @@ describe("compile functions", () => {
       const rangeA1ToB2 = createRange(m.getters, "ABC", "A1:B2")!;
 
       compiledFormula1.execute([rangeA1], refFn, ensureRange, ctx);
-      expect(refFn).toHaveBeenCalledWith(rangeA1, true, "USEMETAARG", 1);
+      expect(refFn).toHaveBeenCalledWith(rangeA1, true);
       expect(ensureRange).toHaveBeenCalledTimes(0);
       refFn.mockReset();
 
       compiledFormula2.execute([rangeA1ToB2], refFn, ensureRange, ctx);
-      expect(refFn).toHaveBeenCalledWith(rangeA1ToB2, true, "USEMETAARG", 1);
+      expect(refFn).toHaveBeenCalledWith(rangeA1ToB2, true);
       expect(ensureRange).toHaveBeenCalledTimes(0);
       refFn.mockReset();
 
       compiledFormula3.execute([rangeA1], refFn, ensureRange, ctx);
-      expect(refFn).toHaveBeenCalledWith(rangeA1, false, "NOTUSEMETAARG", 1);
+      expect(refFn).toHaveBeenCalledWith(rangeA1, false);
       expect(ensureRange).toHaveBeenCalledTimes(0);
       refFn.mockReset();
 
       compiledFormula4.execute([rangeA1ToB2], refFn, ensureRange, ctx);
-      expect(refFn).toHaveBeenCalledWith(rangeA1ToB2, false, "NOTUSEMETAARG", 1);
-      expect(ensureRange).toHaveBeenCalledTimes(0);
+      expect(refFn).toHaveBeenCalledTimes(0);
+      expect(ensureRange).toHaveBeenCalledWith(rangeA1ToB2);
       refFn.mockReset();
     });
   });
 
   test("function cache ignore spaces in functions", () => {
     compiledBaseFunction("=SUM(A1)");
-    expect(Object.keys(functionCache)).toEqual(["=SUM(|0|)"]);
+    expect(Object.keys(functionCache)).toEqual(["=SUM(C|0|)"]);
     compile("= SUM(A1)");
     compile("=SUM( A1)");
     compile("= SUM(A1 )");
     compile("= SUM   (    A1    )");
-    expect(Object.keys(functionCache)).toEqual(["=SUM(|0|)"]);
+    expect(Object.keys(functionCache)).toEqual(["=SUM(C|0|)"]);
   });
 });

tests/evaluation/evaluation.test.ts

@@ -327,12 +327,6 @@ describe("evaluateCells", () => {
     expect(getEvaluatedCell(model, "A1").value).toBe(0);
   });
 
-  test("=Range", () => {
-    const model = new Model();
-    setCellContent(model, "A1", "=A2:A3");
-    expect(getEvaluatedCell(model, "A1").value).toBe("#ERROR");
-  });
-
   test("misc math formulas", () => {
     const model = new Model();
     setCellContent(model, "A1", "42");
@@ -1068,6 +1062,14 @@ describe("evaluateCells", () => {
     expect(getEvaluatedCell(model, "A3").value).toBe("new");
   });
 
+  test("wrong range input can become valid", () => {
+    const model = new Model();
+    setCellContent(model, "A1", "=ADD(B1:C1,1)");
+    expect(getEvaluatedCell(model, "A1").value).toBe("#ERROR");
+    deleteColumns(model, ["C"]);
+    expect(getEvaluatedCell(model, "A1").value).toBe(1);
+  });
+
   test("Coherent handling of #REF when it occurs following a column deletion or a copy/paste", () => {
     const model = new Model();
     setCellContent(model, "A1", "=SUM(B1,C1)");