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

Safety #765

Merged
merged 47 commits into from
Sep 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
2f331ad
Switch UnprotectedStorage::get_mut to accept &self
Imberflur Jul 30, 2022
8366ddb
Update safety comments on uses of `UnprotectedStorage::get_mut` and c…
Imberflur Jul 31, 2022
ba8b976
Change GAT where clause location to match new formatting recommendation
Imberflur Jul 31, 2022
ac2146d
Update DerefFlaggedStorage to account for changes, get_mut impl postp…
Imberflur Jul 31, 2022
bf620e1
Refactor `UnprotectedStorage` and add `SharedGetAccessMut` trait
Imberflur Jul 31, 2022
e7d0aa1
Refactor `Join` family of traits so they can be more safely implement…
Imberflur Aug 13, 2022
cc97588
Update Join/ParJoin/Unprotected implementations to match changes in t…
Imberflur Aug 17, 2022
483914b
Implement SharedGetAccessMutStorage for applicable storages and updat…
Imberflur Aug 19, 2022
57e929c
Remove `get_mut`, rename `get_access_mut` -> `get_mut`, rename
Imberflur Aug 19, 2022
0968089
Various next steps:
Imberflur Jan 22, 2023
25b27e5
Modify LendJoin to support ChangeSet<T> impl of LendJoin with non 'st…
Imberflur Jan 22, 2023
448c9de
Implement LendJoin for owned and referenced ChangeSet<T>
Imberflur Jan 22, 2023
919434b
Changes to allow soundly implementing Join/LendJoin for the owned Cha…
Imberflur Jan 24, 2023
d2d05f2
Updated safety comments for impls/uses of Join (see previous commit for
Imberflur Jan 24, 2023
ce90ce7
Address a few compile errors that snuck through when enabling additional
Imberflur Feb 3, 2023
bea1ea9
Add lend_join example and make Miri run it without errors by fixing a…
Imberflur Feb 5, 2023
e22d22a
Remove "nightly" feature now that generic associated types have stabi…
Imberflur Feb 5, 2023
738dc16
Uncomment `entry` module and rework implementation:
Imberflur Feb 6, 2023
e4c3655
Implement refactored Join traits for `RestrictedStorage` and other re…
Imberflur Feb 13, 2023
6ca2338
Update implementations of ParJoin and callers of ParJoin::get to refl…
Imberflur Feb 13, 2023
0fad41d
Uncomment `drain` module, update safety comments and implement `LendJ…
Imberflur Feb 13, 2023
2c7d5e6
Fix errors and warnings in tests and examples
Imberflur Feb 13, 2023
c33cd7c
Add missing documentation and fix some doc tests
Imberflur Feb 26, 2023
09f1a4c
Adjust for changes in shred MetaTable
Imberflur Feb 26, 2023
c3a301f
Reduce loop iterations on some tests when running with Miri so that t…
Imberflur Feb 26, 2023
e8f80e7
Appease clippy
Imberflur Feb 26, 2023
391b310
Small update to tutorial to reflect method name change
Imberflur Feb 28, 2023
6b74393
Enhance LendJoin docs to hopefully explain its purpose and usage
Imberflur Feb 28, 2023
5aa5abf
Add test for when Send is implemented for PairedStorageWriteShared
Imberflur Feb 28, 2023
2beb089
Switch to git deps from local path deps for shred and hibitset
Imberflur Feb 28, 2023
cbfa283
Order example entries by name
Imberflur Mar 1, 2023
819d6ca
Tweak rustfmt config
Imberflur Mar 1, 2023
2400172
fmt
Imberflur Mar 1, 2023
6736f15
Fix and simply insert code unwinding handling since allocation is not…
Imberflur Jul 23, 2023
a31e781
Add nightly feature to enable shred/nightly for more efficient MetaTa…
Imberflur Jul 23, 2023
1fc3d1f
Fix typo in docs
Imberflur Jul 24, 2023
5abd46a
Fix various compilation warnings, change abort on unwinding during in…
Imberflur Jul 25, 2023
de30c85
Remove apparently unnecessary "where Self: 'next" on LendJoin::get
Imberflur Jul 26, 2023
35da879
Update MSRV to 1.70 since that is apparently necessary to remove "whe…
Imberflur Jul 26, 2023
a8c96f8
Collapse unnecessary lifetimes in restrict storages
Imberflur Jul 26, 2023
784c7b0
Address todo in deref_flagged impl
Imberflur Sep 15, 2023
5d3bfe7
Switch to published versions of hibitset and shred
Imberflur Sep 16, 2023
a53d28e
Remove rustfmt bug todo since things seem to be working now
Imberflur Sep 16, 2023
0a4b99a
Remove temporary continue-on-error setting
Imberflur Sep 16, 2023
6d4724b
Run no_parallel tests under miri
Imberflur Sep 16, 2023
eecf83a
Update changelog
Imberflur Sep 16, 2023
276450e
Remove unnecessary borrow calls on bitset reference in restricted sto…
Imberflur Sep 16, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .config/nextest.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[profile.default-miri]
slow-timeout = { period = "30s", terminate-after = 1 }
fail-fast = false
19 changes: 17 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ jobs:
fail-fast: true
matrix:
os: [macos-latest, windows-latest, ubuntu-latest]
toolchain: [stable, beta, nightly, 1.65.0]
toolchain: [stable, beta, nightly, 1.70.0]
steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v3

# install the toolchain we are going to compile and test with
- name: install ${{ matrix.toolchain }} toolchain
Expand Down Expand Up @@ -77,3 +77,18 @@ jobs:

# - run: mdbook test -L ./target/debug/deps docs/book
# if: matrix.toolchain == 'stable' && matrix.os == 'ubuntu-latest'

miri:
name: "Miri"
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Install Miri
run: |
rustup toolchain install nightly --component miri
rustup override set nightly
cargo miri setup
- name: Install latest nextest release
uses: taiki-e/install-action@nextest
- name: Test with Miri
run: ./miri.sh
Comment on lines +81 to +94
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI now includes running miri.

3 changes: 2 additions & 1 deletion .github/workflows/deny.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ jobs:
# Prevent sudden announcement of a new advisory from failing ci:
continue-on-error: ${{ matrix.checks == 'advisories' }}


steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v3
- uses: EmbarkStudios/cargo-deny-action@v1
with:
command: check ${{ matrix.checks }}
3 changes: 1 addition & 2 deletions .rustfmt.toml
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
hard_tabs = false
imports_granularity = "Crate"
reorder_impl_items = true
use_field_init_shorthand = true
use_try_shorthand = true
format_code_in_doc_comments = true
wrap_comments = true
edition = "2018"
edition = "2021"
version = "Two"
14 changes: 12 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,13 +1,23 @@
# Unreleased

* MSRV to 1.70.0 ([#765])
* Significant refactor of `Join` and related traits to alleviate soundness
issues. Includes introduction of a lending/streaming join via the `LendJoin`
trait which is the new common denominator implemented by joinable types.
([#765])

[#765]: https://github.com/amethyst/specs/pull/765

# 0.19.0 (2023-06-10)

* Bump MSRV to 1.65.0 ([#766])
* Added index where entity deletion stopped to the error returned from `WorldExt::delete_entities` ([#766])
* Fix bug where deleting an entity with the wrong generation could clear the components of an existing entity. ([#766])
* Bump shred to version `0.14.1`, MSRV to 1.60.0 ([shred changelog][shred-changelog], [#756])
* Bump shred to version `0.14.1`, MSRV to 1.60.0 ([shred changelog][shred_changelog], [#756])

[shred_changelog]: https://github.com/amethyst/shred/blob/master/CHANGELOG.md#0141-2022-07-14
[#756]: https://github.com/amethyst/specs/pull/756
[#766]: https://github.com/amethyst/specs/pull/766
[shred-changelog]: https://github.com/amethyst/shred/blob/6b754812e304cf6c63ba0364a82a7e0e5025aaa4/CHANGELOG.md#0140-2022-07-12

# 0.18.0 (2022-07-02)

Expand Down
30 changes: 16 additions & 14 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,20 @@ license = "MIT OR Apache-2.0"
authors = ["slide-rs hackers"]
include = ["/src", "/examples", "/benches", "/README.md", "/LICENSE-MIT", "/LICENSE-APACHE"]
edition = "2021"
rust-version = "1.65.0"
rust-version = "1.70.0"

# the `storage_cmp` and `storage_sparse` benches are called from `benches_main`
autobenches = false

[dependencies]
ahash = "0.7.6"
crossbeam-queue = "0.3"
hibitset = { version = "0.6.3", default-features = false }
hibitset = { version = "0.6.4", default-features = false }
log = "0.4.8"
shred = { version = "0.14.1", default-features = false }
shred = { version = "0.15.0", default-features = false }
shrev = "1.1.1"
tuple_utils = "0.4.0"
nougat = "0.2.3"

rayon = { version = "1.5.1", optional = true }
serde = { version = "1.0.104", optional = true, features = ["serde_derive"] }
Expand All @@ -40,7 +41,7 @@ uuid_entity = ["uuid", "serde"]
stdweb = ["uuid/js"]
storage-event-control = []
derive = ["shred-derive", "specs-derive"]
nightly = []
nightly = ["shred/nightly"]

shred-derive = ["shred/shred-derive"]

Expand All @@ -53,30 +54,31 @@ criterion = "0.3.1"
ron = "0.7.1"
rand = "0.8"
serde_json = "1.0.48"
shred = { version = "0.14.1", default-features = false, features = ["shred-derive"] }
shred = { version = "0.15.0", default-features = false, features = ["shred-derive"] }
specs-derive = { path = "specs-derive", version = "0.4.1" }

[[example]]
name = "async"
[[example]]
name = "basic"

[[example]]
name = "full"

name = "bitset"
[[example]]
name = "cluster_bomb"

[[example]]
name = "bitset"

name = "full"
[[example]]
name = "track"

name = "lend_join"
test = true
[[example]]
name = "ordered_track"

[[example]]
name = "saveload"
required-features = ["serde"]
[[example]]
name = "slices"
[[example]]
name = "track"

[[bench]]
name = "benches_main"
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Unlike most other ECS libraries out there, it provides
other and you can use barriers to force several stages in system execution
* high performance for real-world applications

Minimum Rust version: 1.65
Minimum Rust version: 1.70

## [Link to the book][book]

Expand Down
4 changes: 4 additions & 0 deletions benches/parallel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ impl Component for Lifetime {
}

#[derive(Clone, Copy, Debug)]
#[allow(dead_code)]
struct Ball {
radius: f32,
}
Expand All @@ -64,6 +65,7 @@ impl Component for Ball {
}

#[derive(Clone, Copy, Debug)]
#[allow(dead_code)]
struct Rect {
a: f32,
b: f32,
Expand Down Expand Up @@ -91,6 +93,7 @@ impl Component for SpawnRequests {
}

#[derive(Clone, Copy, Debug)]
#[allow(dead_code)]
struct Collision {
a: Entity,
b: Entity,
Expand All @@ -102,6 +105,7 @@ impl Component for Collision {
}

#[derive(Clone, Copy, Debug)]
#[allow(dead_code)]
struct Room {
inner_width: f32,
inner_height: f32,
Expand Down
6 changes: 3 additions & 3 deletions docs/tutorials/src/12_tracked.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ fetch the component as mutable if/when needed.
for (entity, mut comp) in (&entities, &mut comps.restrict_mut()).join() {
// Check whether this component should be modified, without fetching it as
// mutable.
if comp.get_unchecked().condition < 5 {
let mut comp = comp.get_mut_unchecked();
if comp.get().condition < 5 {
let mut comp = comp.get_mut();
Comment on lines +120 to +121
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed get_unchecked -> get on the paired storage types since it could imply that an important check is missing, but it is just that this gets the component value for the current entity in a join so it doesn't need to check that it is present in the storage. Paired storages also allow getting the component for a different entity than the current one, that was renamed to get_other.

Copy link

@cpetig cpetig Sep 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _unchecked suffix always reminds me of unsafe access (e.g. https://doc.rust-lang.org/std/primitive.slice.html#method.get_unchecked ), so this rename is good.

Update: I later realized that get indeed remains unsafe, and the restriction that the item must exists sounds very similar to the unchecked variant of slice. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I later realized that get indeed remains unsafe, and the restriction that the item must exists sounds very similar to the unchecked variant of slice.

The method discussed here has always been safe. There are other unsafe methods with the same name that this PR touches, so maybe you are thinking of one of those.

// ...
}
}
Expand All @@ -137,4 +137,4 @@ simply call `storage.set_event_emission(true)`.

_See
[FlaggedStorage Doc](https://docs.rs/specs/latest/specs/struct.FlaggedStorage.html)
for more into._
for more into._
52 changes: 52 additions & 0 deletions examples/lend_join.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use specs::prelude::*;
struct Pos(f32);

impl Component for Pos {
type Storage = VecStorage<Self>;
}

fn main() {
let mut world = World::new();

world.register::<Pos>();

let entity0 = world.create_entity().with(Pos(0.0)).build();
world.create_entity().with(Pos(1.6)).build();
world.create_entity().with(Pos(5.4)).build();

let mut pos = world.write_storage::<Pos>();
let entities = world.entities();

// Unlike `join` the type return from `lend_join` does not implement
// `Iterator`. Instead, a `next` method is provided that only allows one
// element to be accessed at once.
let mut lending = (&mut pos).lend_join();

// We copy the value out here so the borrow of `lending` is released.
let a = lending.next().unwrap().0;
// Here we keep the reference from `lending.next()` alive, so `lending`
// remains exclusively borrowed for the lifetime of `b`.
let b = lending.next().unwrap();
// This right fails to compile since `b` is used below:
// let d = lending.next().unwrap();
b.0 = a;

// Items can be iterated with `while let` loop:
let mut lending = (&mut pos).lend_join();
while let Some(pos) = lending.next() {
pos.0 *= 1.5;
}

// A `for_each` method is also available:
(&mut pos).lend_join().for_each(|pos| {
pos.0 += 1.0;
});

// Finally, there is one bonus feature which `.join()` can't soundly provide.
let mut lending = (&mut pos).lend_join();
// That is, there is a method to get the joined result for a particular
// entity:
if let Some(pos) = lending.get(entity0, &entities) {
pos.0 += 5.0;
}
}
2 changes: 1 addition & 1 deletion examples/slices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ impl<'a> System<'a> for SysA {
// Both the `Pos` and `Vel` components use `DefaultVecStorage`, which supports
// `as_slice()` and `as_mut_slice()`. This lets us access components without
// indirection.
let mut pos_slice = pos.as_mut_slice();
let pos_slice = pos.as_mut_slice();
let vel_slice = vel.as_slice();

// Note that an entity which has position but not velocity will still have
Expand Down
2 changes: 1 addition & 1 deletion examples/track.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ impl<'a> System<'a> for SysB {
fn run(&mut self, (entities, mut tracked): Self::SystemData) {
for (entity, mut restricted) in (&entities, &mut tracked.restrict_mut()).join() {
if entity.id() % 2 == 0 {
let mut comp = restricted.get_mut_unchecked();
let comp = restricted.get_mut();
comp.0 += 1;
}
}
Expand Down
31 changes: 31 additions & 0 deletions miri.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/bin/bash
#
# Convenience script for running Miri, also the same one that the CI runs!

set -e

# use half the available threads since miri can be a bit memory hungry
test_threads=$((($(nproc) - 1) / 2 + 1))
echo using $test_threads threads

# filters out long running tests
filter='not (test(100k) | test(map_test::wrap) | test(map_test::insert_same_key) | test(=mixed_create_merge)| test(=par_join_many_entities_and_systems) | test(=stillborn_entities))'
echo "using filter: \"$filter\""

# Miri currently reports leaks in some tests so we disable that check
# here (might be due to ptr-int-ptr in crossbeam-epoch so might be
# resolved in future versions of that crate).
MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-ignore-leaks" \
cargo +nightly miri nextest run \
-E "$filter" \
--test-threads="$test_threads" \
# use nocapture or run miri directly to see warnings from miri
#--nocapture

# Run tests only available when parallel feature is disabled.
MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-ignore-leaks" \
cargo +nightly miri nextest run \
--no-default-features \
-E "binary(no_parallel)" \
--test-threads="$test_threads"

Loading
Loading