amethyst / shred

Shared resource dispatcher
Apache License 2.0
237 stars 66 forks source link

Systems without lifetimes #101

Closed udoprog closed 5 years ago

udoprog commented 5 years ago

Hey!

So to start out: this is an experiment. I don't expect it to be merged, but I also want to make sure to document that I took this approach for a spin.

I was reading the shred code to try to understand what the lifetime associated to systems was actually used for.

The lifetime appears to be required to have a named lifetime in the fetch function that is also associated with the return value (e.g. Self).

This guarantees that Resources cannot be modified, while the fetch function is running.

I then tried to figure out what the scope of this lifetime would be. And it appears like this doesn't actually come into effect that much. Primarily it's used in RunNow: https://github.com/slide-rs/shred/blob/master/src/system.rs#L124

But here is the kicker: resources is already borrowed here, so do we really need to track the lifetime of the values borrowed from it? These should already be valid for the duration of this function call, which calls into the System itself.

So this is a patch that removes that lifetime, in favor of making Read and Write keep track of a raw pointer into TrustCell.

Some major caveats

Resources wrapped inRead and Write must never, ever, ever be able to leave or be stored in the System. This is normally accomplished with the lifetime associated with the system itself. This could probably be patched by treating them as weak pointers to a specific generation of the collection of resources, and panic in case someone tried to use them if this generation has changed.

Some APIs have been completely butchered for their existing safety guarantees, and some safety holes are available as APIs. E.g. it's possible to discard and SystemFetch because that's needed to perform composition of fetches.

To wrap up: this is probably a terrible idea. It was fun to try out, but please don't merge :).

torkleyy commented 5 years ago

Hey! Interesting experiment. I agree with what you said, this produces many safety issues :smile: What also needs to be considered is that resources are also fetched outside of systems. In that case, you can just drop the resources before the handle to it, which would be very bad. As you said you can also just hold the handle which basically makes a lot of other systems panic and allows accessing invalid memory. There are solutions to solve some of these issues, but I'm not sure if it's worth it.

zakarumych commented 5 years ago

This is interesting idea but code violates rust memory model in multiple places. Just for exampe

let cell = TrustCell::new(42u32);
let r = cell.borrow();
drop(cell);
println!("{}", *r); // BOOM!
Xaeroxe commented 5 years ago

On another note we might be able to use Pin pointers to guarantee that read and write structures never move. if that's the only problem this API poses we might be able to make it safe that way.

zakarumych commented 5 years ago

Pin only works well for self-references.

udoprog commented 5 years ago

@omni-viral Good eye! I don't think I have a use like that, but that was what was hinted at with:

Some APIs have been completely butchered for their existing safety guarantees [..]

udoprog commented 5 years ago

On another note we might be able to use Pin pointers to guarantee that read and write structures never move. if that's the only problem this API poses we might be able to make it safe that way.

Yeah, the right solution is to use lifetimes like we are today. It's a pity we don't have HRTBs and are limited to how we can name these. I'd want something like:

impl System for SimpleSystem {
    type SystemData = (
        Read<Something>,
        Write<SomethingElse>,
    );

    fn run<'r>(&mut self, data: SystemData::Fetch<'r>) {
        /*  */
    }
}

Preferably 'r would be elided!

zakarumych commented 5 years ago

It's a pity we don't have HRTBs

This particular use case has a workaround.

@omni-viral Good eye! I don't think I have a use like that

When function is intended to be used only in specific way to avoid UB then it just should be marked unsafe

udoprog commented 5 years ago

@omni-viral

When function is intended to be used only in specific way to avoid UB then it just should be marked unsafe

I was lazy, since this was more like an interesting exercise than anything serious. This is not intended to be merged. I might not have communicated that clearly enough for you.

zakarumych commented 5 years ago

@udoprog I was thinking that this is experiment to see if it's possible to make Systems without lifetimes but still safe and sound. The code is not sound, I was just pointing on that 😄

udoprog commented 5 years ago

Ok, thanks for clarifying!

torkleyy commented 5 years ago

I think we can close this since it's very unlikely it will be merged. I believe Rust even improved the situation by allowing to use '_ as lifetime.