ser: sequences: Rework, fixing handling of nested arrays

Change the representation of our "current location".  Previously it
was a list of increasingly-long full paths, but excepting the putative
final array index.

Now we just record the individual elements. and assemble the whole
path just before calling .set().  This saves a little memory on the
whole since the entries in keys are now a bit shorter.

It is much less confusing.  (There are perhaps still further
opportunities to use Rust's type system to better advantage to
eliminuate opportunities for bugs.)

Arrays that appear other than at the top level are now handled
correctly.  This includes nested arrays, and arrays containing
structs, etc.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

1 files changed, 55 insertions, 49 deletions

diff --git a/src/ser.rs b/src/ser.rs
index 9a86ca1..f87bea2 100644
--- a/src/ser.rs
+++ b/src/ser.rs
@@ -1,4 +1,5 @@
 use std::fmt::Display;
+use std::fmt::Write as _;
 
 use serde::ser;
 
@@ -8,10 +9,25 @@ use crate::Config;
 
 #[derive(Default, Debug)]
 pub struct ConfigSerializer {
-    keys: Vec<(String, Option<usize>)>,
+    keys: Vec<SerKey>,
     pub output: Config,
 }
 
+#[derive(Debug)]
+enum SerKey {
+    Named(String),
+    Seq(usize),
+}
+
+/// Serializer for numbered sequences
+///
+/// This wrapper is present when we are outputting a sequence (numbered indices).
+/// Making this a separate type centralises the handling of sequences
+/// and ensures we don't have any call sites for `ser::SerializeSeq::serialize_element`
+/// that don't do the necessary work of `SeqSerializer::new`.
+///
+/// Existence of this wrapper implies that `.0.keys.last()` is
+/// `Some(SerKey::Seq(next_index))`.
 pub struct SeqSerializer<'a>(&'a mut ConfigSerializer);
 
 impl ConfigSerializer {
@@ -19,68 +35,47 @@ impl ConfigSerializer {
     where
         T: Into<Value> + Display,
     {
-        let key = match self.last_key_index_pair() {
-            Some((key, Some(index))) => format!("{}[{}]", key, index),
-            Some((key, None)) => key.to_string(),
-            None => {
-                return Err(ConfigError::Message(format!(
-                    "key is not found for value {}",
-                    value
-                )))
-            }
-        };
+        // At some future point we could perhaps retain a cursor into the output `Config`,
+        // rather than reifying the whole thing into a single string with `make_full_key`
+        // and passing that whole path to the `set` method.
+        //
+        // That would be marginally more performant, but more fiddly.
+        let key = self.make_full_key()?;
 
         #[allow(deprecated)]
         self.output.set(&key, value.into())?;
         Ok(())
     }
 
-    fn last_key_index_pair(&self) -> Option<(&str, Option<usize>)> {
-        let len = self.keys.len();
-        if len > 0 {
-            self.keys
-                .get(len - 1)
-                .map(|&(ref key, opt)| (key.as_str(), opt))
-        } else {
-            None
-        }
-    }
+    fn make_full_key(&self) -> Result<String> {
+        let mut keys = self.keys.iter();
 
-    fn inc_last_key_index(&mut self) -> Result<()> {
-        let len = self.keys.len();
-        if len > 0 {
-            self.keys
-                .get_mut(len - 1)
-                .map(|pair| pair.1 = pair.1.map(|i| i + 1).or(Some(0)))
-                .ok_or_else(|| {
-                    ConfigError::Message(format!("last key is not found in {} keys", len))
-                })
-        } else {
-            Err(ConfigError::Message("keys is empty".to_string()))
-        }
-    }
+        let mut whole = match keys.next() {
+            Some(SerKey::Named(s)) => s.clone(),
+            _ => {
+                return Err(ConfigError::Message(
+                    "top level is not a struct".to_string(),
+                ))
+            }
+        };
 
-    fn make_full_key(&self, key: &str) -> String {
-        let len = self.keys.len();
-        if len > 0 {
-            if let Some(&(ref prev_key, index)) = self.keys.get(len - 1) {
-                return if let Some(index) = index {
-                    format!("{}[{}].{}", prev_key, index, key)
-                } else {
-                    format!("{}.{}", prev_key, key)
-                };
+        for k in keys {
+            match k {
+                SerKey::Named(s) => write!(whole, ".{}", s),
+                SerKey::Seq(i) => write!(whole, "[{}]", i),
             }
+            .expect("write! to a string failed");
         }
-        key.to_string()
+
+        Ok(whole)
     }
 
     fn push_key(&mut self, key: &str) {
-        let full_key = self.make_full_key(key);
-        self.keys.push((full_key, None));
+        self.keys.push(SerKey::Named(key.to_string()));
     }
 
-    fn pop_key(&mut self) -> Option<(String, Option<usize>)> {
-        self.keys.pop()
+    fn pop_key(&mut self) {
+        self.keys.pop();
     }
 }
 
@@ -265,10 +260,14 @@ impl<'a> ser::Serializer for &'a mut ConfigSerializer {
 
 impl<'a> SeqSerializer<'a> {
     fn new(inner: &'a mut ConfigSerializer) -> Result<Self> {
+        inner.keys.push(SerKey::Seq(0));
+
         Ok(SeqSerializer(inner))
     }
 
     fn end(self) -> &'a mut ConfigSerializer {
+        // This ought to be Some(SerKey::Seq(..)) but we don't want to panic if we are buggy
+        let _: Option<SerKey> = self.0.keys.pop();
         self.0
     }
 }
@@ -281,8 +280,15 @@ impl<'a> ser::SerializeSeq for SeqSerializer<'a> {
     where
         T: ?Sized + ser::Serialize,
     {
-        self.0.inc_last_key_index()?;
         value.serialize(&mut *(self.0))?;
+        match self.0.keys.last_mut() {
+            Some(SerKey::Seq(i)) => *i += 1,
+            _ => {
+                return Err(ConfigError::Message(
+                    "config-rs internal error (ser._element but last not Seq!".to_string(),
+                ))
+            }
+        };
         Ok(())
     }