From 93511917c3be355cf2696ca97fd21ff180dc5f47 Mon Sep 17 00:00:00 2001 From: Chris Morgan Date: Thu, 3 Feb 2022 00:40:47 +1100 Subject: [PATCH] Rename UncheckedAnyExt, fix Extend, tweak things MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The *name* UncheckedAnyExt was ending up visible in docs, but it had become an increasingly unpleasant name. “Downcast” is suitable, though, being private, it’s not still not perfect. But there’s no point in making it public, as people generally can’t implement it because of coherence rules (I tried). Plus documentation and style changes. As for Extend, eh, that should ideally be in a different commit, but it’s here now, and I’m the only one working on this code base in general, so I permit myself to be slightly lazy from time to time. Trouble was Downcast should never have had an Any supertrait, as it was grossly misleading, leading to type_id giving `dyn Any`’s TypeId rather than the underlying type’s. --- CHANGELOG.md | 4 +++ src/any.rs | 74 +++++++++++++++++++++++++++++++++++++++------------- src/lib.rs | 35 ++++++++++++++++--------- 3 files changed, 82 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b91e2bf..8dd437e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ Planned once the dust of 1.0.0-beta.1 settles, since 1.0.0-beta.1 ended up being bigger than I’d earlier intended. +# 1.0.0-beta.2 (unreleased) + +- Fixed the broken `Extend` implementation added in 1.0.0-beta.1. + # 1.0.0-beta.1 (2022-01-25) - Removed `anymap::any::Any` in favour of just plain `core::any::Any`, since its diff --git a/src/any.rs b/src/any.rs index 15b535b..636cb92 100644 --- a/src/any.rs +++ b/src/any.rs @@ -1,5 +1,5 @@ use core::fmt; -use core::any::Any; +use core::any::{Any, TypeId}; #[cfg(not(feature = "std"))] use alloc::boxed::Box; @@ -47,23 +47,61 @@ macro_rules! impl_clone { } } -#[allow(missing_docs)] // Bogus warning (it’s not public outside the crate), ☹ -pub trait UncheckedAnyExt: Any { - unsafe fn downcast_ref_unchecked(&self) -> &T; - unsafe fn downcast_mut_unchecked(&mut self) -> &mut T; - unsafe fn downcast_unchecked(self: Box) -> Box; +/// Methods for downcasting from an `Any`-like trait object. +/// +/// This should only be implemented on trait objects for subtraits of `Any`, though you can +/// implement it for other types and it’ll work fine, so long as your implementation is correct. +pub trait Downcast { + /// Gets the `TypeId` of `self`. + fn type_id(&self) -> TypeId; + + // Note the bound through these downcast methods is 'static, rather than the inexpressible + // concept of Self-but-as-a-trait (where Self is `dyn Trait`). This is sufficient, exceeding + // TypeId’s requirements. Sure, you *can* do CloneAny.downcast_unchecked::() and the + // type system won’t protect you, but that doesn’t introduce any unsafety: the method is + // already unsafe because you can specify the wrong type, and if this were exposing safe + // downcasting, CloneAny.downcast::() would just return an error, which is just as + // correct. + // + // Now in theory we could also add T: ?Sized, but that doesn’t play nicely with the common + // implementation, so I’m doing without it. + + /// Downcast from `&Any` to `&T`, without checking the type matches. + /// + /// # Safety + /// + /// The caller must ensure that `T` matches the trait object, on pain of *undefined behaviour*. + unsafe fn downcast_ref_unchecked(&self) -> &T; + + /// Downcast from `&mut Any` to `&mut T`, without checking the type matches. + /// + /// # Safety + /// + /// The caller must ensure that `T` matches the trait object, on pain of *undefined behaviour*. + unsafe fn downcast_mut_unchecked(&mut self) -> &mut T; + + /// Downcast from `Box` to `Box`, without checking the type matches. + /// + /// # Safety + /// + /// The caller must ensure that `T` matches the trait object, on pain of *undefined behaviour*. + unsafe fn downcast_unchecked(self: Box) -> Box; } -#[doc(hidden)] /// A trait for the conversion of an object into a boxed trait object. -pub trait IntoBox: Any { +pub trait IntoBox: Any { /// Convert self into the appropriate boxed form. fn into_box(self) -> Box; } macro_rules! implement { - ($base:ident, $(+ $bounds:ident)*) => { - impl UncheckedAnyExt for dyn $base $(+ $bounds)* { + ($any_trait:ident $(+ $auto_traits:ident)*) => { + impl Downcast for dyn $any_trait $(+ $auto_traits)* { + #[inline] + fn type_id(&self) -> TypeId { + self.type_id() + } + #[inline] unsafe fn downcast_ref_unchecked(&self) -> &T { &*(self as *const Self as *const T) @@ -80,18 +118,18 @@ macro_rules! implement { } } - impl IntoBox for T { + impl IntoBox for T { #[inline] - fn into_box(self) -> Box { + fn into_box(self) -> Box { Box::new(self) } } } } -implement!(Any,); -implement!(Any, + Send); -implement!(Any, + Send + Sync); +implement!(Any); +implement!(Any + Send); +implement!(Any + Send + Sync); /// [`Any`], but with cloning. /// @@ -99,9 +137,9 @@ implement!(Any, + Send + Sync); /// See [`core::any`] for more details on `Any` in general. pub trait CloneAny: Any + CloneToAny { } impl CloneAny for T { } -implement!(CloneAny,); -implement!(CloneAny, + Send); -implement!(CloneAny, + Send + Sync); +implement!(CloneAny); +implement!(CloneAny + Send); +implement!(CloneAny + Send + Sync); impl_clone!(dyn CloneAny); impl_clone!(dyn CloneAny + Send); impl_clone!(dyn CloneAny + Send + Sync); diff --git a/src/lib.rs b/src/lib.rs index 1742701..0fc2941 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -20,7 +20,7 @@ extern crate alloc; #[cfg(not(feature = "std"))] use alloc::boxed::Box; -use any::{UncheckedAnyExt, IntoBox}; +use any::{Downcast, IntoBox}; pub use any::CloneAny; #[cfg(all(feature = "std", not(feature = "hashbrown")))] @@ -108,12 +108,12 @@ pub type RawMap = HashMap, BuildHasherDefault>; /// /// Values containing non-static references are not permitted. #[derive(Debug)] -pub struct Map { +pub struct Map { raw: RawMap, } // #[derive(Clone)] would want A to implement Clone, but in reality it’s only Box that can. -impl Clone for Map where Box: Clone { +impl Clone for Map where Box: Clone { #[inline] fn clone(&self) -> Map { Map { @@ -129,14 +129,14 @@ impl Clone for Map where Box: Clone { /// It’s a bit sad, really. Ah well, I guess this approach will do. pub type AnyMap = Map; -impl Default for Map { +impl Default for Map { #[inline] fn default() -> Map { Map::new() } } -impl Map { +impl Map { /// Create an empty collection. #[inline] pub fn new() -> Map { @@ -336,17 +336,17 @@ impl Map { } } -impl Extend> for Map { +impl Extend> for Map { #[inline] fn extend>>(&mut self, iter: T) { for item in iter { - let _ = self.raw.insert(item.type_id(), item); + let _ = self.raw.insert(Downcast::type_id(&*item), item); } } } /// A view into a single occupied location in an `Map`. -pub struct OccupiedEntry<'a, A: ?Sized + UncheckedAnyExt, V: 'a> { +pub struct OccupiedEntry<'a, A: ?Sized + Downcast, V: 'a> { #[cfg(all(feature = "std", not(feature = "hashbrown")))] inner: raw_hash_map::OccupiedEntry<'a, TypeId, Box>, #[cfg(feature = "hashbrown")] @@ -355,7 +355,7 @@ pub struct OccupiedEntry<'a, A: ?Sized + UncheckedAnyExt, V: 'a> { } /// A view into a single empty location in an `Map`. -pub struct VacantEntry<'a, A: ?Sized + UncheckedAnyExt, V: 'a> { +pub struct VacantEntry<'a, A: ?Sized + Downcast, V: 'a> { #[cfg(all(feature = "std", not(feature = "hashbrown")))] inner: raw_hash_map::VacantEntry<'a, TypeId, Box>, #[cfg(feature = "hashbrown")] @@ -364,14 +364,14 @@ pub struct VacantEntry<'a, A: ?Sized + UncheckedAnyExt, V: 'a> { } /// A view into a single location in an `Map`, which may be vacant or occupied. -pub enum Entry<'a, A: ?Sized + UncheckedAnyExt, V: 'a> { +pub enum Entry<'a, A: ?Sized + Downcast, V: 'a> { /// An occupied Entry Occupied(OccupiedEntry<'a, A, V>), /// A vacant Entry Vacant(VacantEntry<'a, A, V>), } -impl<'a, A: ?Sized + UncheckedAnyExt, V: IntoBox> Entry<'a, A, V> { +impl<'a, A: ?Sized + Downcast, V: IntoBox> Entry<'a, A, V> { /// Ensures a value is in the entry by inserting the default if empty, and returns /// a mutable reference to the value in the entry. #[inline] @@ -421,7 +421,7 @@ impl<'a, A: ?Sized + UncheckedAnyExt, V: IntoBox> Entry<'a, A, V> { // insert_entry(self, value: V) -> OccupiedEntry<'a, K, V> (1.59.0) } -impl<'a, A: ?Sized + UncheckedAnyExt, V: IntoBox> OccupiedEntry<'a, A, V> { +impl<'a, A: ?Sized + Downcast, V: IntoBox> OccupiedEntry<'a, A, V> { /// Gets a reference to the value in the entry #[inline] pub fn get(&self) -> &V { @@ -454,7 +454,7 @@ impl<'a, A: ?Sized + UncheckedAnyExt, V: IntoBox> OccupiedEntry<'a, A, V> { } } -impl<'a, A: ?Sized + UncheckedAnyExt, V: IntoBox> VacantEntry<'a, A, V> { +impl<'a, A: ?Sized + Downcast, V: IntoBox> VacantEntry<'a, A, V> { /// Sets the value of the entry with the VacantEntry's key, /// and returns a mutable reference to it #[inline] @@ -645,4 +645,13 @@ mod tests { verify_hashing_with(TypeId::of::<&str>()); verify_hashing_with(TypeId::of::>()); } + + #[test] + fn test_extend() { + let mut map = AnyMap::new(); + map.extend([Box::new(123) as Box, Box::new(456), Box::new(true)]); + assert_eq!(map.get(), Some(&456)); + assert_eq!(map.get::(), Some(&true)); + assert!(map.get::>().is_none()); + } }