lmfit / asteval

minimalistic evaluator of python expression using ast module
https://lmfit.github.io/asteval
MIT License
176 stars 41 forks source link

Returned value is repeated on next invocation of eval #99

Closed jprafael closed 2 years ago

jprafael commented 2 years ago

Hi!

I believe there is in an issue with the eval logic when a return node is encountered and the interpreter is reused.

Checkout the following unit test:

def test_eval_return():
    """Tests that eval works with multiple statements with returns."""
    interpreter = Interpreter()

    result = interpreter.eval(
        "if True:\n"
        "    return True\n"
        "False\n"
    )

    assert result == True
    assert interpreter.eval('True') == True
    assert interpreter.eval('False') == False  # this assert fails because the interpreter keeps returning True 

From a user standpoint I'm expecting that multiple invocations to the interpreter will be isolated from each other, except on what gets stored in the symbol tables. Is this not the case?

newville commented 2 years ago

@jprafael thanks. That does look like a bug. I think that what is happening is that return True in your code is not raising an error, as it should (and will do in Python).
Because of that, the internally stored "return value" is left as True when eval() exists.

So, I think the proper fix is to internally set a flag "we're now evaluating a function/procedure body" and then "we're done evaluation a function/procedure body" and raise an error if a return is seen when that flag is not true. It might need two flags, actually to properly allow functions calling functions.

In the short term, you can set interpreter.retval = None after your interpreter.eval(...).

jprafael commented 2 years ago

I wasn't aware that top level returns where not allowed by asteval.

In python scripts the reason they are not allowed is because there is no other code reading the returned value, but with asteval we do have the host code that can read the value, so it makes sense to have it allowed.

This also would fix the following:

# assert fails because the return expression actually evaluates to None
assert interpreter.eval('return True') == True
newville commented 2 years ago

@jprafael

In python scripts the reason they are not allowed is because there is no other code reading the returned value, but with asteval we do have the host code that can read the value, so it makes sense to have it allowed.

Well, that return in the asteval language just returns to the top-level of the asteval interpreter. It does not return to the code calling interpreter.eval(). So I think it should be treated just like in Python. In thinking about this, I think it is not simply a flag but a counter for "how deep in function call stack are we"?

But, interpreter.eval('return True') should raise a SyntaxError, just like in Python.

newville commented 2 years ago

@jprafael I think this is resolved. Closing