Prevent conflicting system resource parameters

[memoryruins]

Currently, a system is allowed to have multiple resource parameters to the same resource type. No current checks prevent multiple mutable borrows aliasing the same resource. Surprisingly, this applies to multiple local resources too in a slightly different way.

examples

#[derive(Default)]
pub struct Buffer(Vec<u8>);

// builds without an error and goes uncaught by runtime checks.
fn system(a: Res<Buffer>, mut b: ResMut<Buffer>, mut c: ResMut<Buffer>) {
    // heck, two aliasing mutable borrows + a shared borrowed
    let x: &Vec<u8> = &a.0;
    let y: &mut Vec<u8> = &mut b.0;
    let z: &mut Vec<u8> = &mut c.0;
    dbg!(x.len());
    y.push(42);
    z.push(42);
    dbg!(x.len());
}

pub fn system(mut a: Local<Buffer>, mut b: Local<Buffer>) {
    let a = &mut a.0;
    let b = &mut b.0;
    assert_eq!(a.len(), 0);
    assert_eq!(b.len(), 0);
    a.push(42);
    assert_eq!(a.len(), 1);'
    
     // this assert will fail because the two locals _are_ the same resource
    assert_eq!(b.len(), 0);
}

This PR adds access checks to the SystemParam::init implementation of Res, ResMut, ChangedRes, and Local.

Something I'm unsure about is how I split out the local type accesses. Without tracking local type access independently, a Local<Foo> and Res<Foo> would incorrectly lead to a conflict. Is that change appropriate or is tracking local resources needed for parallel schedulers checks?

[memoryruins]

This should prevent the issues pointed out in #598 (except for the issue title).
If splitting up the TypeAccess tracking for local resources is the correct course of action too, then it potentially resolves #483 as well.

[bonsairobo]

I think it can be useful to have multiple copies of the same Res<T> in a SystemParam. It enables encapsulation for sub-objects.

#[derive(SystemParam)]
struct Outer<'a> {
    inner: Inner<'a>,
    foo: Res<'a, Foo>,
}

impl<'a> Outer<'a> {
    pub fn use_inner(&self) { self.inner.use_foo() }
}

mod inner {
    #[derive(SystemParam)]
    pub struct Inner<'a> {
        foo: Res<'a, Foo>,
    }

    impl<'a> Inner<'a> {
        pub fn use_foo(&self) {}
    }
}

[memoryruins]

@bonsairobo do you have a fuller example? Inner is repeating the fields thar Outer already has access to.

[bonsairobo]

I just mean that Inner might be some reusable SystemParam object, so you can build larger objects out of it. But sometimes both the inner and outer object need to access the same global resource. It shouldn't be a problem if it's Res, so disallowing this is limiting from a design perspective.

[memoryruins]

Makes sense ^^ it’s a small change to make the Res/ChangedRes checks look for only write accesses rather than reads/writes. I’ll push a change later today.

[bonsairobo]

@memoryruins Thanks for hearing me out. I only spoke up because I've run into this issue before with specs, and I found it annoying that it wouldn't let me read the same thing twice from the same system.

[cart]

Very good call! Thanks 😄

If splitting up the TypeAccess tracking for local resources is the correct course of action too, then it potentially resolves #483 as well

Yeah I think this is a good call. Right now its fine to just ignore local resource access in the scheduler because there are only two ways to access Local resources:

1. exclusive &mut World access
2. via the Local SystemParam, which is restricted to accessing the current system's local resource.

If we ever add the ability to access arbitrary Local resources in query systems, then we will need to take that in to account.