Add new serde option "detect_mixed_tables"

This option would allow detecting mixed tables (with array-like and map-like entries or several borders)
to encoding them chosing the best method (as a map or as a table).

Author: khvzak

src/serde/de.rs

@@ -15,11 +15,12 @@ use crate::userdata::AnyUserData;
 use crate::value::Value;
 
 /// A struct for deserializing Lua values into Rust values.
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct Deserializer {
     value: Value,
     options: Options,
     visited: Rc<RefCell<FxHashSet<*const c_void>>>,
+    len: Option<usize>, // A length hint for sequences
 }
 
 /// A struct with options to change default deserializer behavior.
@@ -54,6 +55,19 @@ pub struct Options {
     ///
     /// Default: **false**
     pub encode_empty_tables_as_array: bool,
+
+    /// If true, enable detection of mixed tables.
+    ///
+    /// A mixed table is a table that has both array-like and map-like entries or several borders.
+    /// See [`The Length Operator`] documentation for details about borders.
+    ///
+    /// When this option is disabled, a table with a non-zero length (with one or more borders) will
+    /// be always encoded as an array.
+    ///
+    /// Default: **false**
+    ///
+    /// [`The Length Operator`]: https://www.lua.org/manual/5.4/manual.html#3.4.7
+    pub detect_mixed_tables: bool,
 }
 
 impl Default for Options {
@@ -70,6 +84,7 @@ impl Options {
             deny_recursive_tables: true,
             sort_keys: false,
             encode_empty_tables_as_array: false,
+            detect_mixed_tables: false,
         }
     }
 
@@ -108,6 +123,15 @@ impl Options {
         self.encode_empty_tables_as_array = enabled;
         self
     }
+
+    /// Sets [`detect_mixed_tables`] option.
+    ///
+    /// [`detect_mixed_tables`]: #structfield.detect_mixed_tables
+    #[must_use]
+    pub const fn detect_mixed_tables(mut self, enable: bool) -> Self {
+        self.detect_mixed_tables = enable;
+        self
+    }
 }
 
 impl Deserializer {
@@ -121,7 +145,7 @@ impl Deserializer {
         Deserializer {
             value,
             options,
-            visited: Rc::new(RefCell::new(FxHashSet::default())),
+            ..Default::default()
         }
     }
 
@@ -130,8 +154,14 @@ impl Deserializer {
             value,
             options,
             visited,
+            ..Default::default()
         }
     }
