From a6c9cbc120a46101a496477feab35b5e7adb2ea0 Mon Sep 17 00:00:00 2001 From: Jos de Jong Date: Thu, 8 Jun 2023 09:40:42 +0200 Subject: [PATCH 1/2] fix: #2880 not possible to map cube root `cbrt` --- HISTORY.md | 1 + src/expression/transform/filter.transform.js | 17 ++---- src/expression/transform/forEach.transform.js | 15 +---- src/expression/transform/map.transform.js | 17 ++---- src/function/matrix/filter.js | 13 +--- src/function/matrix/forEach.js | 15 +---- src/function/matrix/map.js | 46 ++------------ src/utils/applyCallback.js | 60 +++++++++++++++++++ test/unit-tests/function/matrix/map.test.js | 23 +++++-- 9 files changed, 102 insertions(+), 105 deletions(-) create mode 100644 src/utils/applyCallback.js diff --git a/HISTORY.md b/HISTORY.md index 5ea157f210..9cf96e5a8d 100644 --- 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 index 285ceeec19..da59452273 100644 --- 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 index 873b1eb766..4ab5888553 100644 --- 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 index 8ba712d9f6..f9c792b945 100644 --- 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,7 +50,7 @@ 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 @@ -58,9 +58,6 @@ export const createMapTransform = /* #__PURE__ */ factory(name, dependencies, ({ * @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 index 9a4c14a1a0..3bc359aaa6 100644 --- 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 index 73fd7cace7..d31a6f1bf4 100644 --- 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 index 66b1c23878..950a05b748 100644 --- 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,9 +58,6 @@ 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) { @@ -73,34 +65,8 @@ function _map (array, callback) { 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 new file mode 100644 index 0000000000..1586ce30c4 --- /dev/null +++ b/src/utils/applyCallback.js @@ -0,0 +1,60 @@ +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) + } + + function tryWithArgs (signature, args) { + try { + return signature.apply(null, args) + } catch (err) { + 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 index dd2d52026b..e3599a863c 100644 --- 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( From cfd8b943a53554fa4e11cf118f633b3125e000ab Mon Sep 17 00:00:00 2001 From: Jos de Jong Date: Thu, 8 Jun 2023 09:55:53 +0200 Subject: [PATCH 2/2] fix: bind signature, add comments --- src/utils/applyCallback.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/utils/applyCallback.js b/src/utils/applyCallback.js index 1586ce30c4..0135efe1c9 100644 --- a/src/utils/applyCallback.js +++ b/src/utils/applyCallback.js @@ -39,10 +39,17 @@ export function applyCallback (callback, value, index, array, mappingFnName) { 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(null, args) + 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)}`)