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 <[email protected]>

Author: josdejong

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') {

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

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)')

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 () {