fix: #2880 not possible to map cube root cbrt (#2966)

* fix: #2880 not possible to map cube root `cbrt` * fix: bind signature, add comments
josdejong · Jun 8, 2023 · 3c59d1a · 3c59d1a
1 parent 1798dda
commit 3c59d1a
Show file tree

Hide file tree

Showing 9 changed files with 109 additions and 105 deletions.
diff --git a/HISTORY.md b/HISTORY.md
@@ -2,6 +2,7 @@
 
 # unpublished changes since 11.8.0
 
+- Fix #2880: not possible to map cube root `cbrt`.
 - Fix #2938: make the syntax description of all functions consistent in the
   docs (#2941). Thanks @dvd101x.
 - Fix #2954: improve the TypeScript definitions the return type of functions

diff --git a/src/expression/transform/filter.transform.js b/src/expression/transform/filter.transform.js
@@ -1,8 +1,8 @@
-import { isFunctionAssignmentNode, isSymbolNode } from '../../utils/is.js'
+import { applyCallback } from '../../utils/applyCallback.js'
 import { filter, filterRegExp } from '../../utils/array.js'
-import { maxArgumentCount } from '../../utils/function.js'
-import { compileInlineExpression } from './utils/compileInlineExpression.js'
 import { factory } from '../../utils/factory.js'
+import { isFunctionAssignmentNode, isSymbolNode } from '../../utils/is.js'
+import { compileInlineExpression } from './utils/compileInlineExpression.js'
 
 const name = 'filter'
 const dependencies = ['typed']
@@ -65,17 +65,8 @@ export const createFilterTransform = /* #__PURE__ */ factory(name, dependencies,
  * @private
  */
 function _filter (x, callback) {
-  // figure out what number of arguments the callback function expects
-  const args = maxArgumentCount(callback)
-
   return filter(x, function (value, index, array) {
     // invoke the callback function with the right number of arguments
-    if (args === 1) {
-      return callback(value)
-    } else if (args === 2) {
-      return callback(value, [index + 1])
-    } else { // 3 or -1
-      return callback(value, [index + 1], array)
-    }
+    return applyCallback(callback, value, [index + 1], array, 'filter')
   })
 }
diff --git a/src/expression/transform/forEach.transform.js b/src/expression/transform/forEach.transform.js
@@ -1,7 +1,7 @@
-import { isFunctionAssignmentNode, isSymbolNode } from '../../utils/is.js'
-import { maxArgumentCount } from '../../utils/function.js'
+import { applyCallback } from '../../utils/applyCallback.js'
 import { forEach } from '../../utils/array.js'
 import { factory } from '../../utils/factory.js'
+import { isFunctionAssignmentNode, isSymbolNode } from '../../utils/is.js'
 import { compileInlineExpression } from './utils/compileInlineExpression.js'
 
 const name = 'forEach'
@@ -38,9 +38,6 @@ export const createForEachTransform = /* #__PURE__ */ factory(name, dependencies
   // one-based version of forEach
   const _forEach = typed('forEach', {
     'Array | Matrix, function': function (array, callback) {
-      // figure out what number of arguments the callback function expects
-      const args = maxArgumentCount(callback)
-
       const recurse = function (value, index) {
         if (Array.isArray(value)) {
           forEach(value, function (child, i) {
@@ -49,13 +46,7 @@ export const createForEachTransform = /* #__PURE__ */ factory(name, dependencies
           })
         } else {
           // invoke the callback function with the right number of arguments
-          if (args === 1) {
-            callback(value)
-          } else if (args === 2) {
-            callback(value, index)
-          } else { // 3 or -1
-            callback(value, index, array)
-          }
+          return applyCallback(callback, value, index, array, 'forEach')
         }
       }
       recurse(array.valueOf(), []) // pass Array

diff --git a/src/expression/transform/map.transform.js b/src/expression/transform/map.transform.js
@@ -1,7 +1,7 @@
-import { isFunctionAssignmentNode, isSymbolNode } from '../../utils/is.js'
-import { maxArgumentCount } from '../../utils/function.js'
+import { applyCallback } from '../../utils/applyCallback.js'
 import { map } from '../../utils/array.js'
 import { factory } from '../../utils/factory.js'
+import { isFunctionAssignmentNode, isSymbolNode } from '../../utils/is.js'
 import { compileInlineExpression } from './utils/compileInlineExpression.js'
 
 const name = 'map'
@@ -50,17 +50,14 @@ export const createMapTransform = /* #__PURE__ */ factory(name, dependencies, ({
 }, { isTransformFunction: true })
 
 /**
- * Map for a multi dimensional array. One-based indexes
+ * Map for a multidimensional array. One-based indexes
  * @param {Array} array
  * @param {function} callback
  * @param {Array} orig
  * @return {Array}
  * @private
  */
 function _map (array, callback, orig) {
-  // figure out what number of arguments the callback function expects
-  const argsCount = maxArgumentCount(callback)
-
   function recurse (value, index) {
     if (Array.isArray(value)) {
       return map(value, function (child, i) {
@@ -69,13 +66,7 @@ function _map (array, callback, orig) {
       })
     } else {
       // invoke the (typed) callback function with the right number of arguments
-      if (argsCount === 1) {
-        return callback(value)
-      } else if (argsCount === 2) {
-        return callback(value, index)
-      } else { // 3 or -1
-        return callback(value, index, orig)
-      }
+      return applyCallback(callback, value, index, orig, 'map')
     }
   }
 

diff --git a/src/function/matrix/filter.js b/src/function/matrix/filter.js
@@ -1,5 +1,5 @@
+import { applyCallback } from '../../utils/applyCallback.js'
 import { filter, filterRegExp } from '../../utils/array.js'
-import { maxArgumentCount } from '../../utils/function.js'
 import { factory } from '../../utils/factory.js'
 
 const name = 'filter'
@@ -58,17 +58,8 @@ export const createFilter = /* #__PURE__ */ factory(name, dependencies, ({ typed
  * @private
  */
 function _filterCallback (x, callback) {
-  // figure out what number of arguments the callback function expects
-  const args = maxArgumentCount(callback)
-
   return filter(x, function (value, index, array) {
     // invoke the callback function with the right number of arguments
-    if (args === 1) {
-      return callback(value)
-    } else if (args === 2) {
-      return callback(value, [index])
-    } else { // 3 or -1
-      return callback(value, [index], array)
-    }
+    return applyCallback(callback, value, [index], array, 'filter')
   })
 }
diff --git a/src/function/matrix/forEach.js b/src/function/matrix/forEach.js
@@ -1,4 +1,4 @@
-import { maxArgumentCount } from '../../utils/function.js'
+import { applyCallback } from '../../utils/applyCallback.js'
 import { forEach as forEachArray } from '../../utils/array.js'
 import { factory } from '../../utils/factory.js'
 
@@ -39,15 +39,12 @@ export const createForEach = /* #__PURE__ */ factory(name, dependencies, ({ type
 })
 
 /**
- * forEach for a multi dimensional array
+ * forEach for a multidimensional array
  * @param {Array} array
  * @param {Function} callback
  * @private
  */
 function _forEach (array, callback) {
-  // figure out what number of arguments the callback function expects
-  const args = maxArgumentCount(callback)
-
   const recurse = function (value, index) {
     if (Array.isArray(value)) {
       forEachArray(value, function (child, i) {
@@ -56,13 +53,7 @@ function _forEach (array, callback) {
       })
     } else {
       // invoke the callback function with the right number of arguments
-      if (args === 1) {
-        callback(value)
-      } else if (args === 2) {
-        callback(value, index)
-      } else { // 3 or -1
-        callback(value, index, array)
-      }
+      return applyCallback(callback, value, index, array, 'forEach')
     }
   }
   recurse(array, [])

diff --git a/src/function/matrix/map.js b/src/function/matrix/map.js
@@ -1,4 +1,4 @@
-import { maxArgumentCount } from '../../utils/function.js'
+import { applyCallback } from '../../utils/applyCallback.js'
 import { factory } from '../../utils/factory.js'
 
 const name = 'map'
@@ -26,14 +26,9 @@ export const createMap = /* #__PURE__ */ factory(name, dependencies, ({ typed })
    *      return value * value
    *    })  // returns [1, 4, 9]
    *
-   *    // The calling convention for the callback can cause subtleties:
-   *    math.map([1, 2, 3], math.format)
-   *    // throws TypeError: map attempted to call 'format(1,[0])' but argument 2 of type Array does not match expected type number or function or Object or string or boolean
-   *    // [This happens because `format` _can_ take a second argument,
-   *    // but its semantics don't match that of the 2nd argument `map` provides]
-   *
-   *    // To avoid this error, use a function that takes exactly the
-   *    // desired arguments:
+   *    // The callback is normally called with three arguments:
+   *    //    callback(value, index, Array)
+   *    // If you want to call with only one argument, use:
    *    math.map([1, 2, 3], x => math.format(x)) // returns ['1', '2', '3']
    *
    * See also:
@@ -63,44 +58,15 @@ export const createMap = /* #__PURE__ */ factory(name, dependencies, ({ typed })
  * @private
  */
 function _map (array, callback) {
-  // figure out what number of arguments the callback function expects
-  const args = maxArgumentCount(callback)
-
   const recurse = function (value, index) {
     if (Array.isArray(value)) {
       return value.map(function (child, i) {
         // we create a copy of the index array and append the new index value
         return recurse(child, index.concat(i))
       })
     } else {
-      try {
-        // invoke the callback function with the right number of arguments
-        if (args === 1) {
-          return callback(value)
-        } else if (args === 2) {
-          return callback(value, index)
-        } else { // 3 or -1
-          return callback(value, index, array)
-        }
-      } catch (err) {
-        // But maybe the arguments still weren't right
-        if (err instanceof TypeError &&
-            'data' in err &&
-            err.data.category === 'wrongType') {
-          let newmsg = `map attempted to call '${err.data.fn}(${value}`
-          const indexString = JSON.stringify(index)
-          if (args === 2) {
-            newmsg += ',' + indexString
-          } else if (args !== 1) {
-            newmsg += `,${indexString},${array}`
-          }
-          newmsg += `)' but argument ${err.data.index + 1} of type `
-          newmsg += `${err.data.actual} does not match expected type `
-          newmsg += err.data.expected.join(' or ')
-          throw new TypeError(newmsg)
-        }
-        throw err
-      }
+      // invoke the callback function with the right number of arguments
+      return applyCallback(callback, value, index, array, 'map')
     }
   }
 

diff --git a/src/utils/applyCallback.js b/src/utils/applyCallback.js
@@ -0,0 +1,67 @@
+import typed from 'typed-function'
+import { typeOf as _typeOf } from './is.js'
+
+/**
+ * Invoke a callback for functions like map and filter with a matching number of arguments
+ * @param {function} callback
+ * @param {any} value
+ * @param {number | number[]} index
+ * @param {Array} array
+ * @param {string} mappingFnName   The name of the function that is invoking these callbacks, for example "map" or "filter"
+ * @returns {*}
+ */
+export function applyCallback (callback, value, index, array, mappingFnName) {
+  if (typed.isTypedFunction(callback)) {
+    // invoke the typed callback function with the matching number of arguments only
+
+    const args3 = [value, index, array]
+    const signature3 = typed.resolve(callback, args3)
+    if (signature3) {
+      return tryWithArgs(signature3.implementation, args3)
+    }
+
+    const args2 = [value, index]
+    const signature2 = typed.resolve(callback, args2)
+    if (signature2) {
+      return tryWithArgs(signature2.implementation, args2)
+    }
+
+    const args1 = [value]
+    const signature1 = typed.resolve(callback, args1)
+    if (signature1) {
+      return tryWithArgs(signature1.implementation, args1)
+    }
+
+    // fallback (will throw an exception)
+    return tryWithArgs(callback, args3)
+  } else {
+    // A regular JavaScript function
+    return callback(value, index, array)
+  }
+
+  /**
+   * @param {function} signature The selected signature of the typed-function
+   * @param {Array} args List with arguments to apply to the selected signature
+   * @returns {*} Returns the return value of the invoked signature
+   * @throws {TypeError} Throws an error when no matching signature was found
+   */
+  function tryWithArgs (signature, args) {
+    try {
+      return signature.apply(signature, args)
+    } catch (err) {
+      // Enrich the error message so the user understands that it took place inside the callback function
+      if (err instanceof TypeError && err.data?.category === 'wrongType') {
+        const argsDesc = []
+        argsDesc.push(`value: ${_typeOf(value)}`)
+        if (args.length >= 2) { argsDesc.push(`index: ${_typeOf(index)}`) }
+        if (args.length >= 3) { argsDesc.push(`array: ${_typeOf(array)}`) }
+
+        throw new TypeError(`Function ${mappingFnName} cannot apply callback arguments ` +
+          `${callback.name}(${argsDesc.join(', ')}) at index ${JSON.stringify(index)}`)
+      } else {
+        throw new TypeError(`Function ${mappingFnName} cannot apply callback arguments ` +
+          `to function ${callback.name}: ${err.message}`)
+      }
+    }
+  }
+}
diff --git a/test/unit-tests/function/matrix/map.test.js b/test/unit-tests/function/matrix/map.test.js
@@ -63,6 +63,16 @@ describe('map', function () {
     assert.deepStrictEqual(output, [3, 4, 5])
   })
 
+  it('should invoke a typed function with correct number of arguments (4)', function () {
+    // cbrt has a syntax cbrt(x, allRoots), but it should invoke cbrt(x) here
+    assert.deepStrictEqual(math.map([1, 8, 27], math.cbrt), [1, 2, 3])
+  })
+
+  it('should invoke a typed function with correct number of arguments (5)', function () {
+    // cbrt has a syntax cbrt(x, allRoots), but it should invoke cbrt(x) here
+    assert.deepStrictEqual(math.map([1, 8, 27], math.format), ['1', '8', '27'])
+  })
+
   it('should throw an error if called with unsupported type', function () {
     assert.throws(function () { math.map(1, function () {}) })
     assert.throws(function () { math.map('arr', function () {}) })
@@ -72,10 +82,15 @@ describe('map', function () {
     assert.throws(function () { math.map([1, 2, 3]) })
   })
 
-  it('should throw an error if the callback argument types are incorrect',
-    function () {
-      assert.throws(() => math.map([1, 2, 3], math.format), TypeError)
-    })
+  it('should throw an error if the callback argument types are incorrect (1)', function () {
+    assert.throws(() => math.map([1, 2, 3], math.equalText),
+      /Function map cannot apply callback arguments to function equalText: Unexpected type of argument in function compareText \(expected: string or Array or Matrix, actual: number, index: 0\)/)
+  })
+
+  it('should throw an error if the callback argument types are incorrect (2)', function () {
+    assert.throws(() => math.map([math.sin, 2, 3], math.sqrt),
+      /TypeError: Function map cannot apply callback arguments sqrt\(value: function, index: Array, array: Array\) at index \[0]/)
+  })
 
   it('should operate from the parser', function () {
     assert.deepStrictEqual(