+
+    fn with_len(mut self, len: usize) -> Self {
+        self.len = Some(len);
+        self
+    }
 }
 
 impl<'de> serde::Deserializer<'de> for Deserializer {
@@ -155,11 +185,13 @@ impl<'de> serde::Deserializer<'de> for Deserializer {
                 Ok(s) => visitor.visit_str(&s),
                 Err(_) => visitor.visit_bytes(&s.as_bytes()),
             },
-            Value::Table(ref t) if t.raw_len() > 0 || t.is_array() => self.deserialize_seq(visitor),
-            Value::Table(ref t) if self.options.encode_empty_tables_as_array && t.is_empty() => {
-                self.deserialize_seq(visitor)
+            Value::Table(ref t) => {
+                if let Some(len) = t.encode_as_array(self.options) {
+                    self.with_len(len).deserialize_seq(visitor)
+                } else {
+                    self.deserialize_map(visitor)
+                }
             }
-            Value::Table(_) => self.deserialize_map(visitor),
             Value::LightUserData(ud) if ud.0.is_null() => visitor.visit_none(),
             Value::UserData(ud) if ud.is_serializable() => {
                 serde_userdata(ud, |value| value.deserialize_any(visitor))
@@ -270,14 +302,14 @@ impl<'de> serde::Deserializer<'de> for Deserializer {
             Value::Table(t) => {
                 let _guard = RecursionGuard::new(&t, &self.visited);
 
-                let len = t.raw_len();
+                let len = self.len.unwrap_or_else(|| t.raw_len());
                 let mut deserializer = SeqDeserializer {
-                    seq: t.sequence_values(),
+                    seq: t.sequence_values().with_len(len),
                     options: self.options,
                     visited: self.visited,
                 };
                 let seq = visitor.visit_seq(&mut deserializer)?;
-                if deserializer.seq.count() == 0 {
+                if deserializer.seq.next().is_none() {
                     Ok(seq)
                 } else {
                     Err(de::Error::invalid_length(len, &"fewer elements in the table"))

src/table.rs

@@ -668,26 +668,40 @@ impl Table {
             guard: self.0.lua.lock(),
             table: self,
             index: 1,
+            len: None,
             _phantom: PhantomData,
         }
     }
 
     /// Iterates over the sequence part of the table, invoking the given closure on each value.
+    ///
+    /// This methods is similar to [`Table::sequence_values`], but optimized for performance.
     #[doc(hidden)]
-    pub fn for_each_value<V>(&self, mut f: impl FnMut(V) -> Result<()>) -> Result<()>
-    where
-        V: FromLua,
-    {
+    pub fn for_each_value<V: FromLua>(&self, f: impl FnMut(V) -> Result<()>) -> Result<()> {
+        self.for_each_value_by_len(None, f)
+    }
+
+    fn for_each_value_by_len<V: FromLua>(
+        &self,
+        len: impl Into<Option<usize>>,
+        mut f: impl FnMut(V) -> Result<()>,
+    ) -> Result<()> {
+        let len = len.into();
         let lua = self.0.lua.lock();
         let state = lua.state();
         unsafe {
             let _sg = StackGuard::new(state);
             check_stack(state, 4)?;
 
             lua.push_ref(&self.0);
-            let len = ffi::lua_rawlen(state, -1);
-            for i in 1..=len {
-                ffi::lua_rawgeti(state, -1, i as _);
+            for i in 1.. {
+                if len.map(|len| i > len).unwrap_or(false) {
+                    break;
+                }
+                let t = ffi::lua_rawgeti(state, -1, i as _);
+                if len.is_none() && t == ffi::LUA_TNIL {
+                    break;
+                }
                 f(V::from_stack(-1, &lua)?)?;
                 ffi::lua_pop(state, 1);
             }
@@ -720,8 +734,9 @@ impl Table {
         Ok(())
     }
 
+    /// Checks if the table has the array metatable attached.
     #[cfg(feature = "serde")]
-    pub(crate) fn is_array(&self) -> bool {
+    fn has_array_metatable(&self) -> bool {
         let lua = self.0.lua.lock();
         let state = lua.state();
         unsafe {
@@ -737,6 +752,70 @@ impl Table {
         }
     }
 
+    /// If the table is an array, returns the number of non-nil elements and max index.
+    ///
+    /// Returns `None` if the table is not an array.
+    ///
+    /// This operation has O(n) complexity.
+    #[cfg(feature = "serde")]
+    fn find_array_len(&self) -> Option<(usize, usize)> {
+        let lua = self.0.lua.lock();
+        let ref_thread = lua.ref_thread();
+        unsafe {
+            let _sg = StackGuard::new(ref_thread);
+
+            let (mut count, mut max_index) = (0, 0);
+            ffi::lua_pushnil(ref_thread);
+            while ffi::lua_next(ref_thread, self.0.index) != 0 {
+                if ffi::lua_type(ref_thread, -2) != ffi::LUA_TNUMBER {
+                    return None;
+                }
+
+                let k = ffi::lua_tonumber(ref_thread, -2);
+                if k.trunc() != k || k < 1.0 {
+                    return None;
+                }
+                max_index = std::cmp::max(max_index, k as usize);
+                count += 1;
+                ffi::lua_pop(ref_thread, 1);
+            }
+            Some((count, max_index))
+        }
+    }
+
+    /// Determines if the table should be encoded as an array or a map.
+    ///
+    /// The algorithm is the following:
+    /// 1. If `detect_mixed_tables` is enabled, iterate over all keys in the table checking is they
+    ///    all are positive integers. If non-array key is found, return `None` (encode as map).
+    ///    Otherwise check the sparsity of the array. Too sparse arrays are encoded as maps.
+    ///
+    /// 2. If `detect_mixed_tables` is disabled, check if the table has a positive length or has the
+    ///    array metatable. If so, encode as array. If the table is empty and
+    ///    `encode_empty_tables_as_array` is enabled, encode as array.
+    ///
+    /// Returns the length of the array if it should be encoded as an array.
+    #[cfg(feature = "serde")]
+    pub(crate) fn encode_as_array(&self, options: crate::serde::de::Options) -> Option<usize> {
+        if options.detect_mixed_tables {
+            if let Some((len, max_idx)) = self.find_array_len() {
+                // If the array is too sparse, serialize it as a map instead
+                if len < 10 || len * 2 >= max_idx {
+                    return Some(max_idx);
+                }
+            }
+        } else {
+            let len = self.raw_len();
+            if len > 0 || self.has_array_metatable() {
+                return Some(len);
+            }
+            if options.encode_empty_tables_as_array && self.is_empty() {
+                return Some(0);
+            }
+        }
+        None
+    }
+
     #[cfg(feature = "luau")]
     #[inline(always)]
     fn check_readonly_write(&self, lua: &RawLua) -> Result<()> {
@@ -980,6 +1059,15 @@ impl<'a> SerializableTable<'a> {
     }
 }
 
+impl<V> TableSequence<'_, V> {
+    /// Sets the length (hint) of the sequence.
+    #[cfg(feature = "serde")]
+    pub(crate) fn with_len(mut self, len: usize) -> Self {
+        self.len = Some(len);
+        self
+    }
+}
+
 #[cfg(feature = "serde")]
 impl Serialize for SerializableTable<'_> {
     fn serialize<S>(&self, serializer: S) -> StdResult<S::Ok, S::Error>
@@ -1001,14 +1089,10 @@ impl Serialize for SerializableTable<'_> {
         let _guard = RecursionGuard::new(self.table, visited);
 
         // Array
-        let len = self.table.raw_len();
-        if len > 0
-            || self.table.is_array()
-            || (self.options.encode_empty_tables_as_array && self.table.is_empty())
-        {
+        if let Some(len) = self.table.encode_as_array(self.options) {
             let mut seq = serializer.serialize_seq(Some(len))?;
             let mut serialize_err = None;
-            let res = self.table.for_each_value::<Value>(|value| {
+            let res = self.table.for_each_value_by_len::<Value>(len, |value| {
                 let skip = check_value_for_skip(&value, self.options, visited)
                     .map_err(|err| Error::SerializeError(err.to_string()))?;
                 if skip {
@@ -1132,13 +1216,11 @@ pub struct TableSequence<'a, V> {
     guard: LuaGuard,
     table: &'a Table,
     index: Integer,
+    len: Option<usize>,
     _phantom: PhantomData<V>,
 }
 
-impl<V> Iterator for TableSequence<'_, V>
-where
-    V: FromLua,
-{
+impl<V: FromLua> Iterator for TableSequence<'_, V> {
     type Item = Result<V>;
 
     fn next(&mut self) -> Option<Self::Item> {
@@ -1152,7 +1234,7 @@ where
 
             lua.push_ref(&self.table.0);
             match ffi::lua_rawgeti(state, -1, self.index) {
-                ffi::LUA_TNIL => None,
+                ffi::LUA_TNIL if self.index as usize > self.len.unwrap_or(0) => None,
                 _ => {
                     self.index += 1;
                     Some(V::from_stack(-1, lua))

src/value.rs

@@ -717,6 +717,17 @@ impl<'a> SerializableValue<'a> {
         self.options.encode_empty_tables_as_array = enabled;
         self
     }
+
+    /// If true, enable detection of mixed tables.
+    ///
+    /// A mixed table is a table that has both array-like and map-like entries or several borders.
+    ///
+    /// Default: **false**
+    #[must_use]
+    pub const fn detect_mixed_tables(mut self, enabled: bool) -> Self {
+        self.options.detect_mixed_tables = enabled;
+        self
+    }
 }
 
 #[cfg(feature = "serde")]

tests/serde.rs

@@ -269,6 +269,45 @@ fn test_serialize_empty_table() -> LuaResult<()> {
     Ok(())
 }
 
+#[test]
+fn test_serialize_mixed_table() -> LuaResult<()> {
+    let lua = Lua::new();
+
+    // Check that sparse array is serialized similarly when using direct serialization
+    // and via `Lua::from_value`
+    let table = lua.load("{1,2,3,nil,5}").eval::<Value>()?;
+    let json1 = serde_json::to_string(&table).unwrap();
+    let json2 = lua.from_value::<serde_json::Value>(table)?;
+    assert_eq!(json1, json2.to_string());
+
+    // A table with several borders should be correctly encoded when `detect_mixed_tables` is enabled
+    let table = lua
+        .load(
+            r#"
+        local t = {1,2,3,nil,5,6}
+        t[10] = 10
+        return t
+    "#,
+        )
+        .eval::<Value>()?;
+    let json = serde_json::to_string(&table.to_serializable().detect_mixed_tables(true)).unwrap();
+    assert_eq!(json, r#"[1,2,3,null,5,6,null,null,null,10]"#);
+
+    // A mixed table with both array-like and map-like entries
+    let table = lua.load(r#"{1,2,3, key="value"}"#).eval::<Value>()?;
+    let json = serde_json::to_string(&table).unwrap();
+    assert_eq!(json, r#"[1,2,3]"#);
+    let json = serde_json::to_string(&table.to_serializable().detect_mixed_tables(true)).unwrap();
+    assert_eq!(json, r#"{"1":1,"2":2,"3":3,"key":"value"}"#);
+
+    // A mixed table with duplicate keys of different types
+    let table = lua.load(r#"{1,2,3, ["1"]="value"}"#).eval::<Value>()?;
+    let json = serde_json::to_string(&table.to_serializable().detect_mixed_tables(true)).unwrap();
+    assert_eq!(json, r#"{"1":1,"2":2,"3":3,"1":"value"}"#);
+
+    Ok(())
+}
+
 #[test]
 fn test_to_value_struct() -> LuaResult<()> {
     let lua = Lua::new();