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

Provide access to world storages when using UnsafeWorldCell #8175

Closed
wants to merge 5 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Mar 23, 2023

Objective

UnsafeWorldCell is a type used to provide interior mutable access to a world, meant to be a replacement for &World. However, it is not currently possible to directly access the world's underlying storages, which means UnsafeWorldCell is not enough for every case where you need interior mutable world access.

Solution

Provide access to world storages. Make the function unsafe, requiring the user to only use world accesses allowed by the UnsafeWorldCell.


Changelog

  • Added the function UnsafeWorldCell::storages, which gives low-level access to the underlying data storages of the World.

@jakobhellermann
Copy link
Contributor

I'm still not entirely sure how Storages, SparseSets, Tables work regarding their safety contract.

This function is sound:

struct T;

fn use_storage(storages: &Storages) {
  let component_id = /* component ID that I know is for T */;

  let column = storages.tables.get(table_id).unwrap().get_column(component_id).unwrap();
  let ptr = column.get_data(row).unwrap();


  assert!(ptr is aligned for T):
  // SAFETY: ptr points to T and is aligned.
  let value = unsafe { ptr.deref::<T>() };
}

That means, that having access to Storages gives you implicitly access to all values stored in the tables and sparse sets.

Under that model, fn UnsafeWorldCell::storages must be unsafe, as the caller must ensure that the Storages are only accessed in accordance to the UnsafeWorldCell's permissions.

This means however, that World::storages also needs to be unsafe:

fn use_world(world: &World) {
  world.storages().stable.get... // same as above
}

The alternative would be to declare that having a reference to Storages does not implicitly grant you access to all its contents, and instead make something else unsafe.
Either

  1. table.get_column/sparse_sets.get (Column and SparseSet now implicitly grant access)

  2. column.get/column.get_data, component_sparse_sets.get/component_sparse_sets.get_with_ticks/...

  3. make World::storages unsafe

I think I prefer 1 or 2, because these are at the granularity that bevy actually work on. (can multiple systems access distinct parts of the same Column? If so, I definitely prefer 2.)

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 24, 2023

This means however, that World::storages also needs to be unsafe:

fn use_world(world: &World) {
  world.storages().stable.get... // same as above
}

The alternative would be to declare that having a reference to Storages does not implicitly grant you access to all its contents, and instead make something else unsafe.

I think a better solution would be to enforce the fact that &World is only for immutable access, which would mean removing all public ways to mutate a world via &World. Under this model, World::storages would be removed, and you'd have to use UnsafeWorldCell to do any interior mutation.

To me, this would make &World a lot more consistent. Currently you are able to use &World as a SystemParam, which is meant to provide read-only access to the entire world. However, you're still able to call World::storages on it and mutate it, which just doesn't make sense to me. Granted, it takes unsafe code to do that, but I'd still rather we enforce this at a type-level.

@jakobhellermann
Copy link
Contributor

I agree that &World should only be immutable access.

The question is still where the boundary of unsafety is for storage. Either Storages gives you access to everything inside and fn UnsafeWorldCell::storages is unsafe, or Storages doesn't grant you access yet and every call to get in Column and SparseSet needs to have a safety comment arguing why you have access.

I think the second approch leads to better // SAFETY comments because usually in bevy we don't have the entire Storages, just a specific bit, and there the safety should be documented.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 24, 2023

or Storages doesn't grant you access yet and every call to get in Column and SparseSet needs to have a safety comment arguing why you have access.

I strongly prefer this option. That's the approach I went for in this PR.

@jakobhellermann
Copy link
Contributor

I strongly prefer this option. That's the approach I went for in this PR.

I don't think it is yet.
Once you have a &Storages, you can safely get access to its data.

To prevent that we would have to make Table::get and SparseSet::get unsafe.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 24, 2023

Once you have a &Storages, you can safely get access to its data.

The only way to obtain an &Storages from UnsafeWorldCell is through an unsafe function, and that function explicitly forbids you from accessing any data that you are not allowed to.

@jakobhellermann
Copy link
Contributor

Yeah, I'm not saying it is unsound. Both options are valid.

// Safety: we only access what we're allowed to
let storages = unsafe { worldcell.storages() };
let table = storages.tables.get(id);
let value = table.get(row);

vs

let storages = worldcell.storages();
let table = storages.tables.get(id);
// Safety: we only access what we're allowed to 
let value = unsafe { table.get(row) };

I think usually the latter is a better place to put safety comments, especially when there are multiple layers of functions between.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 24, 2023

Ah, I see what you're saying. Yeah, that is a better way to encapsulate these safety invariants, and this PR isn't quite doing that yet.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 24, 2023

To reduce the impact of that refactor, I'd like to first migrate SystemParam and WorldQuery to use UnsafeWorldCell. Could I get your review on #8174?

@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 1, 2023

Closing in favor of #8280, which implements the cleaner unsafe API discussed before.

@JoJoJet JoJoJet closed this Apr 1, 2023
@JoJoJet JoJoJet deleted the unsafe-world-cell-storages branch April 1, 2023 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants