-
Notifications
You must be signed in to change notification settings - Fork 3
Implement functional Evaluator, stateful Context and GoExpr #17
Conversation
Pass concrete (non-ptr) structs around where possible, to enforce statelessness.
Factor value.Nil{} out of Evaluator.
Should i just look at #19 ? That seems to change design of context and how stack is done. Would be easier to review that directly? |
@spy16 Oh whoops, my bad. I mean to post #17 (comment) under #19 🤦♂️. (I'm moving the comment to the appropriate location, now. Sorry for the confusion!) I think it would be easier to start with this one. #19 builds on top of the basic architecture proposed here. |
My main concern right now is the usability/ergonomics of the parens package. I have not clearly understood the benefits of separating var ev parens.Evaluator
ctx := parens.NewContext()
fmt.Println(ev.Eval(ctx, "hello")) Any usage of parens i can think of would always involve starting from a root context. Any child context would be launched as a result of evaluating different expressions (e.g., GoExpr), but except for internal Expr implementations, user wouldn't have to create and manage multiple thread contexts explicitly in Go code.. While it is good to be able to separate out stateless things from the context which is essentially thread state, I am not able to see the benefits over a simpler approach shown below: // context can be called as Env as well.
rootCtx := parens.New(....) // functional options can still set custom analyzer, expander, map-factory etc.
rootCtx.Eval(form)
Like you, I am really happy about the how the That said, if this overly complicates something else, or misses out an obvious benefit, we can definitely keep them separate. |
Concerning the fusing fusing In any case, I'm definitely open to merging these two types. I had considered the possibility of implementing namespaces by passing a different In practical terms, can we merge this in first and then do the refactor in #19? I suspect that embedding |
No worries, it's a good way to get fast feedback and I do this too.. (force push, random commits, multiple near-complete rewrites 😅 ) Do you want me to review this? OR we could just merge it and directly review the other PR and make changes as necessary i think. |
lol we adhere to the highest professional standards, here at
Let's merge it and review #19. BTW, you'll notice a bit of experimental stuff in that PR as well (namely: the error system). Feel free to aggressively prune things back, if you don't see a clear advantage. |
@spy16 Ok here's what I had in mind. Let me know what you think!
A few notes:
1. Zero-value Evaluator is ready to use
I have removed
parens.New
in favor of a concrete (non-pointer)Evaluator
that can be initialized simply asThe
Evaluator
handles nilAnalyzer
andExpander
fields by deferring to the built-in implementations, so the zero-value is usable. I really like this because it helps us communicate to users that the Evaluator is stateless.You'll also notice that all the method receivers are now concrete, as opposed to pointer receivers. I have done this in order to help enforce the statelessness of
Evaluator
.Relatedly ...
2. BasicAnalyzer has been exported
Users can implement dynamic dispatch (i.e.:
replaceBuiltin=false
) by writing a custom analyzer that wrapsBasicAnalyzer
. It would look a bit like this:3. All state is contained in Context
Because state is inherently complex, I've made
Context
an interface. The idea is that users should be able to substitute their own implementations, where appropriate. A few hypothetical use-cases:copy
inbasicContext.NewChild
has become a bottleneck, so I want to replace[]StackFrame
with a persistent vector/list.context.Context
in myparens.Context
implementation and derive a new context usingcontext.WithCancel
on every call toNewChild
.4. GoExpr leverages the fact that Context is passed explicitly
GoExpr.Eval
receives the parentContext
as its first argument, then does the following:NewChild()
contextInvokeExpr
'sEval
method with the child context in ago
statement.