Make TypeIdHasher safe, bump MSRV

Wait a few years and nice things stabilise!

• u64::from_ne_bytes([u8; 8]) is stable in 1.32.0
• TryFrom<&[u8]> for [u8; 8] is stable in 1.34.0

(There are other things I’m touching today that also require a more mild
MSRV bump, but this is the most I *need* at this time.)
This commit is contained in:
Chris Morgan 2022-01-25 14:12:16 +11:00
parent 521fbfe6bc
commit 7866ca8d77
5 changed files with 16 additions and 12 deletions

View file

@ -1,7 +1,7 @@
language: rust
rust:
- 1.34.0
- nightly
- beta
- stable
script:
- if [[ "$(rustc --version)" =~ -(dev|nightly) ]]; then cargo bench; fi

View file

@ -2,6 +2,8 @@
- Relicensed from MIT/Apache-2.0 to BlueOak-1.0.0/MIT/Apache-2.0.
- Increased the minimum supported version of Rust from 1.7.0 to 1.34.0.
- Remove `bench` Cargo feature (by shifting benchmarks out of `src/lib.rs` into
`benches/bench.rs`; it still wont run on anything but nightly, but that
dont signify). Technically a [breaking-change], but it was something for

View file

@ -2,6 +2,7 @@
name = "anymap"
version = "0.12.1"
authors = ["Chris Morgan <rust@chrismorgan.info>"]
rust-version = "1.34"
description = "A safe and convenient store for one value of each type"
repository = "https://github.com/chris-morgan/anymap"
keywords = ["container", "any", "map"]

View file

@ -16,18 +16,17 @@ This library uses a fair bit of unsafe code for several reasons:
- To support Any and CloneAny, unsafe code is required (because of how the `downcast` methods are defined in `impl dyn Any` rather than being trait methods; I think this is kind of a historical detail of the structure of `std::any::Any`); if you wanted to ditch `Clone` support this unsafety could be removed.
- In the interests of performance, skipping various checks that are unnecessary because of the invariants of the data structure (no need to check the type ID when its been statically ensured by being used as the hash map key) and simplifying hashing (type IDs are already good hashes, no need to mangle them through SipHash).
- In the interests of performance, skipping various checks that are unnecessary because of the invariants of the data structure (no need to check the type ID when its been statically ensured by being used as the hash map key).
- In the `Clone` implementation of `dyn CloneAny` with `Send` and/or `Sync` auto traits added, an unsafe block is used where safe code used to be used in order to avoid a [spurious future-compatibility lint](https://github.com/rust-lang/rust/issues/51443#issuecomment-421988013).
Its not possible to remove all unsafety from this library without also removing some of the functionality. Still, at the cost of the `CloneAny` functionality, the raw interface and maybe the concurrency support, you can definitely remove all unsafe code. Heres how you could do it:
Its not possible to remove all unsafety from this library without also removing some of the functionality. Still, at the cost of the `CloneAny` functionality and the raw interface, you can definitely remove all unsafe code. Heres how you could do it:
- Remove the genericness of it all;
- Merge `anymap::raw` into the normal interface, flattening it;
- Change things like `.map(|any| unsafe { any.downcast_unchecked() })` to `.and_then(|any| any.downcast())` (performance cost: one extra superfluous type ID comparison, indirect);
- Ditch the `TypeIdHasher` since transmuting a `TypeId` is right out (cost: SIP mangling of a u64 on every access).
- Change things like `.map(|any| unsafe { any.downcast_unchecked() })` to `.and_then(|any| any.downcast())` (performance cost: one extra superfluous type ID comparison, indirect).
Yeah, the performance costs of going safe are quite small. The more serious matters are the loss of `Clone` and maybe `Send + Sync`.
Yeah, the performance costs of going safe are quite small. The more serious matter is the loss of `Clone`.
But frankly, if you wanted to do all this itd be easier and faster to write it from scratch. The core of the library is actually really simple and perfectly safe, as can be seen in [`src/lib.rs` in the first commit](https://github.com/chris-morgan/anymap/tree/a294948f57dee47bb284d6a3ae1b8f61a902a03c/src/lib.rs) (note that that code wont run without a few syntactic alterations; it was from well before Rust 1.0 and has things like `Any:'static` where now we have `Any + 'static`).

View file

@ -5,12 +5,12 @@
use std::any::TypeId;
use std::borrow::Borrow;
use std::collections::hash_map::{self, HashMap};
use std::convert::TryInto;
use std::hash::Hash;
use std::hash::{Hasher, BuildHasherDefault};
#[cfg(test)]
use std::mem;
use std::ops::{Index, IndexMut};
use std::ptr;
use any::{Any, UncheckedAnyExt};
@ -22,11 +22,13 @@ struct TypeIdHasher {
impl Hasher for TypeIdHasher {
#[inline]
fn write(&mut self, bytes: &[u8]) {
// This expects to receive one and exactly one 64-bit value
debug_assert!(bytes.len() == 8);
unsafe {
ptr::copy_nonoverlapping(&bytes[0] as *const u8 as *const u64, &mut self.value, 1)
}
// This expects to receive exactly one 64-bit value, and theres no realistic chance of
// that changing, but I dont want to depend on something that isnt expressly part of the
// contract for safety. But Im OK with release builds putting everything in one bucket
// if it *did* change (and debug builds panicking).
debug_assert_eq!(bytes.len(), 8);
let _ = bytes.try_into()
.map(|array| self.value = u64::from_ne_bytes(array));
}
#[inline]