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

Allow f32 and f64 map keys #1027

Merged
merged 11 commits into from Jul 11, 2023
Merged

Allow f32 and f64 map keys #1027

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0503111
Allow f32 and f64 keys
overdrivenpotato Jun 17, 2023
f74b422
Return error on non-finite float keys
overdrivenpotato Jul 9, 2023
9963405
Print map float keys with `CompactFormatter`
overdrivenpotato Jul 9, 2023
22a5e9b
Deserialize float keys using `deserialize_number`
overdrivenpotato Jul 9, 2023
324c5b5
Fix formatting
overdrivenpotato Jul 9, 2023
18e1b59
Fix builds without `std` feature
overdrivenpotato Jul 9, 2023
5762701
Use `ryu` directly when formatting float keys
overdrivenpotato Jul 9, 2023
7760a1a
Remove extra variable
overdrivenpotato Jul 9, 2023
b0dbac7
Parse `Value::Map` numeric keys using `crate::from_str`
overdrivenpotato Jul 9, 2023
468a94a
Use Deserializer::* methods instead of `.parse()` for integer keys
overdrivenpotato Jul 9, 2023
a4e2719
Return error if numeric key starts with whitespace
overdrivenpotato Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 15 additions & 13 deletions src/de.rs
View file
Open in desktop
Expand Up @@ -2118,7 +2118,7 @@ struct MapKey<'a, R: 'a> {
de: &'a mut Deserializer<R>,
}

