Fix Incorrect array size error when using column range reference to a…

…n empty sheet (#1346) * Don't throw 'Incorrect array size' for all empty arrays * Add unit tests for function MATCH with empty sheet argument * Throw 'Incorrect array size' error from ArrayValue constructor and Operations.setCellContent to preserve the current behavior in certain situations * Add changelog entry * Add similar test for function COUNTIF * Add similar test for function SUM * Add similar test for function VLOOKUP * Improve test descriptions * Remove unused method * Add a unit test to cover the uncovered line in ArraySize.ts
handsontable · Dec 19, 2023 · 896d501 · 896d501
1 parent afbdfe5
commit 896d501
Show file tree

Hide file tree

Showing 9 changed files with 129 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fixed an issue where `simpleCellAddressFromString` method was crashing when called with a non-ASCII character in an unquoted sheet name. [#1312](https://github.com/handsontable/hyperformula/issues/1312)
 - Fixed a bug where adding a row to the very large spreadsheet resulted in `Maximum call stack size exceeded` error. [#1332](https://github.com/handsontable/hyperformula/issues/1332)
 - Fixed a typo in the JSDoc comment of the `HyperFormula` class. [#1323](https://github.com/handsontable/hyperformula/issues/1323)
+- Fixed the `Incorrect array size` error when using column range reference to an empty sheet as a function argument. [#1147](https://github.com/handsontable/hyperformula/issues/1147)
 - Fixed a bug where function SUBSTITUTE did not work correctly with regexp special characters. [#1289](https://github.com/handsontable/hyperformula/issues/1289)
 
 ## [2.6.0] - 2023-09-19

diff --git a/src/ArraySize.ts b/src/ArraySize.ts
@@ -8,23 +8,15 @@ import {SimpleCellAddress} from './Cell'
 import {Config} from './Config'
 import {FunctionRegistry} from './interpreter/FunctionRegistry'
 import {InterpreterState} from './interpreter/InterpreterState'
-import {FunctionArgumentType} from './interpreter/plugin/FunctionPlugin'
+import {FunctionArgumentType} from './interpreter'
 import {Ast, AstNodeType, ProcedureAst} from './parser'
 
 export class ArraySize {
   constructor(
     public width: number,
     public height: number,
     public isRef: boolean = false,
-  ) {
-    if (width <= 0 || height <= 0) {
-      throw Error('Incorrect array size')
-    }
-  }
-
-  public static fromArray<T>(array: T[][]): ArraySize {
-    return new ArraySize(array.length > 0 ? array[0].length : 0, array.length)
-  }
+  ) {}
 
   public static error() {
     return new ArraySize(1, 1, true)
@@ -35,7 +27,7 @@ export class ArraySize {
   }
 
   isScalar(): boolean {
-    return (this.width <= 1 && this.height <= 1) || this.isRef
+    return (this.width === 1 && this.height === 1) || this.isRef
   }
 }
 
@@ -131,37 +123,53 @@ export class ArraySizePredictor {
   }
 
   private checkArraySizeForFunction(ast: ProcedureAst, state: InterpreterState): ArraySize {
-    const metadata = this.functionRegistry.getMetadata(ast.procedureName)
     const pluginArraySizeFunction = this.functionRegistry.getArraySizeFunction(ast.procedureName)
+
     if (pluginArraySizeFunction !== undefined) {
       return pluginArraySizeFunction(ast, state)
     }
-    const subChecks = ast.args.map((arg) => this.checkArraySizeForAst(arg, new InterpreterState(state.formulaAddress, state.arraysFlag || (metadata?.arrayFunction ?? false))))
-    if (metadata === undefined || metadata.expandRanges || !state.arraysFlag || metadata.vectorizationForbidden || metadata.parameters === undefined) {
+
+    const metadata = this.functionRegistry.getMetadata(ast.procedureName)
+
+    if (
+      metadata === undefined
+      || metadata.expandRanges
+      || !state.arraysFlag
+      || metadata.vectorizationForbidden
+      || metadata.parameters === undefined
+    ) {
       return new ArraySize(1, 1)
     }
+
+    const subChecks = ast.args.map((arg) => this.checkArraySizeForAst(arg, new InterpreterState(state.formulaAddress, state.arraysFlag || (metadata?.arrayFunction ?? false))))
     const argumentDefinitions = [...metadata.parameters]
-    if (metadata.repeatLastArgs === undefined && argumentDefinitions.length < subChecks.length) {
-      return ArraySize.error()
-    }
-    if (metadata.repeatLastArgs !== undefined && argumentDefinitions.length < subChecks.length &&
-      (subChecks.length - argumentDefinitions.length) % metadata.repeatLastArgs !== 0) {
+
+    if (
+      metadata.repeatLastArgs !== undefined
+      && argumentDefinitions.length < subChecks.length
+      && (subChecks.length - argumentDefinitions.length) % metadata.repeatLastArgs !== 0
+    ) {
       return ArraySize.error()
     }
 
     while (argumentDefinitions.length < subChecks.length) {
-      argumentDefinitions.push(...argumentDefinitions.slice(argumentDefinitions.length - metadata.repeatLastArgs!))
+      if (metadata.repeatLastArgs === undefined) {
+        return ArraySize.error()
+      }
+
+      argumentDefinitions.push(...argumentDefinitions.slice(argumentDefinitions.length - metadata.repeatLastArgs))
     }
 
     let maxWidth = 1
     let maxHeight = 1
+
     for (let i = 0; i < subChecks.length; i++) {
       if (argumentDefinitions[i].argumentType !== FunctionArgumentType.RANGE && argumentDefinitions[i].argumentType !== FunctionArgumentType.ANY) {
         maxHeight = Math.max(maxHeight, subChecks[i].height)
         maxWidth = Math.max(maxWidth, subChecks[i].width)
       }
     }
+
     return new ArraySize(maxWidth, maxHeight)
   }
 }
-
diff --git a/src/ArrayValue.ts b/src/ArrayValue.ts
@@ -49,6 +49,10 @@ export class ArrayValue implements CellArray {
   constructor(array: InternalScalarValue[][]) {
     this.size = new ArraySize(array.length > 0 ? array[0].length : 0, array.length)
     this.array = array
+
+    if (this.size.width <= 0 || this.size.height <= 0) {
+      throw Error('Incorrect array size')
+    }
   }
 
   static fromInterpreterValue(value: InterpreterValue) {

diff --git a/src/Operations.ts b/src/Operations.ts
@@ -563,6 +563,11 @@ export class Operations {
       } else {
         try {
           const size = this.arraySizePredictor.checkArraySize(ast, address)
+
+          if (size.width <= 0 || size.height <= 0) {
+            throw Error('Incorrect array size')
+          }
+
           this.setFormulaToCell(address, size, parserResult)
         } catch (error) {
 

diff --git a/test/interpreter/function-countif.spec.ts b/test/interpreter/function-countif.spec.ts
@@ -135,4 +135,13 @@ describe('Function COUNTIF', () => {
 
     expect(engine.getCellValue(adr('A4'))).toEqual(2)
   })
+
+  it('works with a column range reference to an empty sheet', () => {
+    const hf = HyperFormula.buildFromSheets({
+      table1: [],
+      table2: [['=COUNTIF(table1!A:C, ">1")']],
+    })
+
+    expect(hf.getCellValue(adr('A1', 1))).toEqual(0)
+  })
 })
diff --git a/test/interpreter/function-ifs.spec.ts b/test/interpreter/function-ifs.spec.ts
@@ -15,6 +15,18 @@ describe('Function IFS', () => {
     expect(engine.getCellValue(adr('B3'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.WrongArgNumber))
   })
 
+  it('Should not work for wrong number of arguments when array arithmetic is on', () => {
+    const engine = HyperFormula.buildFromArray([
+      [10, '=IFS()'],
+      [20, '=IFS(A1>90)'],
+      [30, '=IFS(A1>90, "A", A1>80)'],
+    ], { useArrayArithmetic: true })
+
+    expect(engine.getCellValue(adr('B1'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.WrongArgNumber))
+    expect(engine.getCellValue(adr('B2'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.WrongArgNumber))
+    expect(engine.getCellValue(adr('B3'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.WrongArgNumber))
+  })
+
   it('Nominal operation', () => {
     const engine = HyperFormula.buildFromArray([
       [11, '=IFS(A1>30, "A", A1>20, "B", A1>10, "C")'],

diff --git a/test/interpreter/function-match.spec.ts b/test/interpreter/function-match.spec.ts
@@ -14,6 +14,16 @@ describe('Function MATCH', () => {
     expect(engine.getCellValue(adr('A2'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.WrongArgNumber))
   })
 
+  it('validates number of arguments when array arithmetics is on', () => {
+    const engine = HyperFormula.buildFromArray([
+      ['=MATCH(1)'],
+      ['=MATCH(1, B1:B3, 0, 42)'],
+    ], { useArrayArithmetic: true })
+
+    expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.WrongArgNumber))
+    expect(engine.getCellValue(adr('A2'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.WrongArgNumber))
+  })
+
   it('validates that 1st argument is number, string or boolean', () => {
     const engine = HyperFormula.buildFromArray([
       ['=MATCH(C2:C3, B1:B1)'],
@@ -704,4 +714,45 @@ describe('Function MATCH', () => {
 
     expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.NA))
   })
+
+  describe('works with a range reference to an empty sheet', () => {
+    it('- cell range', () => {
+      const hf = HyperFormula.buildFromSheets({
+        table1: [],
+        table2: [['=MATCH(0, table1!A1:A10)']],
+      })
+
+      expect(hf.getCellValue(adr('A1', 1))).toEqualError(detailedError(ErrorType.NA))
+    })
+
+    it('- column range', () => {
+      const hf = HyperFormula.buildFromSheets({
+        table1: [[]],
+        table2: [['=MATCH(0, table1!A:A)']],
+      })
+
+      expect(hf.getCellValue(adr('A1', 1))).toEqualError(detailedError(ErrorType.NA))
+    })
+
+    it('- row range', () => {
+      const hf = HyperFormula.buildFromSheets({
+        table1: [],
+        table2: [['=MATCH(0, table1!1:1)']],
+      })
+
+      expect(hf.getCellValue(adr('A1', 1))).toEqualError(detailedError(ErrorType.NA))
+    })
+
+    it('- column range (called from calculateFormula)', () => {
+      const hf = HyperFormula.buildFromArray([])
+
+      expect(hf.calculateFormula('=MATCH(0, Sheet1!A:A)', 0)).toEqualError(detailedError(ErrorType.NA))
+    })
+
+    it('- column range (called from calculateFormula without explicit sheet reference)', () => {
+      const hf = HyperFormula.buildFromArray([])
+
+      expect(hf.calculateFormula('=MATCH(0, A:A)', 0)).toEqualError(detailedError(ErrorType.NA))
+    })
+  })
 })
diff --git a/test/interpreter/function-sum.spec.ts b/test/interpreter/function-sum.spec.ts
@@ -120,6 +120,15 @@ describe('SUM', () => {
     expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.DIV_BY_ZERO))
   })
 
+  it('works with a column range reference to an empty sheet', () => {
+    const hf = HyperFormula.buildFromSheets({
+      table1: [],
+      table2: [['=SUM(table1!A:C)']],
+    })
+
+    expect(hf.getCellValue(adr('A1', 1))).toEqual(0)
+  })
+
   describe('works with reversed cell ranges', () => {
     it('simple case', () => {
       const engine = HyperFormula.buildFromArray([

diff --git a/test/interpreter/function-vlookup.spec.ts b/test/interpreter/function-vlookup.spec.ts
@@ -840,4 +840,13 @@ describe('BinarySearchStrategy', () => {
 
     expect(engine.getCellValue(adr('A1'))).toEqual('1')
   })
+
+  it('works with a column range reference to an empty sheet', () => {
+    const hf = HyperFormula.buildFromSheets({
+      table1: [],
+      table2: [['=VLOOKUP("42", table1!A:C, 1)']],
+    })
+
+    expect(hf.getCellValue(adr('A1', 1))).toEqualError(detailedError(ErrorType.NA))
+  })
 })