jsonata-js/jsonata

Variable references in function definitions don't use lexical scoping in the same scope

smking opened this issue · 4 comments

Function definitions are supposed to be closures: "When a lambda function is defined, the evaluation engine takes a snapshot of the environment and stores it with the function body definition." (https://bit.ly/3YmoKHC). However, the evaluator stores a reference instead of a snapshot of the environment, so references inside a function definition to a variable defined later in the same scope will refer to updated variable values.

For example, This returns an undefined value as expected (https://try.jsonata.org/umn7J2aFi):

(
    $f := function() { $x };
    $f()
)

And this returns 42 as expected because the value of $x is defined before the function definition (https://try.jsonata.org/xvUAkj0sJ):

(
    $x := 42;
    $f := function() { $x };
    $f()
)

However, if $x is assigned after the function definition but before the function call, the function call will use the new value of $x, which is unexpected behaviour (https://try.jsonata.org/ev2AQwZIW):

(
    $x := 42;
    $f := function() { $x };
    $x := 10;
    $f()
)

A possible solution is to store a copy instead of a reference to the environment here: https://github.com/jsonata-js/jsonata/blob/master/src/jsonata.js#L1568

For me current behaviour is an expected one.

This may just be a documentation issue. For example, the big example at the top of https://docs.jsonata.org/programming requires the current behavior to define $sin before $cos.

Thanks for raising this, it's an interesting point.

My initial reaction was that this is a bug for the reasons you've given, and that your proposed solution would fix it (notwithstanding that this would be a breaking change, as highlighted by the last comment).

Unfortunately, implementing this fix would preclude the ability to write recursive functions since the variable $f that's bound to the function won't be in scope at the time that the lambda is evaluated. Even if I was to 'special case' this somehow, it would still preclude writing mutually recursive functions.

So I think we are going to have to stick with the status quo and clarify the documentation.

Thanks Andrew. This makes sense.