Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutable access control for components #1635

Closed
alice-i-cecile opened this issue Mar 12, 2021 · 7 comments
Closed

Mutable access control for components #1635

alice-i-cecile opened this issue Mar 12, 2021 · 7 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-User-Error This issue was caused by a mistake in the user's approach

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Right now, if you want to guarantee true type-level invariants on some of your data, you have to stuff it all together in a container type, and then provide public functions that do the relevant mutation while upholding the required invariants.

Splitting up such a monolithic container into smaller types allows more concurrent read access to the data, but also risks mis-using mutable access for these types.

-@JeanMertz on Discord

What solution would you like?

@TheRawMeatball thinks this could be done with derives for components, allowing you to export immutable-only variants of components for use elsewhere in your game.

What alternative(s) have you considered?

Use pub to control mutable access. Unfortunately, this restricts both immutable and mutable access in the same way, preventing us from reading the component elsewhere. This is an important distinction, since read-only access is much less likely to create horrifying spaghetti code.

Additional context

None.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 12, 2021
@Ratysz
Copy link
Contributor

Ratysz commented Mar 13, 2021

I don't think Bevy needs to provide a built-in "solution" for basic encapsulation.

To clarify: this is not a Bevy-specific problem that needs to be addressed in a Bevy-specific way.

@alice-i-cecile
Copy link
Member Author

Is there an existing tool for this in Rust that you would recommend here, or should we be asking about this upstream?

@Ratysz
Copy link
Contributor

Ratysz commented Mar 13, 2021

What exactly are you trying to prevent, systems using mutable borrows when the user wants to use mutable borrows? This is not a footgun, an emergent antipattern, or a caveat: it's a user error. It's not our fault if the user breaks invariants they themselves wrote, and it's not our job to design their APIs for them.

To answer the question: it's encapsulation. In Rust it's done by using the pub keyword, or omitting it, or using it with a modifier, like pub(in module). How exactly the user sets that up is up to them: public/private accessor methods or traits or ZST tokens or whatever else.

"Unfortunately, this restricts both immutable and mutable access in the same way, preventing us from reading the component elsewhere." from the leading post is just embarassignly wrong:

pub struct MyComponent(usize);

impl MyComponent {
    // This can be used by anything that can access `MyComponent`.
    pub fn get(&self) -> usize {
        self.0
    }

    // This can be used by anything in the same crate.
    pub(crate) fn increment(&mut self) {
        *self.0 += 1;
    }

    // This can be used by anything in the parent module.
    pub(super) fn decrement(&mut self) {
        *self.0 -= 1;
    }

    // This can be used only in the same module.
    fn set(&mut self, data: usize) {
        *self.0 = data;
    }
}

@alice-i-cecile
Copy link
Member Author

This was relayed from @jeanmertz on Discord; rather than being my own issue.

I agree with this explanation; I'll link this and close the issue.

@alice-i-cecile alice-i-cecile added the S-User-Error This issue was caused by a mistake in the user's approach label Mar 13, 2021
@JeanMertz
Copy link
Contributor

@Ratysz here's an example I was thinking of:

Say I have a crate grid which exposes (simplified):

/// Position on the grid
struct Position(x,y);

/// Cache store for fast position -> entities lookup
struct Entities {
	positions: HashMap<Position, HashSet<Entitiy>>,
	entities: HashMap<Entity, Position>
}

/// Listen for position changes, update the `Entities` cache.
fn update_entity_position(
	query: Query<(Entity, Position), Changed<Position>>,
	mut entities: ResMut<Entities>,
) { /* ... */ }

/// Plugin that registers the relevant resources/systems
impl Plugin for GridPlugin { /* ... */ }

Now, in a second crate game, we want to be able to do the following:

  1. Register the GridPlugin.
  2. Spawn new entities with Positions.
  3. Update Positions to reposition entities on the grid.
  4. Get quick access to all entities on a specific grid position through Entities.

So far, all of this works as expected.

However, things break down when someone accidentally starts mutating the Entities store, as it is no longer guaranteed to hold a cache that represents the actual state of the grid.

Now, you are correct that I can just not expose mutable access to the inner hash maps outside the grid crate through pub(crate) (etc.). However, I cannot prevent someone from replacing the cache wholesale by swapping it for a new/empty one.

I get that this could (should?) be considered an edge-case that Bevy shouldn't protect against, but this was mostly me musing on Discord on if/how it would look like if there was a way to "lock" certain resources (or components) in place, providing immutable access to whoever wants to use them, but guaranteeing that they stay in place, and that certain invariants are upheld.


As I'm typing this, I realized that one potential "solution" to this would be to have some kind of internal is_valid: bool flag stored inside the resource, that defaults to false, but gets set to true by the grid crate on initialization, and is then checked whenever the system runs to make sure the resource hasn't been swapped out.

Also, as I'm typing the above, I guess (but I haven't tested this yet), that I can just not implement Default and have the initializer of Entities be private as well, making it impossible for anyone outside the crate to actually replace the resource?

Although of course, that still allows someone to remove the resource, breaking the invariants. This is even more of a niche case though, and probably warrants a "throw hands up, panic and abort" response from the grid plugin, as it can no longer function properly.


So yeah, I believe that "use Rust-provided encapsulation" gets you quite far, but doesn't provide 100% invariant control, as people can still remove initialized resources/components when you don't want that happening. It appears I am mostly looking for a "disallow this resource/component from ever being removed (from this entity)" feature. But this is probably too much of a niche concern to program against in Bevy.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Mar 13, 2021

It appears I am mostly looking for a "disallow this resource/component from ever being removed (from this entity)" feature. But this is probably too much of a niche concern to program against in Bevy.

Archetype invariants (#1481) and kinded entities (#1634) will give you this eventually, presuming that we can get a working implementation :)

I suppose, since #1525 made resources stored as components we actually get resource invariants like this nearly for free too, fully solving your use case...

@Ratysz
Copy link
Contributor

Ratysz commented Mar 14, 2021

I mean, if your users are actively antagonistic against your plugin, should it really try to continue providing them with its functions? You have a guard rail and a safety net, if someone polevaults over all of that in an effort to hurt themselves then so be it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-User-Error This issue was caused by a mistake in the user's approach
Projects
None yet
Development

No branches or pull requests

3 participants