fix: keep implicit multiplication round-trippable after a symbol or accessor

HISTORY.md

@@ -2,6 +2,10 @@
 
 # unpublished changes since 15.2.0
 
+- Fix: `Node.toString` of an implicit multiplication no longer wraps a
+  `ConstantNode` that follows a `SymbolNode` or `AccessorNode` in parentheses,
+  which made the output reparse as a function call or index access
+  (for example `x 2` was stringified as `x (2)`, which parses as `x(2)`).
 - Docs: fix the browser example `rocket_trajectory_optimization.html` (#3654).
   Thanks @dvd101x.
 

src/expression/node/OperatorNode.js

@@ -1,4 +1,4 @@
-import { isNode, isConstantNode, isOperatorNode, isParenthesisNode } from '../../utils/is.js'
+import { isAccessorNode, isConstantNode, isFunctionNode, isNode, isOperatorNode, isParenthesisNode, isSymbolNode } from '../../utils/is.js'
 import { map } from '../../utils/array.js'
 import { createSubScope } from '../../utils/scope.js'
 import { escape } from '../../utils/string.js'
@@ -32,6 +32,39 @@ export const createOperatorNode = /* #__PURE__ */ factory(name, dependencies, ({
     return false
   }
 
+  /**
+   * Returns true if the string representation of the expression ends with a
+   * SymbolNode or AccessorNode, i.e. something that a directly following
+   * parenthesis would bind to as a function invocation or matrix index
+   * (for example `x` in `a b x` or `A[1]` in `c A[1]`). In that situation a
+   * parenthesized ConstantNode must NOT be appended for an implicit
+   * multiplication, since e.g. `x (2)` would reparse as the function call
+   * `x(2)` instead of the implicit multiplication `x * 2`.
+   * @param {Node} expression
+   * @param {string} parenthesis
+   * @return {boolean}
+   */
+  function endsWithSymbolOrAccessor (expr, parenthesis) {
+    // A parenthesized expression ends with ')', which the parser treats as an
+    // implicit multiplication rather than a function call, so it is safe.
+    if (isParenthesisNode(expr)) return false
+    if (isSymbolNode(expr) || isAccessorNode(expr)) return true
+    // FunctionNode also ends with ')', and `)(...)` is implicit multiplication.
+    if (isFunctionNode(expr)) return false
+    if (isOperatorNode(expr)) {
+      if (expr.args.length === 1) {
+        // a prefix unary operator (like `-a`) prints its operand last and can
+        // expose a symbol/accessor, whereas a postfix one (like `a!`) ends with
+        // the operator symbol and is therefore safe
+        return getAssociativity(expr, parenthesis) === 'right' &&
+          endsWithSymbolOrAccessor(expr.args[0], parenthesis)
+      }
+      // binary (and n-ary) operators print their right-most operand last
+      return endsWithSymbolOrAccessor(expr.args[expr.args.length - 1], parenthesis)
+    }
+    return false
+  }
+
   /**
    * Calculate which parentheses are necessary. Gets an OperatorNode
    * (which is the root of the tree) and an Array of Nodes
@@ -226,11 +259,16 @@ export const createOperatorNode = /* #__PURE__ */ factory(name, dependencies, ({
     // of ConstantNode.
     // In that case, parenthesize ConstantNodes that follow an unparenthesized
     // expression, even though they normally wouldn't be printed.
+    // The exception is when the preceding factor ends with a SymbolNode or
+    // AccessorNode: there a parenthesized ConstantNode would reparse as a
+    // function call or index access (e.g. `x (2)` parses as `x(2)`), so the
+    // ConstantNode must be left unparenthesized to keep the round-trip intact.
     if (args.length >= 2 && root.getIdentifier() === 'OperatorNode:multiply' &&
         root.implicit && parenthesis !== 'all' && implicit === 'hide') {
       for (let i = 1; i < result.length; ++i) {
         if (startsWithConstant(args[i], parenthesis) && !result[i - 1] &&
-            (parenthesis !== 'keep' || !isParenthesisNode(args[i - 1]))) {
+            (parenthesis !== 'keep' || !isParenthesisNode(args[i - 1])) &&
+            !endsWithSymbolOrAccessor(args[i - 1], parenthesis)) {
           result[i] = true
         }
       }

test/unit-tests/expression/node/OperatorNode.test.js

@@ -693,6 +693,35 @@ describe('OperatorNode', function () {
     }
   })
 
+  it('should not parenthesize a ConstantNode in an implicit multiplication when the preceding factor is a SymbolNode or AccessorNode', function () {
+    // A leading SymbolNode or AccessorNode followed by a parenthesized
+    // ConstantNode reparses as a function call / index access instead of an
+    // implicit multiplication, e.g. "x (2)" parses as the call "x(2)".
+    // Therefore the ConstantNode must not be parenthesized in those cases.
+    const xtimes2 = new OperatorNode('*', 'multiply', [xsym, two], true)
+    const abtimes2 = new OperatorNode('*', 'multiply',
+      [new OperatorNode('*', 'multiply', [asym, bsym], true), two], true)
+    const accessor = math.parse('A[1]')
+    const accessorTimes2 = new OperatorNode('*', 'multiply', [accessor, two], true)
+
+    for (const paren of ['auto', 'keep']) {
+      for (const expr of [xtimes2, abtimes2, accessorTimes2]) {
+        const estring = expr.toString({ parenthesis: paren, implicit: 'hide' })
+        const reparsed = math.parse(estring)
+        // Parsing the serialized form must reproduce the exact same grouping
+        assert.strictEqual(
+          reparsed.toString({ parenthesis: 'all' }),
+          expr.toString({ parenthesis: 'all' }),
+          `round-trip mismatch for ${estring} (parenthesis: ${paren})`)
+      }
+    }
+
+    // Spot-check the concrete serialized strings
+    assert.strictEqual(xtimes2.toString({ implicit: 'hide' }), 'x 2')
+    assert.strictEqual(abtimes2.toString({ implicit: 'hide' }), 'a b 2')
+    assert.strictEqual(accessorTimes2.toString({ implicit: 'hide' }), 'A[1] 2')
+  })
+
   it('should HTML implicit multiplications between ConstantNodes with parentheses', function () {
     const z = math.parse('(3)x')
     const a = math.parse('(4)(4)(4)(4)')