search

kyren / gc-arena

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

Add `Collect` impl for `PhantomData` #9

Closed Aaron1011 closed 3 years ago

Aaron1011 commented 3 years ago

This makes it easier to use #[derive(Collect)] on a struct containing this type.

kyren commented 3 years ago

PhantomData makes complete sense obviously, but I'm wondering about MutationContext... how can you store a MutationContext inside something that needs to implement Collect? Wouldn't that have to go into the root object of an arena, which shouldn't be possible? I definitely don't think it's harmful, just help me out figuring out how it would work. I'm mostly asking in case there's some kind of hole in the safety logic here around MutationContext.

(Edit: also even if it wasn't otherwise a safety issue it would make the arena type self-borrowing)

Aaron1011 commented 3 years ago

@kyren: The Ruffle project stores a MutationContext in the (non-root) struct UpdateContext

An UpdateContext gets created inside a call to GcArena::mutate, and doesn't end up escaping the closure. UpdateContext never gets stored inside a Gc/GcCell, so there are never any lifetime issues.

adrian17 commented 3 years ago

I'm confused.

UpdateContext never gets stored inside a Gc/GcCell, so there are never any lifetime issues.

Then why would we need the Collect implementation for MutationContext?

Aaron1011 commented 3 years ago

Then why would we need the Collect implementation for MutationContext?

You're right - it looks like UpdateContext doesn't actually need a Collect implementation after all. I submitted a PR to Ruffle to remove it: https://github.com/ruffle-rs/ruffle/pull/3243

I still think a Collect impl for MutationContext would be sound, but I no longer have a use-case for it. It could actually be impossible to use in real code (e.g. behind a Gc/GcCell) due to lifetime issues - I haven't tested.

I'll remove that impl from this PR, and leave just PhantomData.

kyren commented 3 years ago

I still think a Collect impl for MutationContext would be sound

I believe you are correct, but it's maybe misleading to have it.

It could actually be impossible to use in real code (e.g. behind a Gc/GcCell) due to lifetime issues

I believe this is the case without unsafety, and you'd be at risk of creating a self-borrowing arena, which is why it was a bit concerning to me.

kyren commented 3 years ago

LGTM now!