macro_rules! deserialize_integer_key {
macro_rules! deserialize_numeric_key {
($method:ident => $visit:ident) => {
fn $method<V>(self, visitor: V) -> Result<V::Value>
where
Expand Down Expand Up @@ -2155,16 +2155,18 @@ where
}
}

deserialize_integer_key!(deserialize_i8 => visit_i8);
deserialize_integer_key!(deserialize_i16 => visit_i16);
deserialize_integer_key!(deserialize_i32 => visit_i32);
deserialize_integer_key!(deserialize_i64 => visit_i64);
deserialize_integer_key!(deserialize_i128 => visit_i128);
deserialize_integer_key!(deserialize_u8 => visit_u8);
deserialize_integer_key!(deserialize_u16 => visit_u16);
deserialize_integer_key!(deserialize_u32 => visit_u32);
deserialize_integer_key!(deserialize_u64 => visit_u64);
deserialize_integer_key!(deserialize_u128 => visit_u128);
deserialize_numeric_key!(deserialize_i8 => visit_i8);
deserialize_numeric_key!(deserialize_i16 => visit_i16);
deserialize_numeric_key!(deserialize_i32 => visit_i32);
deserialize_numeric_key!(deserialize_i64 => visit_i64);
deserialize_numeric_key!(deserialize_i128 => visit_i128);
deserialize_numeric_key!(deserialize_u8 => visit_u8);
deserialize_numeric_key!(deserialize_u16 => visit_u16);
deserialize_numeric_key!(deserialize_u32 => visit_u32);
deserialize_numeric_key!(deserialize_u64 => visit_u64);
deserialize_numeric_key!(deserialize_u128 => visit_u128);
deserialize_numeric_key!(deserialize_f32 => visit_f32);
deserialize_numeric_key!(deserialize_f64 => visit_f64);
Copy link
Member

Choose a reason for hiding this comment

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

.parse() is not appropriate to use here for floats. That accepts a bunch of syntax that is not considered a valid float in json. For example .0 or 00. Please make sure this accepts only representations that would be valid f32/f64 outside of a map key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this to delegate to the parent Deserializer via self.de.method(...). This unfortunately makes the error messages a bit worse, as we now get something like:

expected value at line 1 column 3

Instead of:

"invalid type: string \"x\", expected f32 at line 1 column 4"

I noticed a similar issue here is present with integer keys, this example works on serde_json master:

extern crate serde_json;
extern crate serde;

fn main() {
    dbg!(serde_json::from_str::<std::collections::HashMap<i32, i32>>(r#"{"00":0}"#));
}

Is it worth updating this as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good call. I filed #1034 to follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an effort to patch #1034, I've also updated integer key parsing to use Deserializer::*. This still exhibits the above error degredation.

Would it make sense here to parse the key into an intermediate string buffer, and then deserialize the integer value using crate::from_str? This would result in the original error messages for unexpected string keys, but may have a performance impact.

Copy link
Member

Choose a reason for hiding this comment

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

This needs a check for leading whitespace, otherwise it would end up accepting potentially invalid JSON syntax.

For example this is not a legal JSON document:

{"
1":null}
// [dependencies]
// ordered-float = { version = "3", features = ["serde"] }
// serde_json = "1"

use ordered_float::NotNan;
use std::collections::BTreeMap;

fn main() {
    let invalid_json = r#"
        {"
        1":null}
    "#;

    let map: BTreeMap<NotNan<f64>, ()> = serde_json::from_str(invalid_json).unwrap();
    println!("{:?}", map);
}

Copy link
Member

Choose a reason for hiding this comment

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

That'll fix the error message problem too. JSON numbers must start with 0-9 or -. If the upcoming character is not one of those, you can avoid calling the function that would produced the bad message in this situation, and directly create a useful "invalid value" error instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've patched this with a check for whitespace characters. I don't quite follow what you mean about fixing the error, the misleading text is still present, no? E.g. this example:

fn main() {
    dbg!(serde_json::from_str::<std::collections::HashMap<i32, i32>>(r#"{"abc":0}"#));
}

Previously would print invalid type: string "abc", expected i32 at line 1 column 6, but after this changeset it prints expected value at line 1 column 3.

Copy link
Member

Choose a reason for hiding this comment

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

This is what I was thinking of doing to give a more meaningful error: #1035.


#[inline]
fn deserialize_option<V>(self, visitor: V) -> Result<V::Value>
Expand Down Expand Up @@ -2221,8 +2223,8 @@ where
}

forward_to_deserialize_any! {
bool f32 f64 char str string unit unit_struct seq tuple tuple_struct map
struct identifier ignored_any
bool char str string unit unit_struct seq tuple tuple_struct map struct
identifier ignored_any
}
}

Expand Down
34 changes: 30 additions & 4 deletions src/ser.rs
View file
Open in desktop
Expand Up @@ -1002,12 +1002,38 @@ where
.map_err(Error::io)
}

fn serialize_f32(self, _value: f32) -> Result<()> {
Err(key_must_be_a_string())
fn serialize_f32(self, value: f32) -> Result<()> {
tri!(self
.ser
.formatter
.begin_string(&mut self.ser.writer)
.map_err(Error::io));
tri!(self
.ser
.formatter
.write_f32(&mut self.ser.writer, value)
.map_err(Error::io));
dtolnay marked this conversation as resolved.
Show resolved Hide resolved
self.ser
.formatter
.end_string(&mut self.ser.writer)
.map_err(Error::io)
}

fn serialize_f64(self, _value: f64) -> Result<()> {
Err(key_must_be_a_string())
fn serialize_f64(self, value: f64) -> Result<()> {
tri!(self
.ser
.formatter
.begin_string(&mut self.ser.writer)
.map_err(Error::io));
tri!(self
.ser
.formatter
.write_f64(&mut self.ser.writer, value)
.map_err(Error::io));
self.ser
.formatter
.end_string(&mut self.ser.writer)
.map_err(Error::io)
}

fn serialize_char(self, value: char) -> Result<()> {
Expand Down
26 changes: 14 additions & 12 deletions src/value/de.rs
View file
Open in desktop
Expand Up @@ -1120,7 +1120,7 @@ struct MapKeyDeserializer<'de> {
key: Cow<'de, str>,
}

macro_rules! deserialize_integer_key {
macro_rules! deserialize_numeric_key {
($method:ident => $visit:ident) => {
fn $method<V>(self, visitor: V) -> Result<V::Value, Error>
where
Expand All @@ -1146,16 +1146,18 @@ impl<'de> serde::Deserializer<'de> for MapKeyDeserializer<'de> {
BorrowedCowStrDeserializer::new(self.key).deserialize_any(visitor)
}

deserialize_integer_key!(deserialize_i8 => visit_i8);
deserialize_integer_key!(deserialize_i16 => visit_i16);
deserialize_integer_key!(deserialize_i32 => visit_i32);
deserialize_integer_key!(deserialize_i64 => visit_i64);
deserialize_integer_key!(deserialize_i128 => visit_i128);
deserialize_integer_key!(deserialize_u8 => visit_u8);
deserialize_integer_key!(deserialize_u16 => visit_u16);
deserialize_integer_key!(deserialize_u32 => visit_u32);
deserialize_integer_key!(deserialize_u64 => visit_u64);
deserialize_integer_key!(deserialize_u128 => visit_u128);
deserialize_numeric_key!(deserialize_i8 => visit_i8);
deserialize_numeric_key!(deserialize_i16 => visit_i16);
deserialize_numeric_key!(deserialize_i32 => visit_i32);
deserialize_numeric_key!(deserialize_i64 => visit_i64);
deserialize_numeric_key!(deserialize_i128 => visit_i128);
deserialize_numeric_key!(deserialize_u8 => visit_u8);
deserialize_numeric_key!(deserialize_u16 => visit_u16);
deserialize_numeric_key!(deserialize_u32 => visit_u32);
deserialize_numeric_key!(deserialize_u64 => visit_u64);
deserialize_numeric_key!(deserialize_u128 => visit_u128);
deserialize_numeric_key!(deserialize_f32 => visit_f32);
deserialize_numeric_key!(deserialize_f64 => visit_f64);
dtolnay marked this conversation as resolved.
Show resolved Hide resolved

#[inline]
fn deserialize_option<V>(self, visitor: V) -> Result<V::Value, Error>
Expand Down Expand Up @@ -1193,7 +1195,7 @@ impl<'de> serde::Deserializer<'de> for MapKeyDeserializer<'de> {
}

forward_to_deserialize_any! {
bool f32 f64 char str string bytes byte_buf unit unit_struct seq tuple
bool char str string bytes byte_buf unit unit_struct seq tuple
tuple_struct map struct identifier ignored_any
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/value/ser.rs
View file
Open in desktop
Expand Up @@ -517,12 +517,12 @@ impl serde::Serializer for MapKeySerializer {
Ok(value.to_string())
}

fn serialize_f32(self, _value: f32) -> Result<String> {
Err(key_must_be_a_string())
fn serialize_f32(self, value: f32) -> Result<String> {
Ok(value.to_string())
}

fn serialize_f64(self, _value: f64) -> Result<String> {
Err(key_must_be_a_string())
fn serialize_f64(self, value: f64) -> Result<String> {
Ok(value.to_string())
}

#[inline]
Expand Down
27 changes: 22 additions & 5 deletions tests/test.rs
View file
Open in desktop
Expand Up @@ -1914,21 +1914,38 @@ fn test_integer128_key() {
}

#[test]
fn test_deny_float_key() {
#[derive(Eq, PartialEq, Ord, PartialOrd)]
fn test_float_key() {
#[derive(Eq, PartialEq, Ord, PartialOrd, Debug, Clone)]
struct Float;
impl Serialize for Float {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
serializer.serialize_f32(1.0)
serializer.serialize_f32(1.23)
}
}
impl<'de> Deserialize<'de> for Float {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: de::Deserializer<'de>,
{
f32::deserialize(deserializer).map(|_| Float)
}
}

// map with float key
let map = treemap!(Float => "x");
assert!(serde_json::to_value(map).is_err());
let map = treemap!(Float => "x".to_owned());
let j = r#"{"1.23":"x"}"#;

test_encode_ok(&[(&map, j)]);
test_parse_ok(vec![(j, map)]);

let j = r#"{"x": null}"#;
test_parse_err::<BTreeMap<Float, ()>>(&[(
j,
"invalid type: string \"x\", expected f32 at line 1 column 4",
)]);
}

#[test]
Expand Down