Repository: josdejong/mathjs
Pull request #1006 (2018-01-08 06:13:51)

Do not let `simplifyConstants` binarize functions with more than two arguments
When `simplify` (or more specifically, `simplifyConstants`) is given a function with more than two arguments, it binarizes the function calls; that is, `simplify("f(x, y, z)")` evaluates to `f(f(x, y), z)`. This is not correct (unless the function happens to be associative), and can even potentially cause the expression to become malformed if `f` requires three arguments.

Additionally, there is a bug where evaluating `simplify("f(4)")` with `f` undefined will throw an error, where `simplify("f(x)")` and `simplify("f(4,5)")` will evaluate fine.

This PR fixes both of these issues: it only allows the functions `add` and `multiply` to become binarized, and will handle undefined unary functions without throwing an error. Please let me know if this PR needs any other improvements, or if there are any unintended side effects of these changes.


Patch:
diff --git a/lib/function/algebra/simplify/simplifyConstant.js b/lib/function/algebra/simplify/simplifyConstant.js
index 0ca60f737..8b22ab15e 100644
--- a/lib/function/algebra/simplify/simplifyConstant.js
+++ b/lib/function/algebra/simplify/simplifyConstant.js
@@ -10,6 +10,7 @@ function factory(type, config, load, typed, math) {
   var createMakeNodeFunction = util.createMakeNodeFunction;
   var ConstantNode = math.expression.node.ConstantNode;
   var OperatorNode = math.expression.node.OperatorNode;
+  var FunctionNode = math.expression.node.FunctionNode;
 
   function simplifyConstant(expr) {
     var res = foldFraction(expr);
@@ -158,6 +159,29 @@ function factory(type, config, load, typed, math) {
         if (math[node.name] && math[node.name].rawArgs) {
           return node;
         }
+
+        // Process operators as OperatorNode
+        var operatorFunctions = [ 'add', 'multiply' ];
+        if (operatorFunctions.indexOf(node.name) === -1) {
+          var args = node.args.map(foldFraction);
+
+          // If all args are numbers
+          if (!args.some(type.isNode)) {
+            try {
+              return _eval(node.name, args);
+            }
+            catch (ignoreandcontine) {}
+          }
+
+          // Convert all args to nodes and construct a symbolic function call
+          args = args.map(function(arg) {
+            return type.isNode(arg) ? arg : _toNode(arg);
+          });
+          return new FunctionNode(node.name, args);
+        }
+        else {
+          // treat as operator
+        }
         /* falls through */
       case 'OperatorNode':
         var fn = node.fn.toString();


Test patch:
diff --git a/test/function/algebra/simplify.test.js b/test/function/algebra/simplify.test.js
index 366961752..89f9c325a 100644
--- a/test/function/algebra/simplify.test.js
+++ b/test/function/algebra/simplify.test.js
@@ -196,6 +196,8 @@ describe('simplify', function() {
 
   it('should handle non-existing functions like a pro', function() {
     simplifyAndCompare('foo(x)', 'foo(x)');
+    simplifyAndCompare('foo(1)', 'foo(1)');
+    simplifyAndCompare('myMultiArg(x, y, z, w)', 'myMultiArg(x, y, z, w)');
   });
 
   it ('should support custom rules', function() {