EvalContext discards context on recursion

[DeadlySurgeon]

evaluator.EvalContext has some points where it calls Eval, which then just calls EvalContext with a background context, thus dropping the original context passed to it. This should be fixed so it instead calls EvalContext with the provided context.

EG: https://github.com/skx/monkey/blob/master/evaluator/evaluator.go#L56

func EvalContext(ctx context.Context, node ast.Node, env *object.Environment) object.Object {
        ...
	case *ast.ExpressionStatement:
		return Eval(node.Expression, env)

[DeadlySurgeon]

This also extends into EvalContext calling evalProgram which then executes Eval, further dropping the context.

[skx]

Now you say it .. yes. This is obviously incorrect! Thank you for the bug-report.

If you want to have a stab at resolving it you're welcome, otherwise I'll look at it over the weekend.

[DeadlySurgeon]

I think you should take a swing at it. I recommend just temp removing the Eval function, and then any places that call it that are giving an error should just be changed to EvalContext, and then once that's resolved add it back.

[skx]

I suspect this solution would get messy quickly, since EvalContext passes to evalBlockStatement, etc, etc.

The better change is to add a call to "SetContext", and use that to update the global context variable - which defaults to context.Background, and only test that. There then wouldn't be any difference between Eval and EvalWithContext.

• Shuffle the code into eval where it used to live.
  • Add the context-testing at the head of that function, using the global variable.
• EvalWithContext is then a minimal wrapper which does two things:
  • call SetContext to update the global variable.
  • call Eval(...)

Possibly SetContext and the global variable can be local/package-local, rather than externally accessible. That preserves the current API.

[DeadlySurgeon]

I wouldn't have a global variable context, that's pretty bad practice. Changing unexported functions isn't a breaking change.

EDIT:
I just noticed the existing global context, and I'd honestly remove it as it isn't concurrent safe and allows for bad flow control.

Also it looks like SetContext is only used in a test file, removing it wouldn't break any other bits if your code base, you could just change your tests to call EvalContext.

[skx]

Yeah having the global is not great - the issue with this implemetnation is that everything's global, more or less, there should be a struct to start state of the interpreter and mediate access appropriately.

Still it looks like your PR does all the right things and I'll merge that. Thanks for taking the time to report the issue and resolve it.

[DeadlySurgeon]

NP. I've been trying to complete my own monkey interpreter for a while and kept getting sidetracked, and seeing your repository has helped a lot in my own pursuit, especially around the part where I parse my tokens into the AST for then further execution.