brundonsmith / rust_lisp

A Rust-embeddable Lisp, with support for interop with native Rust functions
234 stars 20 forks source link

Add environment state map #22

Closed Qix- closed 1 year ago

Qix- commented 1 year ago

This provides the Env with the ability to attach arbitrary state instances to be retrieved later on, without exposing them to the actual runtime environment. Surprised that this wasn't a feature already, perhaps I missed something obvious. This allows applications to host the runtime and use it more like a scripting engine.

brundonsmith commented 1 year ago

Hmm. To me, this seems redundant with the existing Env entries... i.e. you can set/replace values in the main HashMap

It also seems redundant with PR #25. i.e. if you want to expose non-lisp Rust state to your Rust env functions, you could just do that via a closure (once the other PR is finished/merged) instead of introducing a new concept to the lisp environment

Am I missing a use-case?

Qix- commented 1 year ago

The main hash map exposes things to the lisp environment. I do not want that.

Also, closures don't allow their contents to be shared easily when Env's are .extend().

brundonsmith commented 1 year ago

I do not want that

If the state shouldn't be exposed to the lisp environment, why does it need to go in the Env itself (vs just existing in the normal, outside Rust code)? Is it meant to be exposed to the native Rust functions called from the lisp code, but not the lisp code itself?

If so- that's where I think it's redundant with closures (once those are implemented):

let mut my_state = 0;

env.define(
    Symbol::from("count-and-print"),
    Value::NativeFunc(|_env, args| {
        my_state += 1;
        println!("{}", my_state);
        Ok(expr.clone())
    }),
);

(again this doesn't work right now, but it seems like the most natural way to implement Rust-only state, and avoid introducing a new concept)

brundonsmith commented 1 year ago

Update: the above works now https://github.com/brundonsmith/rust_lisp/commit/af8ba5ff05ad6f4476f384e23cc443eef9fc883e

Please let me know if this serves the use-case here

Qix- commented 1 year ago

See tide - they implement state similarly. In fact, in a much more intrusive way. Everything down to Value and beyond would have had a <State> generic tacked on to it. I was going down that road with this library in particular and the changes would have touched pretty much every file in the repo 😅

The state map was a solution that provided the functionality without requiring a full refactor of the entire library, hence this PR.

I think in this case I'm inclined to agree, for now, that closures are a better solution. I would imagine there are cases that it'd break down but I can't imagine what they'd be, especially with the FnMut implementation.