From a30d1f8ce6e4f5613cd26f76a5d85db579fa801f Mon Sep 17 00:00:00 2001 From: Jos de Jong Date: Thu, 27 Jul 2023 11:42:36 +0200 Subject: [PATCH] fix: #2990 DenseMatrix can mutate input arrays (#2991) * fix: #2990 DenseMatrix can mutate input arrays * chore: simplify internal function `preprocess` * chore: document ugly workaround of using `matrix.subset` to mutate a nested Array * chore: better solution for `assign` * chore: fix linting issue * chore: add a unit test for `multiply` testing whether the operation is immutable * chore: fix linting issue --------- Co-authored-by: Glen Whitney --- src/expression/node/utils/assign.js | 10 +++++-- src/type/matrix/DenseMatrix.js | 17 ++++++----- .../function/arithmetic/multiply.test.js | 20 +++++++++++++ .../type/matrix/DenseMatrix.test.js | 28 +++++++++++++++++++ 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/src/expression/node/utils/assign.js b/src/expression/node/utils/assign.js index b05c317818..6e1b59b833 100644 --- a/src/expression/node/utils/assign.js +++ b/src/expression/node/utils/assign.js @@ -19,8 +19,14 @@ export function assignFactory ({ subset, matrix }) { return function assign (object, index, value) { try { if (Array.isArray(object)) { - // we use matrix.subset here instead of the function subset because we must not clone the contents - return matrix(object).subset(index, value).valueOf() + const result = matrix(object).subset(index, value).valueOf() + + // shallow copy all (updated) items into the original array + result.forEach((item, index) => { + object[index] = item + }) + + return object } else if (object && typeof object.subset === 'function') { // Matrix return object.subset(index, value) } else if (typeof object === 'string') { diff --git a/src/type/matrix/DenseMatrix.js b/src/type/matrix/DenseMatrix.js index a16d037e3c..027cf697fe 100644 --- a/src/type/matrix/DenseMatrix.js +++ b/src/type/matrix/DenseMatrix.js @@ -920,19 +920,18 @@ export const createDenseMatrixClass = /* #__PURE__ */ factory(name, dependencies /** * Preprocess data, which can be an Array or DenseMatrix with nested Arrays and - * Matrices. Replaces all nested Matrices with Arrays + * Matrices. Clones all (nested) Arrays, and replaces all nested Matrices with Arrays * @memberof DenseMatrix - * @param {Array} data + * @param {Array | Matrix} data * @return {Array} data */ function preprocess (data) { - for (let i = 0, ii = data.length; i < ii; i++) { - const elem = data[i] - if (isArray(elem)) { - data[i] = preprocess(elem) - } else if (elem && elem.isMatrix === true) { - data[i] = preprocess(elem.valueOf()) - } + if (isMatrix(data)) { + return preprocess(data.valueOf()) + } + + if (isArray(data)) { + return data.map(preprocess) } return data diff --git a/test/unit-tests/function/arithmetic/multiply.test.js b/test/unit-tests/function/arithmetic/multiply.test.js index 159dc4c6db..089bce97bb 100644 --- a/test/unit-tests/function/arithmetic/multiply.test.js +++ b/test/unit-tests/function/arithmetic/multiply.test.js @@ -875,6 +875,26 @@ describe('multiply', function () { }) }) + describe('immutable operations', function () { + it('should not mutate the input (arrays)', function () { + const a = Object.freeze([[1, 2], [3, 4]]) + const b = Object.freeze([[5, 6], [7, 8]]) + + assert.deepStrictEqual(multiply(a, b), [[19, 22], [43, 50]]) + assert.deepStrictEqual(a, [[1, 2], [3, 4]]) + assert.deepStrictEqual(b, [[5, 6], [7, 8]]) + }) + + it('should not mutate the input (arrays with nested Matrices)', function () { + const a = Object.freeze([math.matrix([1, 2]), math.matrix([3, 4])]) + const b = Object.freeze([math.matrix([5, 6]), math.matrix([7, 8])]) + + assert.deepStrictEqual(multiply(a, b), [[19, 22], [43, 50]]) + assert.deepStrictEqual(a, [math.matrix([1, 2]), math.matrix([3, 4])]) + assert.deepStrictEqual(b, [math.matrix([5, 6]), math.matrix([7, 8])]) + }) + }) + it('should LaTeX multiply', function () { const expression = math.parse('multiply(2,3)') assert.strictEqual(expression.toTex(), '\\left(2\\cdot3\\right)') diff --git a/test/unit-tests/type/matrix/DenseMatrix.test.js b/test/unit-tests/type/matrix/DenseMatrix.test.js index 665fe42adb..b7193ae6f6 100644 --- a/test/unit-tests/type/matrix/DenseMatrix.test.js +++ b/test/unit-tests/type/matrix/DenseMatrix.test.js @@ -158,6 +158,34 @@ describe('DenseMatrix', function () { it('should throw an error when called with invalid datatype', function () { assert.throws(function () { console.log(new DenseMatrix([], 1)) }) }) + + it('should not mutate the input data when creating a Matrix (1)', function () { + const data = [[1, 2]] + Object.freeze(data) + + const matrix = new DenseMatrix(data) // should not throw "TypeError: Cannot assign to read only property '0' of object '[object Array]'" + assert.deepStrictEqual(matrix.valueOf(), [[1, 2]]) + assert.notStrictEqual(matrix.valueOf(), data) + }) + + it('should not mutate the input data when creating a Matrix (2)', function () { + const nestedMatrix = new DenseMatrix([1, 2]) + const data = [nestedMatrix] + + const matrix = new DenseMatrix(data) + assert.deepStrictEqual(matrix._data, [[1, 2]]) + assert.deepStrictEqual(data, [nestedMatrix]) // should not have replaced the nestedMatrix in data itself + }) + + it('should not mutate the input data operating on a Matrix', function () { + const data = [[1, 2]] + + const matrix = new DenseMatrix(data) + matrix.set([0, 1], 42) + + assert.deepStrictEqual(matrix, new DenseMatrix([[1, 42]])) + assert.deepStrictEqual(data, [[1, 2]]) + }) }) describe('size', function () {