Repository: ron-rs/ron
Pull request #455 (2023-04-24 11:06:56)

Add minimal support for flattening roundtrips through maps
Supersedes #403, #453, and #454 to add minimal support for `#[serde(flatten)]` in RON. Instead of many workarounds to make flattened structs serialise into nice RON struct syntax and deserialise from them, both of which are really difficult since serde gives us no information to work with and just delegates to map serialisation/deserialisation, this PR just ensures that at least flattened structs can roundtrip through RON.

For this, just one hack is necessary. If we detect that a map is deserialised into what looks like a struct (thanks @clouds56 for contributing the detection in #454), we enforce that map key identifiers are actually strings written exactly as `"`, identifier, `"`. This seems to be enough to get the tests to work that I initially designed for https://github.com/ron-rs/ron/pull/403.

@torkleyy I'd be in favour of landing this instead of going down the rabbit hole of https://github.com/ron-rs/ron/pull/403 since that just fixes one of the two parts and getting flattened structs to serialise correctly would require hacks of another order.

This would not really fix https://github.com/ron-rs/ron/issues/115 but at least get some working if imperfect solution.

* [x] Add tests for the new error cases, i.e. when flattening goes wrong
* [x] I've included my change in `CHANGELOG.md`


codecov-commenter: ## [Codecov](https://codecov.io/gh/ron-rs/ron/pull/455?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
Patch coverage: **`96.17`**% and project coverage change: **`-0.41`** :warning:
> Comparison is base [(`484fcab`)](https://codecov.io/gh/ron-rs/ron/commit/484fcab0686dfd18c7e29b6c1815c5f74f6c6db4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 86.65% compared to head [(`1c06880`)](https://codecov.io/gh/ron-rs/ron/pull/455?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 86.24%.

:mega: This organization is not using Codecov’s [GitHub App Integration](https://github.com/apps/codecov). We recommend you install it so Codecov can continue to function properly for your repositories. [Learn more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
- Coverage   86.65%   86.24%   -0.41%     
==========================================
  Files          60       61       +1     
  Lines        7364     7619     +255     
==========================================
+ Hits         6381     6571     +190     
- Misses        983     1048      +65     
```


| [Impacted Files](https://codecov.io/gh/ron-rs/ron/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [src/de/mod.rs](https://codecov.io/gh/ron-rs/ron/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2RlL21vZC5ycw==) | `73.73% <80.48%> (-3.19%)` | :arrow_down: |
| [src/de/id.rs](https://codecov.io/gh/ron-rs/ron/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2RlL2lkLnJz) | `87.65% <100.00%> (-12.35%)` | :arrow_down: |
| [tests/115\_minimal\_flattening.rs](https://codecov.io/gh/ron-rs/ron/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dGVzdHMvMTE1X21pbmltYWxfZmxhdHRlbmluZy5ycw==) | `100.00% <100.00%> (ø)` | |

... and [2 files with indirect coverage changes](https://codecov.io/gh/ron-rs/ron/pull/455/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

</details>

[:umbrella: View full report in Codecov by Sentry](https://codecov.io/gh/ron-rs/ron/pull/455?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 82bdefd6..f3e681c2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Bump dependency `base64` to 0.21 ([#433](https://github.com/ron-rs/ron/pull/433))
 - Depend on `serde_derive` directly to potentially enable more compilation parallelism ([#441](https://github.com/ron-rs/ron/pull/441))
 - Add `compact_maps` and `compact_structs` options to `PrettyConfig` to allow serialising maps and structs on a single line ([#448](https://github.com/ron-rs/ron/pull/448))
+- Add minimal support for `#[serde(flatten)]` with roundtripping through RON maps ([#455](https://github.com/ron-rs/ron/pull/455))
 
 ## [0.8.0] - 2022-08-17
 
diff --git a/README.md b/README.md
index 6c7cc31f..8ba55fd7 100644
--- a/README.md
+++ b/README.md
@@ -107,7 +107,10 @@ Note the following advantages of RON over JSON:
 RON is not designed to be a fully self-describing format (unlike JSON) and is thus not guaranteed to work when [`deserialize_any`](https://docs.rs/serde/latest/serde/trait.Deserializer.html#tymethod.deserialize_any) is used instead of its typed alternatives. In particular, the following Serde attributes are not yet supported:
 - `#[serde(tag = "type")]`, i.e. internally tagged enums
 - `#[serde(untagged)]`, i.e. untagged enums
-- `#[serde(flatten)]`, i.e. flattening an inner struct into its outer container
+
+Furthermore, `#[serde(flatten)]` only has limited support and relies on a small hack [^serde-flatten-hack]. Specifically, flattened structs are only serialised as maps and deserialised from maps. However, this limited implementation supports full roundtripping.
+
+[^serde-flatten-hack]: Deserialising a flattened struct from a map requires that the struct's [`Visitor::expecting`](https://docs.rs/serde/latest/serde/de/trait.Visitor.html#tymethod.expecting) implementation formats a string starting with `"struct "`. This is the case for automatically-derived [`Deserialize`](https://docs.rs/serde/latest/serde/de/trait.Deserialize.html) impls on structs.
 
 ## RON syntax overview
 
diff --git a/src/de/id.rs b/src/de/id.rs
index e7230da5..f5cd84c6 100644
--- a/src/de/id.rs
+++ b/src/de/id.rs
@@ -4,11 +4,12 @@ use super::{Deserializer, Error, Result};
 
 pub struct IdDeserializer<'a, 'b: 'a> {
     d: &'a mut Deserializer<'b>,
+    map_as_struct: bool,
 }
 
 impl<'a, 'b: 'a> IdDeserializer<'a, 'b> {
-    pub fn new(d: &'a mut Deserializer<'b>) -> Self {
-        IdDeserializer { d }
+    pub fn new(map_as_struct: bool, d: &'a mut Deserializer<'b>) -> Self {
+        IdDeserializer { d, map_as_struct }
     }
 }
 
@@ -19,7 +20,14 @@ impl<'a, 'b: 'a, 'c> de::Deserializer<'b> for &'c mut IdDeserializer<'a, 'b> {
     where
         V: Visitor<'b>,
     {
-        self.d.deserialize_identifier(visitor)
+        if self.map_as_struct {
+            self.d.bytes.expect_byte(b'"', Error::ExpectedString)?;
+        }
+        let result = self.d.deserialize_identifier(visitor);
+        if self.map_as_struct {
+            self.d.bytes.expect_byte(b'"', Error::ExpectedStringEnd)?;
+        }
+        result
     }
 
     fn deserialize_str<V>(self, visitor: V) -> Result<V::Value>
diff --git a/src/de/mod.rs b/src/de/mod.rs
index 012da06c..fb7c07b5 100644
--- a/src/de/mod.rs
+++ b/src/de/mod.rs
@@ -1,5 +1,9 @@
 /// Deserialization module.
-use std::{borrow::Cow, io, str};
+use std::{
+    borrow::Cow,
+    io::{self, Write},
+    str,
+};
 
 use base64::Engine;
 use serde::de::{self, DeserializeSeed, Deserializer as SerdeError, Visitor};
@@ -176,7 +180,7 @@ impl<'de> Deserializer<'de> {
 
             let value = guard_recursion! { self =>
                 visitor
-                    .visit_map(CommaSeparated::new(b')', self))
+                    .visit_map(CommaSeparated::new(Terminator::Struct, self))
                     .map_err(|err| {
                         struct_error_name(
                             err,
@@ -525,7 +529,7 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
 
         if self.bytes.consume("[") {
             let value = guard_recursion! { self =>
-                visitor.visit_seq(CommaSeparated::new(b']', self))?
+                visitor.visit_seq(CommaSeparated::new(Terminator::Seq, self))?
             };
             self.bytes.skip_ws()?;
 
@@ -548,7 +552,7 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
             self.newtype_variant = false;
 
             let value = guard_recursion! { self =>
-                visitor.visit_seq(CommaSeparated::new(b')', self))?
+                visitor.visit_seq(CommaSeparated::new(Terminator::Tuple, self))?
             };
             self.bytes.skip_ws()?;
 
@@ -585,11 +589,29 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
     where
         V: Visitor<'de>,
     {
+        // Detect `#[serde(flatten)]` as a struct deserialised as a map
+        const SERDE_FLATTEN_CANARY: &[u8] = b"struct ";
+
+        struct VisitorExpecting<V>(V);
+        impl<'de, V: Visitor<'de>> std::fmt::Display for VisitorExpecting<&'_ V> {
+            fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
+                self.0.expecting(fmt)
+            }
+        }
+
         self.newtype_variant = false;
 
+        let mut canary_buffer = [0u8; SERDE_FLATTEN_CANARY.len()];
+        let _ = write!(canary_buffer.as_mut(), "{}", VisitorExpecting(&visitor));
+        let terminator = if canary_buffer == SERDE_FLATTEN_CANARY {
+            Terminator::MapAsStruct
+        } else {
+            Terminator::Map
+        };
+
         if self.bytes.consume("{") {
             let value = guard_recursion! { self =>
-                visitor.visit_map(CommaSeparated::new(b'}', self))?
+                visitor.visit_map(CommaSeparated::new(terminator, self))?
             };
             self.bytes.skip_ws()?;
 
@@ -666,14 +688,33 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
     }
 }
 
+#[derive(Clone, Copy, PartialEq, Eq)]
+enum Terminator {
+    Map,
+    MapAsStruct,
+    Tuple,
+    Struct,
+    Seq,
+}
+
+impl Terminator {
+    fn as_byte(self) -> u8 {
+        match self {
+            Terminator::Map | Terminator::MapAsStruct => b'}',
+            Terminator::Tuple | Terminator::Struct => b')',
+            Terminator::Seq => b']',
+        }
+    }
+}
+
 struct CommaSeparated<'a, 'de: 'a> {
     de: &'a mut Deserializer<'de>,
-    terminator: u8,
+    terminator: Terminator,
     had_comma: bool,
 }
 
 impl<'a, 'de> CommaSeparated<'a, 'de> {
-    fn new(terminator: u8, de: &'a mut Deserializer<'de>) -> Self {
+    fn new(terminator: Terminator, de: &'a mut Deserializer<'de>) -> Self {
         CommaSeparated {
             de,
             terminator,
@@ -686,7 +727,7 @@ impl<'a, 'de> CommaSeparated<'a, 'de> {
 
         match (
             self.had_comma,
-            self.de.bytes.peek_or_eof()? != self.terminator,
+            self.de.bytes.peek_or_eof()? != self.terminator.as_byte(),
         ) {
             // Trailing comma, maybe has a next element
             (true, has_element) => Ok(has_element),
@@ -725,12 +766,14 @@ impl<'de, 'a> de::MapAccess<'de> for CommaSeparated<'a, 'de> {
         K: DeserializeSeed<'de>,
     {
         if self.has_element()? {
-            if self.terminator == b')' {
-                guard_recursion! { self.de =>
-                    seed.deserialize(&mut IdDeserializer::new(&mut *self.de)).map(Some)
-                }
-            } else {
-                guard_recursion! { self.de => seed.deserialize(&mut *self.de).map(Some) }
+            match self.terminator {
+                Terminator::Struct => guard_recursion! { self.de =>
+                    seed.deserialize(&mut IdDeserializer::new(false, &mut *self.de)).map(Some)
+                },
+                Terminator::MapAsStruct => guard_recursion! { self.de =>
+                    seed.deserialize(&mut IdDeserializer::new(true, &mut *self.de)).map(Some)
+                },
+                _ => guard_recursion! { self.de => seed.deserialize(&mut *self.de).map(Some) },
             }
         } else {
             Ok(None)


Test patch:
diff --git a/tests/115_minimal_flattening.rs b/tests/115_minimal_flattening.rs
new file mode 100644
index 00000000..f4bb581c
--- /dev/null
+++ b/tests/115_minimal_flattening.rs
@@ -0,0 +1,324 @@
+use std::collections::HashMap;
+
+use serde::{Deserialize, Serialize};
+
+#[derive(Deserialize, Serialize, PartialEq, Eq, Debug)]
+struct Main {
+    #[serde(flatten)]
+    required: Required,
+    #[serde(flatten)]
+    optional: Optional,
+
+    some_other_field: u32,
+}
+
+#[derive(Deserialize, Serialize, PartialEq, Eq, Debug)]
+struct Required {
+    first: u32,
+    second: u32,
+}
+
+#[derive(Deserialize, Serialize, PartialEq, Eq, Debug)]
+struct Optional {
+    third: Option<u32>,
+}
+
+#[derive(Deserialize, Serialize, PartialEq, Eq, Debug)]
+struct MyType {
+    first: u32,
+    second: u32,
+    #[serde(flatten)]
+    everything_else: HashMap<String, ron::Value>,
+}
+
+#[derive(Deserialize, Serialize, PartialEq, Eq, Debug)]
+struct AllOptional {
+    #[serde(flatten)]
+    everything_else: HashMap<String, ron::Value>,
+}
+
+#[derive(Deserialize, Serialize, PartialEq, Eq, Debug)]
+enum Newtype {
+    Main(Main),
+    MyType(MyType),
+    AllOptional(AllOptional),
+}
+
+#[test]
+fn test_flatten_struct_into_struct() {
+    let val = Main {
+        required: Required {
+            first: 1,
+            second: 2,
+        },
+        optional: Optional { third: Some(3) },
+        some_other_field: 1337,
+    };
+
+    let ron = ron::ser::to_string_pretty(&val, ron::ser::PrettyConfig::default()).unwrap();
+
+    assert_eq!(
+        ron,
+        "{
+    \"first\": 1,
+    \"second\": 2,
+    \"third\": Some(3),
+    \"some_other_field\": 1337,
+}"
+    );
+
+    let de: Main = ron::from_str(&ron).unwrap();
+
+    assert_eq!(de, val);
+
+    let val = Newtype::Main(Main {
+        required: Required {
+            first: 1,
+            second: 2,
+        },
+        optional: Optional { third: Some(3) },
+        some_other_field: 1337,
+    });
+
+    let ron = ron::ser::to_string_pretty(
+        &val,
+        ron::ser::PrettyConfig::default()
+            .extensions(ron::extensions::Extensions::UNWRAP_VARIANT_NEWTYPES),
+    )
+    .unwrap();
+
+    assert_eq!(
+        ron,
+        "#![enable(unwrap_variant_newtypes)]
+Main({
+    \"first\": 1,
+    \"second\": 2,
+    \"third\": Some(3),
+    \"some_other_field\": 1337,
+})"
+    );
+
+    let ron = ron::ser::to_string_pretty(&val, ron::ser::PrettyConfig::default()).unwrap();
+
+    let de: Newtype = ron::from_str(&ron).unwrap();
+
+    assert_eq!(de, val);
+
+    assert_eq!(
+        ron::from_str::<Main>(
+            "{
+        first\": 1,
+        \"second\": 2,
+        \"third\": Some(3),
+        \"some_other_field\": 1337,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedString,
+            position: ron::error::Position { line: 2, col: 10 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<Main>(
+            "{
+        \"first\": 1,
+        \"second: 2,
+        \"third\": Some(3),
+        \"some_other_field\": 1337,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedStringEnd,
+            position: ron::error::Position { line: 3, col: 17 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<Main>(
+            "{
+        \"first\": 1,
+        \"second\": 2,
+        third\": Some(3),
+        \"some_other_field\": 1337,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedString,
+            position: ron::error::Position { line: 4, col: 10 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<Main>(
+            "{
+        \"first\": 1,
+        \"second\": 2,
+        \"third\": Some(3),
+        \"some_other_field: 1337,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedStringEnd,
+            position: ron::error::Position { line: 5, col: 27 },
+        })
+    );
+}
+
+#[test]
+fn test_flatten_rest() {
+    let val = MyType {
+        first: 1,
+        second: 2,
+        everything_else: {
+            let mut map = HashMap::new();
+            map.insert(
+                String::from("third"),
+                ron::Value::Number(ron::value::Number::from(3)),
+            );
+            map
+        },
+    };
+
+    let ron = ron::ser::to_string_pretty(&val, ron::ser::PrettyConfig::default()).unwrap();
+
+    assert_eq!(
+        ron,
+        "{
+    \"first\": 1,
+    \"second\": 2,
+    \"third\": 3,
+}"
+    );
+
+    let de: MyType = ron::from_str(&ron).unwrap();
+
+    assert_eq!(de, val);
+
+    let val = Newtype::MyType(MyType {
+        first: 1,
+        second: 2,
+        everything_else: {
+            let mut map = HashMap::new();
+            map.insert(
+                String::from("third"),
+                ron::Value::Number(ron::value::Number::from(3)),
+            );
+            map
+        },
+    });
+
+    let ron = ron::ser::to_string_pretty(
+        &val,
+        ron::ser::PrettyConfig::default()
+            .extensions(ron::extensions::Extensions::UNWRAP_VARIANT_NEWTYPES),
+    )
+    .unwrap();
+
+    assert_eq!(
+        ron,
+        "#![enable(unwrap_variant_newtypes)]
+MyType({
+    \"first\": 1,
+    \"second\": 2,
+    \"third\": 3,
+})"
+    );
+
+    let de: Newtype = ron::from_str(&ron).unwrap();
+
+    assert_eq!(de, val);
+
+    assert_eq!(
+        ron::from_str::<MyType>(
+            "{
+        first\": 1,
+        \"second\": 2,
+        \"third\": 3,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedString,
+            position: ron::error::Position { line: 2, col: 10 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<MyType>(
+            "{
+        \"first\": 1,
+        \"second: 2,
+        \"third\": 3,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedStringEnd,
+            position: ron::error::Position { line: 3, col: 17 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<MyType>(
+            "{
+        \"first\": 1,
+        \"second\": 2,
+        third\": 3,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedString,
+            position: ron::error::Position { line: 4, col: 10 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<MyType>(
+            "{
+        \"first\": 1,
+        \"second\": 2,
+        \"third: 3,
+    }"
+        ),
+        Err(ron::error::SpannedError {
+            code: ron::error::Error::ExpectedStringEnd,
+            position: ron::error::Position { line: 4, col: 16 },
+        })
+    );
+}
+
+#[test]
+fn test_flatten_only_rest() {
+    let val = AllOptional {
+        everything_else: HashMap::new(),
+    };
+
+    let ron = ron::ser::to_string(&val).unwrap();
+
+    assert_eq!(ron, "{}");
+
+    let de: AllOptional = ron::from_str(&ron).unwrap();
+
+    assert_eq!(de, val);
+
+    let val = Newtype::AllOptional(AllOptional {
+        everything_else: HashMap::new(),
+    });
+
+    let ron = ron::ser::to_string_pretty(
+        &val,
+        ron::ser::PrettyConfig::default()
+            .extensions(ron::extensions::Extensions::UNWRAP_VARIANT_NEWTYPES),
+    )
+    .unwrap();
+
+    assert_eq!(
+        ron,
+        "#![enable(unwrap_variant_newtypes)]
+AllOptional({
+})"
+    );
+
+    let de: Newtype = ron::from_str(&ron).unwrap();
+
+    assert_eq!(de, val);
+}