New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in FunctionAssignmentNode compilation causes crash if parameter is later used as global symbol #485

Closed
balagge opened this Issue Oct 20, 2015 · 2 comments

Comments

Projects
None yet
2 participants
@balagge

balagge commented Oct 20, 2015

FunctionAssignmentNode creates an entry in defs.args for each parameter name (https://github.com/josdejong/mathjs/blob/master/lib/expression/node/FunctionAssignmentNode.js#L55-L57). This is used by SymbolNode, which in turn changes the way it compiles a symbol (instead of generating code that retrieves the symbol from scope / math defs, it simply inserts the symbol name verbatim into the compiled expression.) (https://github.com/josdejong/mathjs/blob/master/lib/expression/node/SymbolNode.js#L47-L50).

This results in the right code generated for the function definition. However, the defs.args entries stay for the whole time of the compilation, and any later part of the expression referring to the same parameter name as a global symbol name will always be generated incorrecly (verbatim, instead of getting the value from scope).

Example expression:

var formula = "a = x; f(x) = a * x; b = x;"

The above expression references "x" three times: first as a global symbol, second as a function formal parameter name, then again as a global symbol.

If you compile the above formula:

var expr = math.parse(formula);
var code = expr.compile();
var js = code.eval.toString();

and examine the generated code, the third time x is referenced, it yields an undefined symbol, as js has the following value above:

function (scope) {    if (scope) _validateScope(scope);    scope = scope || {};    return (function () {var results = [];scope["a"] = ("x" in scope ? scope["x"] : undef("x"));scope["f"] =   (function () {    var fn = function f(x) {      if (arguments.length != 1) {        throw new SyntaxError("Wrong number of arguments in function f (" + arguments.length + " provided, 1 expected)");      }      return math.multiply(("a" in scope ? scope["a"] : undef("a")), x)    };    fn.syntax = "f(x)";    return fn;  })();scope["b"] = x;return new ResultSet(results);})();  }

or, inserting line breaks:

function (scope) {
    if (scope) _validateScope(scope);
    scope = scope || {};
    return (function () {
        var results = [];
        scope["a"] = ("x" in scope ? scope["x"] : undef("x"));
        scope["f"] = (function () {
            var fn = function f(x) {
                if (arguments.length != 1) {
                    throw new SyntaxError("Wrong number of arguments in function f (" + arguments.length + " provided, 1 expected)");
                }
                return math.multiply(("a" in scope ? scope["a"] : undef("a")), x)
            };
            fn.syntax = "f(x)";
            return fn;
        })();
        scope["b"] = x;
        return new ResultSet(results);
    })();
}

The third reference of x is caused by the bug, which is obviously an undefined reference for x and will cause crashing at runtime:

), x)    };    fn.syntax = "f(x)";    return fn;  })();scope["b"] = x;return n
                                                                    ^
ReferenceError: x is not defined

Another malfunctioning would be in the following formula:

var formula = "f(x) = a * x; g(y) = x * y;"

Where the second reference to x would be as a formal parameter name, instead of a global symbol name (x is NOT a parameter of g)

Suggested solution:

In the _compile method of FunctionAssignmentNode, instead of returning the compiled code directly, first compile the function expression into a temporary value, then reset defs.args before returning the compiled code.

defs.args is actually only required while generating the code for the internal return statement

this.expr._compile(defs) (on line 67)

(https://github.com/josdejong/mathjs/blob/master/lib/expression/node/FunctionAssignmentNode.js#L67),

so probably that should be done on a separate line of code instead of inserted into the string expression directly.

@josdejong

This comment has been minimized.

Owner

josdejong commented Oct 24, 2015

Thanks for reporting this bug, will fix it.

@josdejong josdejong added the bug label Oct 24, 2015

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 28, 2015

This is fixed now in the develop branch.

@josdejong josdejong closed this in 9ffab39 Dec 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment