Skip to content
This repository has been archived by the owner on May 23, 2021. It is now read-only.

Implement functional Evaluator, stateful Context and GoExpr #17

Merged
merged 4 commits into from
Sep 11, 2020

Conversation

lthibault
Copy link
Collaborator

@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 as

var ev parens.Evaluator

The Evaluator handles nil Analyzer and Expander 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 wraps BasicAnalyzer. It would look a bit like this:

type myAnalyzer struct {
    default parens.Analyzer
}

func (a myAnalyzer)Analyze(ev Evaluator, form value.Any) (Expr, error) {
    // type switch is just an example.  Any custom logic will work.
    switch f := form.(type) {
    case MyType:
        return customLogic(f)
    default:
        return a.default.Analyze(ev, form)
    }
}

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:

  1. I'm spawning lots of goroutines that copy very deep stacks, and the call to copy in basicContext.NewChild has become a bottleneck, so I want to replace []StackFrame with a persistent vector/list.
  2. I want to organize my goroutines in something akin to a supervision tree in order to clean up goroutines when their parents stop. To do this, I embed a context.Context in my parens.Context implementation and derive a new context using context.WithCancel on every call to NewChild.

4. GoExpr leverages the fact that Context is passed explicitly

GoExpr.Eval receives the parent Context as its first argument, then does the following:

  1. instantiate a NewChild() context
  2. call the embedded InvokeExpr's Eval method with the child context in a go statement.

Pass concrete (non-ptr) structs around where possible, to enforce
statelessness.
Factor value.Nil{} out of Evaluator.
@lthibault lthibault self-assigned this Sep 10, 2020
@lthibault
Copy link
Collaborator Author

Addendum: this PR implements #12 #14 and most of #16 (special form go is still needed).

@lthibault lthibault marked this pull request as draft September 11, 2020 11:00
@spy16
Copy link
Owner

spy16 commented Sep 11, 2020

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?

@lthibault
Copy link
Collaborator Author

lthibault commented Sep 11, 2020

@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.

@lthibault lthibault marked this pull request as ready for review September 11, 2020 13:30
@lthibault lthibault mentioned this pull request Sep 11, 2020
2 tasks
@spy16
Copy link
Owner

spy16 commented Sep 11, 2020

My main concern right now is the usability/ergonomics of the parens package. I have not clearly understood the benefits of separating Evaluator and Context.. Particularly this snippet:

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)
  • Context/Env can still provide public functions to create sub-contexts thus making it possible to implement different Exprs.
  • Every time we create a child from the root, we pass the analyzer/expander dependencies as well across.

Like you, I am really happy about the how the repl package has turned out and hoping for the same simplicity for this package too. (If we do this effectively, we have a nice and consistent design in all packages, repl.New(), reader.New(), parens.New())..

That said, if this overly complicates something else, or misses out an obvious benefit, we can definitely keep them separate.

@lthibault
Copy link
Collaborator Author

lthibault commented Sep 11, 2020

Concerning the fusing fusing Context and Evaluator, their current separation is mostly a side-effect of my hacking around: I was rearranging parts left and right, and this happened to be convenient for this exploratory coding sprint.

In any case, I'm definitely open to merging these two types. I had considered the possibility of implementing namespaces by passing a different Context to the evaluator, but this pushes the responsibility for namespace management up to the REPL, which feels like bad design. I don't have a compelling argument to keep them separate, anymore.

In practical terms, can we merge this in first and then do the refactor in #19? I suspect that embedding Evaluator into Context at this stage will cause conflicts in #19. (In retrospect, I should have managed my branching a bit differently, but I was in "move fast and break things" mode ...)

@spy16
Copy link
Owner

spy16 commented Sep 11, 2020

"move fast and break things" mode

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.

@lthibault
Copy link
Collaborator Author

force push, random commits, multiple near-complete rewrites 😅

lol we adhere to the highest professional standards, here at parens! 😆

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.

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.

@lthibault lthibault merged commit d90f0e3 into master Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants