fix: Remove some recursion to avoid overflow (#213)

This is a part of #206. This wasn't even my goal but to work out what was going on with lifetimes which led to the loosening of lifetimes on `*Table::entry_format`. Hopefully this will also help with performance.
toml-rs · Sep 24, 2021 · bb444ea · bb444ea
1 parent 41b4527
commit bb444ea
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 34 deletions.
diff --git a/src/inline_table.rs b/src/inline_table.rs
@@ -180,7 +180,7 @@ impl InlineTable {
     }
 
     /// Gets the given key's corresponding entry in the Table for in-place manipulation.
-    pub fn entry_format<'a>(&'a mut self, key: &'a Key) -> InlineEntry<'a> {
+    pub fn entry_format<'a>(&'a mut self, key: &Key) -> InlineEntry<'a> {
         // Accept a `&Key` to be consistent with `entry`
         match self.items.entry(key.get().into()) {
             indexmap::map::Entry::Occupied(mut entry) => {
@@ -200,7 +200,7 @@ impl InlineTable {
             }
             indexmap::map::Entry::Vacant(entry) => InlineEntry::Vacant(InlineVacantEntry {
                 entry,
-                key: Some(key),
+                key: Some(key.clone()),
             }),
         }
     }
@@ -498,7 +498,7 @@ impl<'a> InlineOccupiedEntry<'a> {
 /// A view into a single empty location in a `IndexMap`.
 pub struct InlineVacantEntry<'a> {
     entry: indexmap::map::VacantEntry<'a, InternalString, TableKeyValue>,
-    key: Option<&'a Key>,
+    key: Option<Key>,
 }
 
 impl<'a> InlineVacantEntry<'a> {
@@ -520,9 +520,10 @@ impl<'a> InlineVacantEntry<'a> {
     /// Sets the value of the entry with the VacantEntry's key,
     /// and returns a mutable reference to it
     pub fn insert(self, value: Value) -> &'a mut Value {
-        let key = self.key.cloned().unwrap_or_else(|| Key::new(self.key()));
+        let entry = self.entry;
+        let key = self.key.unwrap_or_else(|| Key::new(entry.key().as_str()));
         let value = Item::Value(value);
-        self.entry
+        entry
             .insert(TableKeyValue::new(key, value))
             .value
             .as_value_mut()

diff --git a/src/parser/document.rs b/src/parser/document.rs
@@ -140,7 +140,7 @@ impl TomlParser {
         }
 
         let table = &mut self.current_table;
-        let table = Self::descend_path(table, &path, 0, true)?;
+        let table = Self::descend_path(table, &path, true)?;
 
         // "Since tables cannot be defined more than once, redefining such tables using a [table] header is not allowed"
         let duplicate_key = table.contains_key(kv.key.get());

diff --git a/src/parser/inline_table.rs b/src/parser/inline_table.rs
@@ -28,7 +28,7 @@ fn table_from_pairs(
     root.items.reserve(v.len());
 
     for (path, kv) in v {
-        let table = descend_path(&mut root, &path, 0)?;
+        let table = descend_path(&mut root, &path)?;
         if table.contains_key(kv.key.get()) {
             return Err(CustomError::DuplicateKey {
                 key: kv.key.get().into(),
@@ -41,11 +41,10 @@ fn table_from_pairs(
 }
 
 fn descend_path<'a>(
-    table: &'a mut InlineTable,
+    mut table: &'a mut InlineTable,
     path: &'a [Key],
-    i: usize,
 ) -> Result<&'a mut InlineTable, CustomError> {
-    if let Some(key) = path.get(i) {
+    for (i, key) in path.iter().enumerate() {
         let entry = table.entry_format(key).or_insert_with(|| {
             let mut new_table = InlineTable::new();
             new_table.set_dotted(true);
@@ -54,13 +53,14 @@ fn descend_path<'a>(
         });
         match *entry {
             Value::InlineTable(ref mut sweet_child_of_mine) => {
-                descend_path(sweet_child_of_mine, path, i + 1)
+                table = sweet_child_of_mine;
+            }
+            _ => {
+                return Err(duplicate_key(path, i));
             }
-            _ => Err(duplicate_key(path, i)),
         }
-    } else {
-        Ok(table)
     }
+    Ok(table)
 }
 
 // inline-table-open  = %x7B ws     ; {

diff --git a/src/parser/mod.rs b/src/parser/mod.rs
@@ -47,7 +47,7 @@ impl TomlParser {
 
         // Look up the table on start to ensure the duplicate_key error points to the right line
         let root = self.document.as_table_mut();
-        let parent_table = Self::descend_path(root, &path[..path.len() - 1], 0, false)?;
+        let parent_table = Self::descend_path(root, &path[..path.len() - 1], false)?;
         let key = &path[path.len() - 1];
         let entry = parent_table
             .entry_format(key)
@@ -72,7 +72,7 @@ impl TomlParser {
         // 1. Look up the table on start to ensure the duplicate_key error points to the right line
         // 2. Ensure any child tables from an implicit table are preserved
         let root = self.document.as_table_mut();
-        let parent_table = Self::descend_path(root, &path[..path.len() - 1], 0, false)?;
+        let parent_table = Self::descend_path(root, &path[..path.len() - 1], false)?;
         let key = &path[path.len() - 1];
         if let Some(entry) = parent_table.remove(key.get()) {
             match entry {
@@ -101,7 +101,7 @@ impl TomlParser {
             assert!(root.is_empty());
             std::mem::swap(&mut table, root);
         } else if self.current_is_array {
-            let parent_table = Self::descend_path(root, &path[..path.len() - 1], 0, false)?;
+            let parent_table = Self::descend_path(root, &path[..path.len() - 1], false)?;
             let key = &path[path.len() - 1];
 
             let entry = parent_table
@@ -112,7 +112,7 @@ impl TomlParser {
                 .ok_or_else(|| duplicate_key(&path, path.len() - 1))?;
             array.push(table);
         } else {
-            let parent_table = Self::descend_path(root, &path[..path.len() - 1], 0, false)?;
+            let parent_table = Self::descend_path(root, &path[..path.len() - 1], false)?;
             let key = &path[path.len() - 1];
 
             let entry = parent_table.entry_format(key);

diff --git a/src/parser/table.rs b/src/parser/table.rs
@@ -82,13 +82,12 @@ pub(crate) fn duplicate_key(path: &[Key], i: usize) -> CustomError {
 }
 
 impl TomlParser {
-    pub(crate) fn descend_path<'a>(
-        table: &'a mut Table,
-        path: &'a [Key],
-        i: usize,
+    pub(crate) fn descend_path<'t, 'k>(
+        mut table: &'t mut Table,
+        path: &'k [Key],
         dotted: bool,
-    ) -> Result<&'a mut Table, CustomError> {
-        if let Some(key) = path.get(i) {
+    ) -> Result<&'t mut Table, CustomError> {
+        for (i, key) in path.iter().enumerate() {
             let entry = table.entry_format(key).or_insert_with(|| {
                 let mut new_table = Table::new();
                 new_table.set_implicit(true);
@@ -97,23 +96,24 @@ impl TomlParser {
                 Item::Table(new_table)
             });
             match *entry {
-                Item::Value(..) => Err(duplicate_key(path, i)),
+                Item::Value(..) => {
+                    return Err(duplicate_key(path, i));
+                }
                 Item::ArrayOfTables(ref mut array) => {
                     debug_assert!(!array.is_empty());
 
                     let index = array.len() - 1;
                     let last_child = array.get_mut(index).unwrap();
 
-                    Self::descend_path(last_child, path, i + 1, dotted)
+                    table = last_child;
                 }
                 Item::Table(ref mut sweet_child_of_mine) => {
-                    TomlParser::descend_path(sweet_child_of_mine, path, i + 1, dotted)
+                    table = sweet_child_of_mine;
                 }
                 _ => unreachable!(),
             }
-        } else {
-            Ok(table)
         }
+        Ok(table)
     }
 
     fn on_std_header(&mut self, path: Vec1<Key>, trailing: &str) -> Result<(), CustomError> {

diff --git a/src/table.rs b/src/table.rs
@@ -213,13 +213,13 @@ impl Table {
     }
 
     /// Gets the given key's corresponding entry in the Table for in-place manipulation.
-    pub fn entry_format<'a>(&'a mut self, key: &'a Key) -> Entry<'a> {
+    pub fn entry_format<'a>(&'a mut self, key: &Key) -> Entry<'a> {
         // Accept a `&Key` to be consistent with `entry`
         match self.items.entry(key.get().into()) {
             indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
             indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry {
                 entry,
-                key: Some(key),
+                key: Some(key.to_owned()),
             }),
         }
     }
@@ -568,7 +568,7 @@ impl<'a> OccupiedEntry<'a> {
 /// A view into a single empty location in a `IndexMap`.
 pub struct VacantEntry<'a> {
     entry: indexmap::map::VacantEntry<'a, InternalString, TableKeyValue>,
-    key: Option<&'a Key>,
+    key: Option<Key>,
 }
 
 impl<'a> VacantEntry<'a> {
@@ -590,7 +590,8 @@ impl<'a> VacantEntry<'a> {
     /// Sets the value of the entry with the VacantEntry's key,
     /// and returns a mutable reference to it
     pub fn insert(self, value: Item) -> &'a mut Item {
-        let key = self.key.cloned().unwrap_or_else(|| Key::new(self.key()));
-        &mut self.entry.insert(TableKeyValue::new(key, value)).value
+        let entry = self.entry;
+        let key = self.key.unwrap_or_else(|| Key::new(entry.key().as_str()));
+        &mut entry.insert(TableKeyValue::new(key, value)).value
     }
 }