From 3150e21c8e83c19f3b4cc90e86eb4133271f3d1d Mon Sep 17 00:00:00 2001 From: jos Date: Thu, 9 Apr 2015 20:29:51 +0200 Subject: [PATCH] Fixed #313: parsed functions did not handle recursive calls correctly --- HISTORY.md | 7 ++ lib/expression/node/FunctionAssignmentNode.js | 15 ++-- lib/expression/node/IndexNode.js | 18 ++--- lib/expression/node/Node.js | 1 + lib/expression/node/SymbolNode.js | 7 +- lib/expression/node/UpdateNode.js | 9 ++- .../node/FunctionAssignmentNode.test.js | 70 +++++++++++++++++++ 7 files changed, 109 insertions(+), 18 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 4520845e7..9ce5ce3f9 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,12 @@ # History + +## not yet released, version 1.5.2 + +- Fixed #313: parsed functions did not handle recursive calls correctly. +- Performance improvements in parsed functions. + + ## 2015-04-08, version 1.5.1 - Fixed #316: a bug in rounding values when formatting. diff --git a/lib/expression/node/FunctionAssignmentNode.js b/lib/expression/node/FunctionAssignmentNode.js index 03fe8f43d..ebd18ef95 100644 --- a/lib/expression/node/FunctionAssignmentNode.js +++ b/lib/expression/node/FunctionAssignmentNode.js @@ -45,23 +45,24 @@ FunctionAssignmentNode.prototype.type = 'FunctionAssignmentNode'; * @private */ FunctionAssignmentNode.prototype._compile = function (defs) { + // add the function arguments to defs (used by SymbolNode and UpdateNode) + this.params.forEach(function (variable) { + defs.args[variable] = true; + }); + return 'scope["' + this.name + '"] = ' + - ' (function (scope) {' + - ' scope = Object.create(scope); ' + + ' (function () {' + ' var fn = function ' + this.name + '(' + this.params.join(',') + ') {' + ' if (arguments.length != ' + this.params.length + ') {' + - // TODO: use util.error.ArgumentsError here + // TODO: use util.error.ArgumentsError here? // TODO: test arguments error ' throw new SyntaxError("Wrong number of arguments in function ' + this.name + ' (" + arguments.length + " provided, ' + this.params.length + ' expected)");' + ' }' + - this.params.map(function (variable, index) { - return 'scope["' + variable + '"] = arguments[' + index + '];'; - }).join('') + ' return ' + this.expr._compile(defs) + '' + ' };' + ' fn.syntax = "' + this.name + '(' + this.params.join(', ') + ')";' + ' return fn;' + - ' })(scope);'; + ' })();'; }; /** diff --git a/lib/expression/node/IndexNode.js b/lib/expression/node/IndexNode.js index ed12171f7..3149b17a3 100644 --- a/lib/expression/node/IndexNode.js +++ b/lib/expression/node/IndexNode.js @@ -94,16 +94,17 @@ IndexNode.prototype.compileSubset = function(defs, replacement) { var useEnd = rangesUseEnd[i]; if (range instanceof RangeNode) { if (useEnd) { + defs.args.end = true; + // resolve end and create range - return '(function (scope) {' + - ' scope = Object.create(scope); ' + - ' scope["end"] = size[' + i + '];' + + return '(function () {' + + ' var end = size[' + i + '];' + ' return range(' + ' ' + range.start._compile(defs) + ', ' + ' ' + range.end._compile(defs) + ', ' + ' ' + (range.step ? range.step._compile(defs) : '1') + ' );' + - '})(scope)'; + '})()'; } else { // create range @@ -116,12 +117,13 @@ IndexNode.prototype.compileSubset = function(defs, replacement) { } else { if (useEnd) { + defs.args.end = true; + // resolve the parameter 'end' - return '(function (scope) {' + - ' scope = Object.create(scope); ' + - ' scope["end"] = size[' + i + '];' + + return '(function () {' + + ' var end = size[' + i + '];' + ' return ' + range._compile(defs) + ';' + - '})(scope)' + '})()' } else { // just evaluate the expression diff --git a/lib/expression/node/Node.js b/lib/expression/node/Node.js index 7d4813dd0..92827efbf 100644 --- a/lib/expression/node/Node.js +++ b/lib/expression/node/Node.js @@ -39,6 +39,7 @@ Node.prototype.compile = function (math) { // definitions globally available inside the closure of the compiled expressions var defs = { math: _transform(math), + args: {}, // can be filled with names of FunctionAssignment arguments _validateScope: _validateScope }; diff --git a/lib/expression/node/SymbolNode.js b/lib/expression/node/SymbolNode.js index 6d807d31d..aa9e3f785 100644 --- a/lib/expression/node/SymbolNode.js +++ b/lib/expression/node/SymbolNode.js @@ -41,7 +41,12 @@ SymbolNode.prototype._compile = function (defs) { defs['undef'] = undef; defs['Unit'] = Unit; - if (this.name in defs.math) { + if (this.name in defs.args) { + // this is a FunctionAssignment argument + // (like an x when inside the expression of a function assignment `f(x) = ...`) + return this.name; + } + else if (this.name in defs.math) { return '("' + this.name + '" in scope ? scope["' + this.name + '"] : math["' + this.name + '"])'; } else { diff --git a/lib/expression/node/UpdateNode.js b/lib/expression/node/UpdateNode.js index 323ad1f12..58c3b120c 100644 --- a/lib/expression/node/UpdateNode.js +++ b/lib/expression/node/UpdateNode.js @@ -40,8 +40,13 @@ UpdateNode.prototype.type = 'UpdateNode'; * @private */ UpdateNode.prototype._compile = function (defs) { - return 'scope["' + this.index.objectName() + '\"] = ' + - this.index.compileSubset(defs, this.expr._compile(defs)); + var lhs = (this.index.objectName() in defs.args) + ? this.name + ' = ' // this is a FunctionAssignment argument + : 'scope["' + this.index.objectName() + '\"]'; + + var rhs = this.index.compileSubset(defs, this.expr._compile(defs)); + + return lhs + ' = ' + rhs; }; /** diff --git a/test/expression/node/FunctionAssignmentNode.test.js b/test/expression/node/FunctionAssignmentNode.test.js index a7d8cff62..b6eb505da 100644 --- a/test/expression/node/FunctionAssignmentNode.test.js +++ b/test/expression/node/FunctionAssignmentNode.test.js @@ -5,6 +5,8 @@ var assert = require('assert'), Node = require('../../../lib/expression/node/Node'), ConstantNode = require('../../../lib/expression/node/ConstantNode'), OperatorNode = require('../../../lib/expression/node/OperatorNode'), + ConditionalNode = require('../../../lib/expression/node/ConditionalNode'), + FunctionNode = require('../../../lib/expression/node/FunctionNode'), FunctionAssignmentNode = require('../../../lib/expression/node/FunctionAssignmentNode'), AssignmentNode = require('../../../lib/expression/node/AssignmentNode'), RangeNode = require('../../../lib/expression/node/RangeNode'), @@ -48,6 +50,74 @@ describe('FunctionAssignmentNode', function() { }); + it ('should eval a recursive FunctionAssignmentNode', function () { + var x = new SymbolNode('x'); + var one = new ConstantNode(1); + var condition = new OperatorNode('<=', 'smallerEq', [x, one]); + var truePart = one; + var falsePart = new OperatorNode('*', 'multiply', [ + x, + new FunctionNode('factorial', [ + new OperatorNode('-', 'subtract', [ + x, + one + ]) + ]) + ]); + var n1 = new ConditionalNode(condition, truePart, falsePart); + + var n2 = new FunctionAssignmentNode('factorial', ['x'], n1); + + var expr = n2.compile(math); + var scope = {}; + var factorial = expr.eval(scope); + assert.equal(typeof scope.factorial, 'function'); + assert.equal(factorial(3), 6); + assert.equal(factorial(5), 120); + }); + + it ('should eval a recursive FunctionAssignmentNode with two recursive calls', function () { + var x = new SymbolNode('x'); + var zero = new ConstantNode(0); + var one = new ConstantNode(1); + var two = new ConstantNode(2); + + var n1 = new ConditionalNode( + new OperatorNode('<=', 'smallerEq', [x, zero]), + zero, + new ConditionalNode( + new OperatorNode('<=', 'smallerEq', [x, two]), + one, + new OperatorNode('+', 'add', [ + new FunctionNode('fib', [ + new OperatorNode('-', 'subtract', [ x, one ]) + ]), + new FunctionNode('fib', [ + new OperatorNode('-', 'subtract', [ x, two ]) + ]) + ]) + ) + ); + + var n2 = new FunctionAssignmentNode('fib', ['x'], n1); + //var n2 = math.parse('fib(x) = (x <= 0) ? 0 : ((x <= 2) ? 1 : (fib(x - 1) + f(fib - 2)))'); + + var expr = n2.compile(math); + var scope = {}; + var fib = expr.eval(scope); + + assert.equal(typeof fib, 'function'); + assert.equal(fib(0), 0); + assert.equal(fib(1), 1); + assert.equal(fib(2), 1); + assert.equal(fib(3), 2); + assert.equal(fib(4), 3); + assert.equal(fib(5), 5); + assert.equal(fib(6), 8); + assert.equal(fib(7), 13); + assert.equal(fib(8), 21); + }); + it ('should filter a FunctionAssignmentNode', function () { var a = new ConstantNode(2); var x = new SymbolNode('x');