Incorrect MapAccess usage allows parsing malformed JSON / Incorrect MapAccess usage is not detected

[Marcono1234]

Version

1.0.91

Description

Relates to #568

Incorrect usage of MapAccess allows parsing JSON data which is malformed. In particular none of these error cases are detected:

• leading value (without key in front)
• multiple keys without values in between
• multiple values without keys in between
• trailing key (without value afterwards)

Maybe serde_json should return an error or even panic in these cases since this is a programming error by the user and unrelated to the JSON data being read. Tracking the state probably only requires one additional bool field to determine whether a key or value is expected.

However, I can also understand if you consider this "won't fix" since the user code is incorrect. Though the current behavior can make troubleshooting more difficult because in case serde_json reports errors they suggest the JSON data is malformed while actually the user code is incorrect (as seen with #568).

Reproducer code

use serde::de::{Deserializer, Visitor};

struct V;
impl<'de> Visitor<'de> for V {
    type Value = Vec<u8>;

    fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
        write!(formatter, "map")
    }

    fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
    where
        A: serde::de::MapAccess<'de>,
    {
        Ok(vec![
            map.next_key()?.unwrap(),
            // Incorrect MapAccess usage
            map.next_key()?.unwrap(),
            map.next_key()?.unwrap(),
            map.next_value()?,
            map.next_value()?
        ])
    }
}

fn main() {
    let mut de = serde_json::Deserializer::from_str(r#"{"1", "2", "3": 4: 5}"#);
    let map = de.deserialize_map(V).unwrap();
    println!("map: {map:?}");
    de.end().unwrap();
}

This deserializes the malformed JSON object {"1", "2", "3": 4: 5} without reporting any error.

[dtolnay]

Buggy Deserialize/Visitor implementations and buggy Deserializer/MapAccess implementations can both exhibit buggy behavior. I think this is fine as currently implemented.