Repository: alexcrichton/toml-rs
Pull request #199 (2017-07-22 20:20:59)

demonstrate failing test for 'empty' values
This is NOT intended to be merged yet, but rather point add a unit test for something that fails when it should not.

vitiral: @alexcrichton I tried to dig into this and have to admit I'm pretty lost with the serde implementation. Any guesses on your part about why this is happening?
alexcrichton: Table headers are currently [lazily emitted](https://github.com/alexcrichton/toml-rs/blob/f6673dfdb3f93bacfbf0c0cab4d78bc4154eec0c/src/ser.rs#L232-L235), which means that if no values are emitted the table header is itself omitted 
vitiral: Can it be changed to fix this bug? Artifact relies on empty objects still
existing.

This worked in toml 0.2

alexcrichton: Yeah seems fine to me to fix! Just wanted to point out where it was located.

Although... now that I [look closer](https://github.com/alexcrichton/toml-rs/blob/f6673dfdb3f93bacfbf0c0cab4d78bc4154eec0c/src/ser.rs#L668-L671) this should be implemented... Odd! I wonder if it's a top-level vs nested distinction that shouldn't be there?
vitiral: OK, sounds good. I think I can probably take a shot at this

vitiral: So I found one of the problems: if a Struct doesn't serialize anything it needed to emit `Error::UnsupportedNone`

Now I'm hitting anoter bug, in `emit_table_header` the `state=State::End`... it seems like this is the default first state? I'm not sure what to do about this.
alexcrichton: Sorry I don't have time right now to help investigate :(
vitiral: @alexcrichton no problem :)

I think I've got a possible fix. The problem had nothing to do with `fn end` -- that was the wrong place to handle this case. The issue is when you have an empty struct you need to still emit the header.

> Note: my fix is currently just a proof of concept and slightly hacky... I'll find a better way to insert the header soon hopefully.

However, to get this to work I made empty structs return an `UnsupportedNone` type -- which breaks a unit test.

However, I would argue the unit test was invalid. I inserted a `println!("toml repr:\n{}", toml::to_string(&Foo { a: None }).unwrap());` into `empty_arrays2` test and got `'toml repr:\n'`... in other words nothing was actually serialized!

I would argue this result proves that the "empty struct" has ALWAYS been equivalent to `None` and that this is therefore NOT a breaking change -- it is a bug fix.
vitiral: Actually, the unit test might be right -- maybe empty types should just return empty strings? Is serializing `None` or `MyStruct { a: None }` as a empty string bad? I don't think so...
vitiral: I applied the same logic to SerializeMap and hit several bugs. Going to look into them. I think I am onto something with allowing `UnsupportedNone` and treating it correctly.
vitiral: well... I was completely on the wrong track and you were completely right. It WAS pretty much just the line of code you pointed to -- it wasn't implemented for `SerializeStruct`.

I just added it to `SerializeStruct` and also added a few good tests.

Thanks for the help. The good news is that after going through 5 rabbit holes on this issue I think I really understand what this library is doing and why.

Patch:
diff --git a/src/ser.rs b/src/ser.rs
index 863bd75..3c9b354 100644
--- a/src/ser.rs
+++ b/src/ser.rs
@@ -930,6 +930,15 @@ impl<'a, 'b> ser::SerializeStruct for SerializeTable<'a, 'b> {
     }
 
     fn end(self) -> Result<(), Error> {
+        match self {
+            SerializeTable::Datetime(_) => {},
+            SerializeTable::Table { mut ser, first, ..  } => {
+                if first.get() {
+                    let state = ser.state.clone();
+                    ser.emit_table_header(&state)?;
+                }
+            }
+        }
         Ok(())
     }
 }


Test patch:
diff --git a/tests/serde.rs b/tests/serde.rs
index c61edf5..57fa5db 100644
--- a/tests/serde.rs
+++ b/tests/serde.rs
@@ -554,3 +554,25 @@ fn newtypes2() {
         }),
     }
 }
+
+#[derive(Debug, Default, PartialEq, Serialize, Deserialize)]
+struct CanBeEmpty {
+    a: Option<String>,
+    b: Option<String>,
+}
+
+#[test]
+fn table_structs_empty() {
+    let text = "[bar]\n\n[baz]\n\n[bazv]\na = \"foo\"\n\n[foo]\n";
+    let value: BTreeMap<String, CanBeEmpty> = toml::from_str(text).unwrap();
+    let mut expected: BTreeMap<String, CanBeEmpty> = BTreeMap::new();
+    expected.insert("bar".to_string(), CanBeEmpty::default());
+    expected.insert("baz".to_string(), CanBeEmpty::default());
+    expected.insert(
+        "bazv".to_string(), 
+        CanBeEmpty {a: Some("foo".to_string()), b: None},
+    );
+    expected.insert("foo".to_string(), CanBeEmpty::default());
+    assert_eq!(value, expected);
+    assert_eq!(toml::to_string(&value).unwrap(), text);
+}
diff --git a/tests/valid.rs b/tests/valid.rs
index e7577ad..6a04866 100644
--- a/tests/valid.rs
+++ b/tests/valid.rs
@@ -162,6 +162,9 @@ test!(table_empty,
 test!(table_sub_empty,
        include_str!("valid/table-sub-empty.toml"),
        include_str!("valid/table-sub-empty.json"));
+test!(table_multi_empty,
+       include_str!("valid/table-multi-empty.toml"),
+       include_str!("valid/table-multi-empty.json"));
 test!(table_whitespace,
        include_str!("valid/table-whitespace.toml"),
        include_str!("valid/table-whitespace.json"));
diff --git a/tests/valid/table-multi-empty.json b/tests/valid/table-multi-empty.json
new file mode 100644
index 0000000..a6e17c9
--- /dev/null
+++ b/tests/valid/table-multi-empty.json
@@ -0,0 +1,5 @@
+{
+    "a": { "b": {} },
+    "b": {},
+    "c": { "a": {} }
+}
diff --git a/tests/valid/table-multi-empty.toml b/tests/valid/table-multi-empty.toml
new file mode 100644
index 0000000..2266ed2
--- /dev/null
+++ b/tests/valid/table-multi-empty.toml
@@ -0,0 +1,5 @@
+[a]
+[a.b]
+[b]
+[c]
+[c.a]