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.

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

Implement visit_i128 and visit_u128 for ContentVisitor #2230

Open
appcypher opened this issue Jun 20, 2022 · 2 comments · May be fixed by #2781
Open

Implement visit_i128 and visit_u128 for ContentVisitor #2230

appcypher opened this issue Jun 20, 2022 · 2 comments · May be fixed by #2781

Comments

@appcypher
Copy link

appcypher commented Jun 20, 2022

I'm working with a library that only supports 128-bit integers for its data format. It is a self describing format which means it implements deserialize_any. It mostly works fine with serde out of the box because there is a custom visitor impl that implements the visit_*128 methods.

The issue I have only rears its head when I derive Deserialize on an untagged enum. The debugger drops me in a Content::deserialize, which calls deserializer.__deserialize_content(actually_private::T, visitor) that takes a ContentVisitor.

let visitor = ContentVisitor { value: PhantomData };
deserializer.__deserialize_content(actually_private::T, visitor)

ContentVisitor does not implement visit_i128 and visit_u128. Which I assume is because there is no Content::I128 and Content::U128 variants.

pub enum Content<'de> {
Bool(bool),
U8(u8),
U16(u16),
U32(u32),
U64(u64),
I8(i8),
I16(i16),
I32(i32),
I64(i64),
F32(f32),
F64(f64),
Char(char),
String(String),
Str(&'de str),
ByteBuf(Vec<u8>),
Bytes(&'de [u8]),
None,
Some(Box<Content<'de>>),
Unit,
Newtype(Box<Content<'de>>),
Seq(Vec<Content<'de>>),
Map(Vec<(Content<'de>, Content<'de>)>),
}

So the default visit_*128 methods get called instead causing the error I end up getting.

serde/serde/src/de/mod.rs

Lines 1362 to 1376 in e1c4517

serde_if_integer128! {
/// The input contains a `i128`.
///
/// This method is available only on Rust compiler versions >=1.26. The
/// default implementation fails with a type error.
fn visit_i128<E>(self, v: i128) -> Result<Self::Value, E>
where
E: Error,
{
let mut buf = [0u8; 58];
let mut writer = format::Buf::new(&mut buf);
fmt::Write::write_fmt(&mut writer, format_args!("integer `{}` as i128", v)).unwrap();
Err(Error::invalid_type(Unexpected::Other(writer.as_str()), &self))
}
}

I implemented a quick solution that ought to be less intrusive but it ends up breaking multiple libraries, including serde_json, that use the Unexpected::* enum. In fact, in my case it broke so many libraries that I have decided to take a different approach to the problem.

master...appcypher:appcypher/content-visitor-i128

128-bit integers have been part of stable Rust for years now and with more people working with 128-bit integers, I think it is worth finding a permanent solution to this problem.

@sunny-g
Copy link

sunny-g commented Sep 14, 2022

Any obvious blockers on this? I am running into the same issue with untagged enums, caused by the same underlying missing variants.

In addition (and likely out of scope for this issue, but related) - is there a public API for hand-writing Deserialize implementations for my own untagged enums (mainly because I am trying to deserialize them with a type that implements DeserializeSeed)?

@appetrosyan
Copy link

+1. Just spent a few hours debugging the problem, thanks to untagged enums gobbling up error messages.

Would be happy to provide a fix, if need be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants