From: Chris Morgan Date: Tue, 25 Jan 2022 03:12:16 +0000 (+1100) Subject: Make TypeIdHasher safe, bump MSRV X-Git-Tag: 1.0.0-beta.1~19 X-Git-Url: https://git.chrismorgan.info/anymap/commitdiff_plain/7866ca8d779805dc3a3134f2d247889a1721633c 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.) --- diff --git a/.travis.yml b/.travis.yml index 0b06902..d6e378b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,7 @@ language: rust rust: + - 1.34.0 - nightly - - beta - stable script: - if [[ "$(rustc --version)" =~ -(dev|nightly) ]]; then cargo bench; fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 72f1270..04ff219 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 won’t run on anything but nightly, but that don’t signify). Technically a [breaking-change], but it was something for diff --git a/Cargo.toml b/Cargo.toml index b9ce2e1..be51bde 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,6 +2,7 @@ name = "anymap" version = "0.12.1" authors = ["Chris Morgan "] +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"] diff --git a/README.md b/README.md index 9c2905c..91471de 100644 --- a/README.md +++ b/README.md @@ -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 it’s 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 it’s 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). -It’s 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. Here’s how you could do it: +It’s 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. Here’s 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 it’d 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 won’t 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`). diff --git a/src/raw.rs b/src/raw.rs index aa3d6dc..0094b34 100644 --- a/src/raw.rs +++ b/src/raw.rs @@ -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 there’s no realistic chance of + // that changing, but I don’t want to depend on something that isn’t expressly part of the + // contract for safety. But I’m 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]