Safety

[Imberflur]

Fixes https://github.com/amethyst/specs/issues/647

See API changes section for description.

Checklist

• [ ] I've added tests for all code changes and additions (where applicable)
• [x] I've added a demonstration of the new feature to one or more examples
• [ ] I've updated the book to reflect my changes
• [ ] Usage of new public items is shown in the API docs

Todo

• [x] Add more to PR description
• [x] Fix this https://github.com/amethyst/specs/pull/765#discussion_r1131532958
• [ ] LendJoin uses nougat since I started working on it before GATs stabilized. I kept it because I thought it might be useful to add more iterator combinators (some combinators are currently impossible to implement with actual GATs). However, I think it would probably be prudent to remove this and use regular GATs for simplicity. So I wanted to remove nougat but it turns out that I can't implement for_each without it (without either running into requiring Self: 'static due to for<'_> hrtb or not being able to have lifetimes in the GAT (LendJoin::Type<'next>) that aren't 'next and aren't 'static.
• [x] May want to create a quick 0.19.0 release first? See https://github.com/amethyst/specs/issues/764
• [x] Compare performance to make sure no significant regressions were introduced (and to satisfy my curiosity)
  • [x] Benchmarks (I compared several versions of specs using ecs_bench_suite, there is maybe a 3% increase in add_remove, there was a significant increase (10+%) in deletion times due to shred::MetaTable changes, so I introduce a nightly feature that enables a more efficient implementation using ptr_metadata, fetch heavy code shows improvements due to the use of atomic_refcell in shred instead of the custom atomic cell implementation)
  • [x] Veloren server
• [x] Publish new versions of hibitset and shred to crates.io and use those instead of git dependencies.
• [x] Update changelog

API changes

This is a breaking change.

This PR fixes several soundness issues that I encountered when working on #737. These include:

• The Join implementation for specs::storage::Entries allows creating aliasing &mut references to the underlying storage (i.e. when not using the JoinIter as a lending/streaming iterator).
• The Join implementation and API of specs::storage::RestrictedStorage similarly can create mutable aliasing references to storages as well as to a specific component.
• The Join implementation of Storage allowed DerefFlaggedStorage to create aliasing &mut references to the internal events channel.
• JoinIter::get allows creating aliasing mutable refs, see #647.
• Parallel iteration creates aliasing &mut Join::Value references.
• Under stacked borrows certain Join::get implementations were invalidating previously returned mutable references (e.g. by creating a slice reference to the whole underlying storage).

Many (but not all) of these issues stem from artificially lengthening the lifetime of the &mut Join::Value within Join::get implementations. We now avoid this and leverage alternative mechanisms including interior mutability and lending iteration.

In more detail, we:

• Introduce LendJoin trait or lending (aka streaming) iteration of joined values.
• Make some things like Entries now only implement LendJoin (i.e. remove unsound Join implementations).
• Implement LendJoin for everything that implements Join(manually, not a blanket impl since there are difference in the implementation).
• Move JoinIter::get to JoinLendIter::get.
• Add the RepeatableLendGet trait (which JoinLendIter::get requires). This is to facilitate destructive implementations of LendJoin where getting the same index more than once would be unsound (i.e. literally just the owned implementation of LendJoin for ChangeSet which removes values as it iterates).
• Refactor ParJoin trait to not rely on Join implementation so we have additional flexibility to use different implementations and keep &mut Join::Value in Join::get.
• Make Join an unsafe trait. The new LendJoin trait is also unsafe to implement.
• Storage no longer implements mutable Joins (the non-lending variant) for all UnprotectedStorages. Instead this implementation requires that the storage implements SharedGetMutStorage. The trait which provides a shared_get_mut(&self, id: Index) method. Notice this takes a shared reference. Storages that can soundly implement this wrap their components in UnsafeCell to allow constructing a mutable reference from a shared reference.
• Revamp many pieces of safety documentation.
• Add #![deny(unsafe_op_in_unsafe_fn)] to make it easier to identify and document unsafe operations.

To try to catch any remaining UB, I ran the available tests under Miri . This also identified some issues in dependencies for which I have submitted PRs to fix:

• https://github.com/amethyst/shred/pull/223
• https://github.com/amethyst/shred/pull/226
• https://github.com/amethyst/hibitset/pull/61

(We need to publish new versions of these to crates.io)

This PR adds Miri to the CI.

Additionally, specs exposed a nightly cargo feature that enabled additional APIs using GATs. Since GATs are now stabilized, I bumped the MSRV to be able to eliminate this feature and remove a bunch of cfg complexity.

Also I introduced AccessMut trait which is similar to DerefMut except it requires explicit use. The associated typeUnprotectedStorage::AccessMut<'_> now requires AccessMut instead of DerefMut. This is to faciliate my work in https://github.com/amethyst/specs/pull/737 where I am exploring a flagged storage type that makes generating modification events more explicit. A blanket implementation of AccessMut for anything implementing DerefMut is included.

[Imberflur]

Exciting news! I finally got a chance to update veloren to use this and profile it and I don't see any particular regressions. Might try to post some tracy pictures later.

I think this should be ready to merge soon.

[Imberflur]

Here are some profiling results in veloren, "this trace" is before changes here and "external trace" is after. I focused on two systems. "character_behavior" which has a join over a lot of component types and "phys" which has multiple joins over fewer component types of which some joins are parallel ones. It seems like there are no regressions and potentially a slight improvement (the profiling conditions have room to be more strictly controlled so I would not trust that this improvement is as significant as it appears here).

[Imberflur]

@xMAC94x thanks for taking a look :heart:

I will wait till this weekend in case anyone else is interested in reviewing. If anyone interested needs more time just let me know.

[zesterer]

I just wanted to say: thanks for pushing this all the way to completion :) I would have reviewed, but I've been busy with other things recently. Nice work!