From 245a6e692a8663d6d94dffeb693305d45803919e Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Thu, 16 Nov 2023 03:19:40 -0800 Subject: [PATCH] Remove the protobuf::__runtime::Map type Before this change the runtimes export a Map and MapInner. This change merges the Map and MapInner types into a single MapInner type, removing the Map type from the runtime (upb.rs, cpp.rs). The motivation for this change is twofold: 1) A separate Map type is not strictly needed by the runtime. I hope this reduces some complexity. 2) After this change we can introduce a runtime-agnostic protobuf::Map type that implements Proxied. PiperOrigin-RevId: 582978008 --- rust/cpp.rs | 49 ++++++--------- rust/map.rs | 14 ++--- rust/upb.rs | 62 +++++++++---------- .../protobuf/compiler/rust/accessors/map.cc | 20 ++++-- 4 files changed, 70 insertions(+), 75 deletions(-) diff --git a/rust/cpp.rs b/rust/cpp.rs index 11fd63462c..a0c319d0e2 100644 --- a/rust/cpp.rs +++ b/rust/cpp.rs @@ -320,34 +320,22 @@ impl<'msg, T: RepeatedScalarOps> RepeatedField<'msg, T> { } #[derive(Debug)] -pub struct Map<'msg, K: ?Sized, V: ?Sized> { - inner: MapInner<'msg>, - _phantom_key: PhantomData<&'msg mut K>, - _phantom_value: PhantomData<&'msg mut V>, -} - -#[derive(Clone, Copy, Debug)] -pub struct MapInner<'msg> { +pub struct MapInner<'msg, K: ?Sized, V: ?Sized> { pub raw: RawMap, - pub _phantom: PhantomData<&'msg ()>, + pub _phantom_key: PhantomData<&'msg mut K>, + pub _phantom_value: PhantomData<&'msg mut V>, } // These use manual impls instead of derives to avoid unnecessary bounds on `K` // and `V`. This problem is referred to as "perfect derive". // https://smallcultfollowing.com/babysteps/blog/2022/04/12/implied-bounds-and-perfect-derive/ -impl<'msg, K: ?Sized, V: ?Sized> Copy for Map<'msg, K, V> {} -impl<'msg, K: ?Sized, V: ?Sized> Clone for Map<'msg, K, V> { - fn clone(&self) -> Map<'msg, K, V> { +impl<'msg, K: ?Sized, V: ?Sized> Copy for MapInner<'msg, K, V> {} +impl<'msg, K: ?Sized, V: ?Sized> Clone for MapInner<'msg, K, V> { + fn clone(&self) -> MapInner<'msg, K, V> { *self } } -impl<'msg, K: ?Sized, V: ?Sized> Map<'msg, K, V> { - pub fn from_inner(_private: Private, inner: MapInner<'msg>) -> Self { - Map { inner, _phantom_key: PhantomData, _phantom_value: PhantomData } - } -} - macro_rules! impl_scalar_map_values { ($kt:ty, $trait:ident for $($t:ty),*) => { paste! { $( @@ -419,34 +407,35 @@ macro_rules! impl_scalar_maps { $t, [< MapWith $t:camel KeyOps >] for i32, u32, f32, f64, bool, u64, i64 ); - impl<'msg, V: [< MapWith $t:camel KeyOps >]> Map<'msg, $t, V> { - pub fn new() -> Self { - let inner = MapInner { raw: V::new_map(), _phantom: PhantomData }; - Map { - inner, + impl<'msg, V: [< MapWith $t:camel KeyOps >]> Default for MapInner<'msg, $t, V> { + fn default() -> Self { + MapInner { + raw: V::new_map(), _phantom_key: PhantomData, _phantom_value: PhantomData } } + } + impl<'msg, V: [< MapWith $t:camel KeyOps >]> MapInner<'msg, $t, V> { pub fn size(&self) -> usize { - V::size(self.inner.raw) + V::size(self.raw) } pub fn clear(&mut self) { - V::clear(self.inner.raw) + V::clear(self.raw) } pub fn get(&self, key: $t) -> Option { - V::get(self.inner.raw, key) + V::get(self.raw, key) } pub fn remove(&mut self, key: $t) -> Option { - V::remove(self.inner.raw, key) + V::remove(self.raw, key) } pub fn insert(&mut self, key: $t, value: V) -> bool { - V::insert(self.inner.raw, key, value); + V::insert(self.raw, key, value); true } } @@ -502,7 +491,7 @@ mod tests { #[test] fn i32_i32_map() { - let mut map = Map::<'_, i32, i32>::new(); + let mut map: MapInner<'_, i32, i32> = Default::default(); assert_that!(map.size(), eq(0)); assert_that!(map.insert(1, 2), eq(true)); @@ -522,7 +511,7 @@ mod tests { #[test] fn i64_f64_map() { - let mut map = Map::<'_, i64, f64>::new(); + let mut map: MapInner<'_, i64, f64> = Default::default(); assert_that!(map.size(), eq(0)); assert_that!(map.insert(1, 2.5), eq(true)); diff --git a/rust/map.rs b/rust/map.rs index ab54698ee3..a18ccdc787 100644 --- a/rust/map.rs +++ b/rust/map.rs @@ -8,7 +8,7 @@ use crate::{ __internal::Private, __runtime::{ - Map, MapInner, MapWithBoolKeyOps, MapWithI32KeyOps, MapWithI64KeyOps, MapWithU32KeyOps, + MapInner, MapWithBoolKeyOps, MapWithI32KeyOps, MapWithI64KeyOps, MapWithU32KeyOps, MapWithU64KeyOps, }, }; @@ -17,24 +17,24 @@ use paste::paste; #[derive(Clone, Copy)] #[repr(transparent)] pub struct MapView<'a, K: ?Sized, V: ?Sized> { - inner: Map<'a, K, V>, + inner: MapInner<'a, K, V>, } #[derive(Clone, Copy)] #[repr(transparent)] pub struct MapMut<'a, K: ?Sized, V: ?Sized> { - inner: Map<'a, K, V>, + inner: MapInner<'a, K, V>, } impl<'a, K: ?Sized, V: ?Sized> MapView<'a, K, V> { - pub fn from_inner(_private: Private, inner: MapInner<'a>) -> Self { - Self { inner: Map::<'a, K, V>::from_inner(_private, inner) } + pub fn from_inner(_private: Private, inner: MapInner<'a, K, V>) -> Self { + Self { inner } } } impl<'a, K: ?Sized, V: ?Sized> MapMut<'a, K, V> { - pub fn from_inner(_private: Private, inner: MapInner<'a>) -> Self { - Self { inner: Map::<'a, K, V>::from_inner(_private, inner) } + pub fn from_inner(_private: Private, inner: MapInner<'a, K, V>) -> Self { + Self { inner } } } diff --git a/rust/upb.rs b/rust/upb.rs index 8f713dad69..cb3551d810 100644 --- a/rust/upb.rs +++ b/rust/upb.rs @@ -474,49 +474,43 @@ pub unsafe fn empty_array() -> RepeatedFieldInner<'static> { /// /// TODO: Split MapInner into mut and const variants to /// enforce safety. The returned array must never be mutated. -pub unsafe fn empty_map() -> MapInner<'static> { - fn new_map_inner() -> MapInner<'static> { +pub unsafe fn empty_map() -> MapInner<'static, K, V> { + fn new_map_inner() -> MapInner<'static, i32, i32> { // TODO: Consider creating empty map in C. let arena = Box::leak::<'static>(Box::new(Arena::new())); // Provide `i32` as a placeholder type. - Map::<'static, i32, i32>::new(arena).inner + MapInner::<'static, i32, i32>::new(arena) } thread_local! { - static MAP: MapInner<'static> = new_map_inner(); + static MAP: MapInner<'static, i32, i32> = new_map_inner(); } - MAP.with(|inner| *inner) + MAP.with(|inner| MapInner { + raw: inner.raw, + arena: inner.arena, + _phantom_key: PhantomData, + _phantom_value: PhantomData, + }) } -#[derive(Clone, Copy, Debug)] -pub struct MapInner<'msg> { +#[derive(Debug)] +pub struct MapInner<'msg, K: ?Sized, V: ?Sized> { pub raw: RawMap, pub arena: &'msg Arena, -} - -#[derive(Debug)] -pub struct Map<'msg, K: ?Sized, V: ?Sized> { - inner: MapInner<'msg>, - _phantom_key: PhantomData<&'msg mut K>, - _phantom_value: PhantomData<&'msg mut V>, + pub _phantom_key: PhantomData<&'msg mut K>, + pub _phantom_value: PhantomData<&'msg mut V>, } // These use manual impls instead of derives to avoid unnecessary bounds on `K` // and `V`. This problem is referred to as "perfect derive". // https://smallcultfollowing.com/babysteps/blog/2022/04/12/implied-bounds-and-perfect-derive/ -impl<'msg, K: ?Sized, V: ?Sized> Copy for Map<'msg, K, V> {} -impl<'msg, K: ?Sized, V: ?Sized> Clone for Map<'msg, K, V> { - fn clone(&self) -> Map<'msg, K, V> { +impl<'msg, K: ?Sized, V: ?Sized> Copy for MapInner<'msg, K, V> {} +impl<'msg, K: ?Sized, V: ?Sized> Clone for MapInner<'msg, K, V> { + fn clone(&self) -> MapInner<'msg, K, V> { *self } } -impl<'msg, K: ?Sized, V: ?Sized> Map<'msg, K, V> { - pub fn from_inner(_private: Private, inner: MapInner<'msg>) -> Self { - Map { inner, _phantom_key: PhantomData, _phantom_value: PhantomData } - } -} - macro_rules! impl_scalar_map_for_key_type { ($key_t:ty, $key_ufield:ident, $key_upb_tag:expr, $trait:ident for $($t:ty, $ufield:ident, $upb_tag:expr, $zero_val:literal;)*) => { paste! { $( @@ -596,34 +590,34 @@ macro_rules! impl_scalar_map_for_key_types { bool, bool_val, UpbCType::Bool, false; ); - impl<'msg, V: [< MapWith $t:camel KeyOps >]> Map<'msg, $t, V> { + impl<'msg, V: [< MapWith $t:camel KeyOps >]> MapInner<'msg, $t, V> { pub fn new(arena: &'msg mut Arena) -> Self { - let inner = MapInner { raw: V::new_map(arena.raw()), arena }; - Map { - inner, + MapInner { + raw: V::new_map(arena.raw()), + arena, _phantom_key: PhantomData, _phantom_value: PhantomData } } pub fn size(&self) -> usize { - V::size(self.inner.raw) + V::size(self.raw) } pub fn clear(&mut self) { - V::clear(self.inner.raw) + V::clear(self.raw) } pub fn get(&self, key: $t) -> Option { - V::get(self.inner.raw, key) + V::get(self.raw, key) } pub fn remove(&mut self, key: $t) -> Option { - V::remove(self.inner.raw, key) + V::remove(self.raw, key) } pub fn insert(&mut self, key: $t, value: V) -> bool { - V::insert(self.inner.raw, self.inner.arena.raw(), key, value) + V::insert(self.raw, self.arena.raw(), key, value) } } )* } @@ -717,7 +711,7 @@ mod tests { #[test] fn i32_i32_map() { let mut arena = Arena::new(); - let mut map = Map::<'_, i32, i32>::new(&mut arena); + let mut map = MapInner::<'_, i32, i32>::new(&mut arena); assert_that!(map.size(), eq(0)); assert_that!(map.insert(1, 2), eq(true)); @@ -738,7 +732,7 @@ mod tests { #[test] fn i64_f64_map() { let mut arena = Arena::new(); - let mut map = Map::<'_, i64, f64>::new(&mut arena); + let mut map = MapInner::<'_, i64, f64>::new(&mut arena); assert_that!(map.size(), eq(0)); assert_that!(map.insert(1, 2.5), eq(true)); diff --git a/src/google/protobuf/compiler/rust/accessors/map.cc b/src/google/protobuf/compiler/rust/accessors/map.cc index 1df6b23c42..b398df17c4 100644 --- a/src/google/protobuf/compiler/rust/accessors/map.cc +++ b/src/google/protobuf/compiler/rust/accessors/map.cc @@ -34,7 +34,12 @@ void Map::InMsgImpl(Context field) const { let inner = unsafe { $getter_thunk$(self.inner.msg) }.map_or_else(|| unsafe {$pbr$::empty_map()}, |raw| { - $pbr$::MapInner{ raw, arena: &self.inner.arena } + $pbr$::MapInner{ + raw, + arena: &self.inner.arena, + _phantom_key: std::marker::PhantomData, + _phantom_value: std::marker::PhantomData, + } }); $pb$::MapView::from_inner($pbi$::Private, inner) })rs"); @@ -43,7 +48,8 @@ void Map::InMsgImpl(Context field) const { pub fn r#$field$(&self) -> $pb$::MapView<'_, $Key$, $Value$> { let inner = $pbr$::MapInner { raw: unsafe { $getter_thunk$(self.inner.msg) }, - _phantom: std::marker::PhantomData + _phantom_key: std::marker::PhantomData, + _phantom_value: std::marker::PhantomData, }; $pb$::MapView::from_inner($pbi$::Private, inner) })rs"); @@ -57,7 +63,12 @@ void Map::InMsgImpl(Context field) const { let raw = unsafe { $getter_mut_thunk$(self.inner.msg, self.inner.arena.raw()) }; - let inner = $pbr$::MapInner{ raw, arena: &self.inner.arena }; + let inner = $pbr$::MapInner{ + raw, + arena: &self.inner.arena, + _phantom_key: std::marker::PhantomData, + _phantom_value: std::marker::PhantomData, + }; $pb$::MapMut::from_inner($pbi$::Private, inner) })rs"); } else { @@ -65,7 +76,8 @@ void Map::InMsgImpl(Context field) const { pub fn r#$field$_mut(&mut self) -> $pb$::MapMut<'_, $Key$, $Value$> { let inner = $pbr$::MapInner { raw: unsafe { $getter_mut_thunk$(self.inner.msg) }, - _phantom: std::marker::PhantomData + _phantom_key: std::marker::PhantomData, + _phantom_value: std::marker::PhantomData, }; $pb$::MapMut::from_inner($pbi$::Private, inner) })rs");