kyren / gc-arena

Incremental garbage collection from safe Rust
Creative Commons Zero v1.0 Universal
436 stars 36 forks source link

Expand upon `Lock` APIs #59

Closed moulins closed 1 year ago

moulins commented 1 year ago

A bunch of bells and whistles:

I wanted to add OnceLock too, but I realized that OnceCell will only be stabilized in Rust 1.70, so I guess I'll have to wait a little still.

kyren commented 1 year ago

We discussed this a bit in Discord, just repeating myself here for future reference...

1) I love the name unlock to go with the lock types, makes it absolutely clear to me what they do.

2) The safety of Lock::take and RefLock::take have some real subtlety to them. The parametricity argument makes complete sense to me, but I pointed out an extreme corner case point that it does make potentially unsound code with a Default impl have some... non-locality to it. One could imagine the unsafe internals of a module containing a Default::default that is doing some weird tricks to materialize a MutationContext from global state, and the existence of RefLock::take making it... "more unsafe" than it would otherwise be. This is an extremely fine point and it is very difficult to imagine the existence of RefLock::take making a sound API into an unsound one, especially since having such a Default impl is automatically unsound as an API, but it might be theoretically possible. In any case, it's such a minor point that we agreed it's fine to just leave a slightly more detailed comment explaining the situation. @moulins also pointed out that there is a more "local" possible API that is strictly more powerful: fn set_static<R, 'gc>(&Lock<Root<'gc, R>>, f: impl for<'r> FnOnce() -> Root<'r, R>) where R: for<'r> Rootable<'r>; but also pointed out it would be a bit obnoxious to use.

kyren commented 1 year ago

This looks really good to me!

Do you think it would be worth it to provide a more convenient constructor on Gc<Lock> / Gc<RefLock> like the old GcCell had?

We have the Gc[Ref]Lock typedefs, but you still have to type e.g. Gc::new(mc, RefLock::new(t)). We could have Gc::new_lock / Gc::new_ref_lock or something to make it slightly more ergonomic? It might not even really be worth it, but just mentioning it as a suggestion.

Other than that and the extremely minor point before, LGTM! Feel free to merge as soon as you're comfortable, with / without either of those changes because they're pretty minor.

moulins commented 1 year ago

We have the Gc[Ref]Lock typedefs, but you still have to type e.g. Gc::new(mc, RefLock::new(t)). We could have Gc::new_lock / Gc::new_ref_lock or something to make it slightly more ergonomic? It might not even really be worth it, but just mentioning it as a suggestion.

This already adds impl<T> From<T> for [Ref]Lock<T>, so you can use Gc::new(mc, t.into()), which (I think) is ergonomic enough.


P.S. Added a commit to expand on the safety comment of [Ref]Lock::take.

kyren commented 1 year ago

This already adds impl From for [Ref]Lock, so you can use Gc::new(mc, t.into()), which (I think) is ergonomic enough.

Ah yeah, fair enough, that's probably plenty. LGTM!