Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deserialization for VarZeroVec<VarZeroSlice>, take 2 #3649

Merged
merged 3 commits into from
Jul 10, 2023
Merged

Deserialization for VarZeroVec<VarZeroSlice>, take 2 #3649

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19b1e0e
Revert "Support Deserialization for `VarZeroVec<VarZeroSlice>` (#3643)"
Manishearth Jul 7, 2023
228c99c
More succinct deserialize impl for Box<VZS>
Manishearth Jul 7, 2023
bca3867
use better api
Manishearth Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 18 additions & 98 deletions utils/zerovec/src/varzerovec/serde.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,32 +25,6 @@ impl<T: ?Sized, F: VarZeroVecFormat> Default for VarZeroVecVisitor<T, F> {
}
}

struct VarZeroSliceBoxVisitor<T: ?Sized, F: VarZeroVecFormat> {
#[allow(clippy::type_complexity)] // this is a private marker type, who cares
marker: PhantomData<(fn() -> Box<T>, F)>,
}

impl<T: ?Sized, F: VarZeroVecFormat> Default for VarZeroSliceBoxVisitor<T, F> {
fn default() -> Self {
Self {
marker: PhantomData,
}
}
}

struct VarZeroSliceRefVisitor<T: ?Sized, F: VarZeroVecFormat> {
#[allow(clippy::type_complexity)] // this is a private marker type, who cares
marker: PhantomData<(fn() -> Box<T>, F)>,
}

impl<T: ?Sized, F: VarZeroVecFormat> Default for VarZeroSliceRefVisitor<T, F> {
fn default() -> Self {
Self {
marker: PhantomData,
}
}
}

impl<'de, T, F> Visitor<'de> for VarZeroVecVisitor<T, F>
where
T: VarULE + ?Sized,
Expand Down Expand Up @@ -86,62 +60,6 @@ where
}
}

impl<'de, T, F> Visitor<'de> for VarZeroSliceBoxVisitor<T, F>
where
T: VarULE + ?Sized,
Box<T>: Deserialize<'de>,
F: VarZeroVecFormat,
{
type Value = Box<VarZeroSlice<T, F>>;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a sequence or borrowed buffer of bytes")
}

fn visit_borrowed_bytes<E>(self, bytes: &'de [u8]) -> Result<Self::Value, E>
where
E: de::Error,
{
VarZeroSlice::parse_byte_slice(bytes)
.map(VarULE::to_boxed)
.map_err(de::Error::custom)
}

fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: SeqAccess<'de>,
{
let mut vec: Vec<Box<T>> = if let Some(capacity) = seq.size_hint() {
Vec::with_capacity(capacity)
} else {
Vec::new()
};
while let Some(value) = seq.next_element::<Box<T>>()? {
vec.push(value);
}
Ok(VarZeroVec::from(&vec).to_boxed())
}
}

impl<'de, T, F> Visitor<'de> for VarZeroSliceRefVisitor<T, F>
where
T: VarULE + ?Sized,
F: VarZeroVecFormat,
{
type Value = &'de VarZeroSlice<T, F>;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a sequence or borrowed buffer of bytes")
}

fn visit_borrowed_bytes<E>(self, bytes: &'de [u8]) -> Result<Self::Value, E>
where
E: de::Error,
{
VarZeroSlice::parse_byte_slice(bytes).map_err(de::Error::custom)
}
}

/// This impl requires enabling the optional `serde` Cargo feature of the `zerovec` crate
impl<'de, 'a, T, F> Deserialize<'de> for VarZeroVec<'a, T, F>
where
Expand All @@ -154,7 +72,7 @@ where
where
D: Deserializer<'de>,
{
let visitor: VarZeroVecVisitor<T, F> = VarZeroVecVisitor::default();
let visitor = VarZeroVecVisitor::<T, F>::default();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually different code 👀?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, the former still runs some type resolution in picking a deserialize impl. It picked wrong.

if deserializer.is_human_readable() {
deserializer.deserialize_seq(visitor)
} else {
Expand All @@ -167,6 +85,7 @@ where
impl<'de, 'a, T, F> Deserialize<'de> for &'a VarZeroSlice<T, F>
where
T: VarULE + ?Sized,
Box<T>: Deserialize<'de>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use a VarZeroSliceRefVisitor, we can get rid of this bound entirely. Not sure if that's worth an entirely separate Visitor, though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair; I think it[s fine because it ought to be pretty rare to want to deserialize this anyway. It's kinda a code smell to not support human readable deserialization, and the code optimizes away anyway since the choice is known at compile time.

F: VarZeroVecFormat,
'de: 'a,
{
Expand All @@ -179,31 +98,32 @@ where
"&VarZeroSlice cannot be deserialized from human-readable formats",
))
} else {
let visitor: VarZeroSliceRefVisitor<T, F> = VarZeroSliceRefVisitor::default();
let deserialized: &'a VarZeroSlice<_, _> = deserializer.deserialize_bytes(visitor)?;
Ok(deserialized)
let deserialized = VarZeroVec::<'a, T, F>::deserialize(deserializer)?;
let borrowed = if let VarZeroVec::Borrowed(b) = deserialized {
b
} else {
return Err(de::Error::custom(
"&VarZeroSlice can only deserialize in zero-copy ways",
));
};
Ok(borrowed)
}
}
}

/// This impl requires enabling the optional `serde` Cargo feature of the `zerovec` crate
impl<'de, 'a, T, F> Deserialize<'de> for Box<VarZeroSlice<T, F>>
impl<'de, T, F> Deserialize<'de> for Box<VarZeroSlice<T, F>>
where
T: VarULE + ?Sized,
Box<T>: Deserialize<'de>,
F: VarZeroVecFormat,
'de: 'a,
{
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let visitor: VarZeroSliceBoxVisitor<T, F> = VarZeroSliceBoxVisitor::default();
if deserializer.is_human_readable() {
deserializer.deserialize_seq(visitor)
} else {
deserializer.deserialize_bytes(visitor)
}
let deserialized = VarZeroVec::<T, F>::deserialize(deserializer)?;
Ok(deserialized.to_boxed())
}
}

Expand Down Expand Up @@ -257,15 +177,15 @@ mod test {
}

#[derive(serde::Serialize, serde::Deserialize)]
struct DeriveTest_VarZeroVec_of_VarZeroSlice<'data> {
struct DeriveTest_VarZeroSlice<'data> {
#[serde(borrow)]
_data: VarZeroVec<'data, VarZeroSlice<str>>,
_data: &'data VarZeroSlice<str>,
}

#[derive(serde::Serialize, serde::Deserialize)]
struct DeriveTest_VarZeroSlice<'data> {
struct DeriveTest_VarZeroVec_of_VarZeroSlice<'data> {
#[serde(borrow)]
_data: &'data VarZeroSlice<str>,
_data: VarZeroVec<'data, VarZeroSlice<str>>,
}

// ["foo", "bar", "baz", "dolor", "quux", "lorem ipsum"];
Expand Down
Loading