Switch to Arc?

[msparkles]

Is there any reason not to prefer Arc over Rc?

[brundonsmith]

Arc is a little more expensive to clone than Rc is. Do you have a specific case where you need Lisp values to be shareable across threads? If so, I could imagine this being a crate feature that people can toggle at build-time

[msparkles]

We had a case where that'd be needed, yes, but we got over the idea of using Lisp for the project.

It'd still be nice to see it being a crate feature, though. Since Arc's are indeed a bit more expensive.

[msparkles]

re this issue: We're going to still use the project, so we'd very much like to see it have an Arc feature, yes!

[brundonsmith]

Ah ok! In that case I'll look into adding it 👍

[msparkles]

Hmm, Arc and RefCell.

Probably need to switch to Arc\ for the Arc version?

[msparkles]

We don't see a way to get the pointer like with RefCells with Mutex... Do we really need a RefCell in the first place? Or can we just clone-on-write if there's other strong references?

[msparkles]

We made a fork using Arcs with a new test for it and all tests passed!

https://github.com/Mg138/rust_lisp

May check it out for how it can be done, or possible bugs?

[brundonsmith]

I made an implementation that can be toggled with the feature flag arc: https://github.com/brundonsmith/rust_lisp/tree/arc-feature-addition

Both versions build and have all the existing tests pass, but I wasn't able to get the new test you added to pass. I banged on it for a little while before setting it aside for the time being.

I'm not super experienced with concurrent Rust, so I was wondering if you could take a look at the error?

error[E0277]: `(dyn Any + 'static)` cannot be shared between threads safely
   --> tests/interpreter.rs:403:24
    |
403 |       std::thread::spawn(move || {
    |  _____------------------_^
    | |     |
    | |     required by a bound introduced by this call
404 | |         env.define(
405 | |             Symbol::from("my_closure"),
406 | |             Value::NativeClosure(reference::new(
...   |
423 | |         eval(env, &expression).unwrap();
424 | |     })
    | |_____^ `(dyn Any + 'static)` cannot be shared between threads safely
    |
    = help: the trait `Sync` is not implemented for `(dyn Any + 'static)`
    = note: required for `Arc<(dyn Any + 'static)>` to implement `Send`
    = note: required because it appears within the type `Value`
    = note: required because it appears within the type `(rust_lisp::model::Symbol, Value)`
    = note: required for `hashbrown::raw::RawTable<(rust_lisp::model::Symbol, Value)>` to implement `Send`
    = note: required because it appears within the type `hashbrown::map::HashMap<rust_lisp::model::Symbol, Value, RandomState>`
    = note: required because it appears within the type `std::collections::HashMap<rust_lisp::model::Symbol, Value>`
    = note: required because it appears within the type `Env`

Otherwise maybe I'll make another attempt later

[msparkles]

You need to manually add + Send + Sync for the closure's trait bound.

That's a thing because Rust can't automatically add trait bounds, your closure would automatically implement Send and Sync so long as it doesn't borrow any non-Send/non-Sync.

[msparkles]

Should also add + Send + Sync to the external ref, btw.