From 4134a95878be83bf45b862ac4c2a48982373c358 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:59:06 -0600 Subject: [PATCH 01/16] test(path): Switch to snapbox --- src/path/parser.rs | 80 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 14 deletions(-) diff --git a/src/path/parser.rs b/src/path/parser.rs index dcd0f284..f4246e0c 100644 --- a/src/path/parser.rs +++ b/src/path/parser.rs @@ -93,53 +93,105 @@ fn integer(i: &mut &str) -> PResult { #[cfg(test)] mod test { + use snapbox::prelude::*; use snapbox::{assert_data_eq, str}; - use super::Expression::*; use super::*; #[test] fn test_id() { let parsed: Expression = from_str("abcd").unwrap(); - assert_eq!(parsed, Identifier("abcd".into())); + assert_data_eq!( + parsed.to_debug(), + str![[r#" +Identifier( + "abcd", +) + +"#]] + ); } #[test] fn test_id_dash() { let parsed: Expression = from_str("abcd-efgh").unwrap(); - assert_eq!(parsed, Identifier("abcd-efgh".into())); + assert_data_eq!( + parsed.to_debug(), + str![[r#" +Identifier( + "abcd-efgh", +) + +"#]] + ); } #[test] fn test_child() { let parsed: Expression = from_str("abcd.efgh").unwrap(); - let expected = Child(Box::new(Identifier("abcd".into())), "efgh".into()); + assert_data_eq!( + parsed.to_debug(), + str![[r#" +Child( + Identifier( + "abcd", + ), + "efgh", +) - assert_eq!(parsed, expected); +"#]] + ); let parsed: Expression = from_str("abcd.efgh.ijkl").unwrap(); - let expected = Child( - Box::new(Child(Box::new(Identifier("abcd".into())), "efgh".into())), - "ijkl".into(), - ); + assert_data_eq!( + parsed.to_debug(), + str![[r#" +Child( + Child( + Identifier( + "abcd", + ), + "efgh", + ), + "ijkl", +) - assert_eq!(parsed, expected); +"#]] + ); } #[test] fn test_subscript() { let parsed: Expression = from_str("abcd[12]").unwrap(); - let expected = Subscript(Box::new(Identifier("abcd".into())), 12); + assert_data_eq!( + parsed.to_debug(), + str![[r#" +Subscript( + Identifier( + "abcd", + ), + 12, +) - assert_eq!(parsed, expected); +"#]] + ); } #[test] fn test_subscript_neg() { let parsed: Expression = from_str("abcd[-1]").unwrap(); - let expected = Subscript(Box::new(Identifier("abcd".into())), -1); + assert_data_eq!( + parsed.to_debug(), + str![[r#" +Subscript( + Identifier( + "abcd", + ), + -1, +) - assert_eq!(parsed, expected); +"#]] + ); } #[test] From c855e4fc594c773b9bed2a89f284dbccfe742122 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 11:38:20 -0600 Subject: [PATCH 02/16] refactor(path): Avoid Expression implementation details --- src/path/mod.rs | 6 ++++++ src/source.rs | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index f96abb4f..1faae617 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -13,6 +13,12 @@ pub(crate) enum Expression { Subscript(Box, isize), } +impl Expression { + pub(crate) fn root(root: String) -> Self { + Expression::Identifier(root) + } +} + impl FromStr for Expression { type Err = ConfigError; diff --git a/src/source.rs b/src/source.rs index b68e3950..c11bae5e 100644 --- a/src/source.rs +++ b/src/source.rs @@ -33,7 +33,7 @@ fn set_value(cache: &mut Value, key: &str, value: &Value) { Ok(expr) => expr.set(cache, value.clone()), // Set directly anyway - _ => path::Expression::Identifier(key.to_owned()).set(cache, value.clone()), + _ => path::Expression::root(key.to_owned()).set(cache, value.clone()), } } From 6abd1c9526635da15dce4a252b5c63a2b2f3b429 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 12:16:31 -0600 Subject: [PATCH 03/16] fix(set): Correctly handle negative indices on non-leaf --- src/path/mod.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 1faae617..31fa681b 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -145,13 +145,23 @@ impl Expression { match value.kind { ValueKind::Array(ref mut array) => { - let index = abs_index(index, array.len()).ok()?; - - if index >= array.len() { - array.resize(index + 1, Value::new(None, ValueKind::Nil)); - } + let uindex = match abs_index(index, array.len()) { + Ok(uindex) => { + if uindex >= array.len() { + array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); + } + uindex + } + Err(insertion) => { + array.splice( + 0..0, + (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), + ); + 0 + } + }; - Some(&mut array[index]) + Some(&mut array[uindex]) } _ => None, From d3dd1bb1b081cd2de83fb1968165b9fc9df185db Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 12:36:42 -0600 Subject: [PATCH 04/16] refactor(path): Clarify a case is unreachable --- src/path/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 31fa681b..5563e86f 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -164,7 +164,7 @@ impl Expression { Some(&mut array[uindex]) } - _ => None, + _ => unreachable!(), } } _ => None, From aa3922c6e48fb2f3ac08a9bc356865d823eee3c2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 12:44:47 -0600 Subject: [PATCH 05/16] refactor(path): Explicitly mark unreachable branches --- src/path/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/path/mod.rs b/src/path/mod.rs index 5563e86f..aef7a49a 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -208,6 +208,8 @@ impl Expression { } else { map.insert(id.clone(), value); } + } else { + unreachable!(); } } } @@ -247,6 +249,8 @@ impl Expression { }; array[uindex] = value; + } else { + unreachable!() } } } From dcb17d0e67ae28c6b41b9ac1545004aa2d7168fd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 12:45:40 -0600 Subject: [PATCH 06/16] refactor(path): Be consistent in overriding entries --- src/path/mod.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index aef7a49a..65f12682 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -138,9 +138,8 @@ impl Expression { Self::Subscript(ref expr, index) => match expr.get_mut_forcibly(root) { Some(value) => { - match value.kind { - ValueKind::Array(_) => (), - _ => *value = Vec::::new().into(), + if !matches!(value.kind, ValueKind::Array(_)) { + *value = Vec::::new().into(); } match value.kind { @@ -176,12 +175,8 @@ impl Expression { match *self { Self::Identifier(ref id) => { // Ensure that root is a table - match root.kind { - ValueKind::Table(_) => {} - - _ => { - *root = Map::::new().into(); - } + if !matches!(root.kind, ValueKind::Table(_)) { + *root = Map::::new().into(); } match value.kind { From 4e1afb003471226fd0ab9e2c0f91b3ab51167598 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 12:46:15 -0600 Subject: [PATCH 07/16] refactor(path): Consistently use and `if` Less nesting --- src/path/mod.rs | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 65f12682..2a650b1c 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -142,28 +142,26 @@ impl Expression { *value = Vec::::new().into(); } - match value.kind { - ValueKind::Array(ref mut array) => { - let uindex = match abs_index(index, array.len()) { - Ok(uindex) => { - if uindex >= array.len() { - array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); - } - uindex - } - Err(insertion) => { - array.splice( - 0..0, - (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), - ); - 0 + if let ValueKind::Array(ref mut array) = value.kind { + let uindex = match abs_index(index, array.len()) { + Ok(uindex) => { + if uindex >= array.len() { + array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); } - }; - - Some(&mut array[uindex]) - } + uindex + } + Err(insertion) => { + array.splice( + 0..0, + (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), + ); + 0 + } + }; - _ => unreachable!(), + Some(&mut array[uindex]) + } else { + unreachable!() } } _ => None, From 2193da2551d842f403352048084d3187f0198051 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 12:46:43 -0600 Subject: [PATCH 08/16] refactor(path): Don't duplicate lookup logic --- src/path/mod.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 2a650b1c..430d2499 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -114,22 +114,17 @@ impl Expression { Self::Child(ref expr, ref key) => match expr.get_mut_forcibly(root) { Some(value) => { + if !matches!(value.kind, ValueKind::Table(_)) { + *value = Map::::new().into(); + } + if let ValueKind::Table(ref mut map) = value.kind { Some( map.entry(key.clone()) .or_insert_with(|| Value::new(None, ValueKind::Nil)), ) } else { - *value = Map::::new().into(); - - if let ValueKind::Table(ref mut map) = value.kind { - Some( - map.entry(key.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)), - ) - } else { - unreachable!(); - } + unreachable!() } } From 5844d415bcb4815a6a16023e615042636cd08ce0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:00:30 -0600 Subject: [PATCH 09/16] fix(path): Ensure we always postfix --- src/path/mod.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 430d2499..77b55608 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -103,14 +103,20 @@ impl Expression { pub(crate) fn get_mut_forcibly<'a>(&self, root: &'a mut Value) -> Option<&'a mut Value> { match *self { - Self::Identifier(ref id) => match root.kind { - ValueKind::Table(ref mut map) => Some( - map.entry(id.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)), - ), + Self::Identifier(ref id) => { + if !matches!(root.kind, ValueKind::Table(_)) { + *root = Map::::new().into(); + } - _ => None, - }, + if let ValueKind::Table(ref mut map) = root.kind { + Some( + map.entry(id.clone()) + .or_insert_with(|| Value::new(None, ValueKind::Nil)), + ) + } else { + unreachable!() + } + } Self::Child(ref expr, ref key) => match expr.get_mut_forcibly(root) { Some(value) => { From 3b46d76215c070ecf151d525cacb5ecb7d87f44f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:02:11 -0600 Subject: [PATCH 10/16] refactor(path): Remove unused Option --- src/path/mod.rs | 141 ++++++++++++++++++++++-------------------------- 1 file changed, 65 insertions(+), 76 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 77b55608..8131cc72 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -101,7 +101,7 @@ impl Expression { } } - pub(crate) fn get_mut_forcibly<'a>(&self, root: &'a mut Value) -> Option<&'a mut Value> { + pub(crate) fn get_mut_forcibly<'a>(&self, root: &'a mut Value) -> &'a mut Value { match *self { Self::Identifier(ref id) => { if !matches!(root.kind, ValueKind::Table(_)) { @@ -109,64 +109,55 @@ impl Expression { } if let ValueKind::Table(ref mut map) = root.kind { - Some( - map.entry(id.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)), - ) + map.entry(id.clone()) + .or_insert_with(|| Value::new(None, ValueKind::Nil)) } else { unreachable!() } } - Self::Child(ref expr, ref key) => match expr.get_mut_forcibly(root) { - Some(value) => { - if !matches!(value.kind, ValueKind::Table(_)) { - *value = Map::::new().into(); - } - - if let ValueKind::Table(ref mut map) = value.kind { - Some( - map.entry(key.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)), - ) - } else { - unreachable!() - } + Self::Child(ref expr, ref key) => { + let value = expr.get_mut_forcibly(root); + if !matches!(value.kind, ValueKind::Table(_)) { + *value = Map::::new().into(); } - _ => None, - }, + if let ValueKind::Table(ref mut map) = value.kind { + map.entry(key.clone()) + .or_insert_with(|| Value::new(None, ValueKind::Nil)) + } else { + unreachable!() + } + } - Self::Subscript(ref expr, index) => match expr.get_mut_forcibly(root) { - Some(value) => { - if !matches!(value.kind, ValueKind::Array(_)) { - *value = Vec::::new().into(); - } + Self::Subscript(ref expr, index) => { + let value = expr.get_mut_forcibly(root); + if !matches!(value.kind, ValueKind::Array(_)) { + *value = Vec::::new().into(); + } - if let ValueKind::Array(ref mut array) = value.kind { - let uindex = match abs_index(index, array.len()) { - Ok(uindex) => { - if uindex >= array.len() { - array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); - } - uindex + if let ValueKind::Array(ref mut array) = value.kind { + let uindex = match abs_index(index, array.len()) { + Ok(uindex) => { + if uindex >= array.len() { + array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); } - Err(insertion) => { - array.splice( - 0..0, - (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), - ); - 0 - } - }; + uindex + } + Err(insertion) => { + array.splice( + 0..0, + (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), + ); + 0 + } + }; - Some(&mut array[uindex]) - } else { - unreachable!() - } + &mut array[uindex] + } else { + unreachable!() } - _ => None, - }, + } } } @@ -210,42 +201,40 @@ impl Expression { } Self::Child(ref expr, ref key) => { - if let Some(parent) = expr.get_mut_forcibly(root) { - if !matches!(parent.kind, ValueKind::Table(_)) { - // Didn't find a table. Oh well. Make a table and do this anyway - *parent = Map::::new().into(); - } - Self::Identifier(key.clone()).set(parent, value); + let parent = expr.get_mut_forcibly(root); + if !matches!(parent.kind, ValueKind::Table(_)) { + // Didn't find a table. Oh well. Make a table and do this anyway + *parent = Map::::new().into(); } + Self::Identifier(key.clone()).set(parent, value); } Self::Subscript(ref expr, index) => { - if let Some(parent) = expr.get_mut_forcibly(root) { - if !matches!(parent.kind, ValueKind::Array(_)) { - *parent = Vec::::new().into(); - } + let parent = expr.get_mut_forcibly(root); + if !matches!(parent.kind, ValueKind::Array(_)) { + *parent = Vec::::new().into(); + } - if let ValueKind::Array(ref mut array) = parent.kind { - let uindex = match abs_index(index, array.len()) { - Ok(uindex) => { - if uindex >= array.len() { - array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); - } - uindex - } - Err(insertion) => { - array.splice( - 0..0, - (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), - ); - 0 + if let ValueKind::Array(ref mut array) = parent.kind { + let uindex = match abs_index(index, array.len()) { + Ok(uindex) => { + if uindex >= array.len() { + array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); } - }; + uindex + } + Err(insertion) => { + array.splice( + 0..0, + (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), + ); + 0 + } + }; - array[uindex] = value; - } else { - unreachable!() - } + array[uindex] = value; + } else { + unreachable!() } } } From 8b074cb2900c1648e88da36653b1bdbe12e9e23e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:07:31 -0600 Subject: [PATCH 11/16] refactor(path): Consolidate table checks --- src/path/mod.rs | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 8131cc72..8a390e33 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -169,34 +169,31 @@ impl Expression { *root = Map::::new().into(); } - match value.kind { - ValueKind::Table(ref incoming_map) => { - // Pull out another table - let target = if let ValueKind::Table(ref mut map) = root.kind { - map.entry(id.clone()) - .or_insert_with(|| Map::::new().into()) - } else { - unreachable!(); - }; - - // Continue the deep merge - for (key, val) in incoming_map { - Self::Identifier(key.clone()).set(target, val.clone()); + if let ValueKind::Table(ref mut map) = root.kind { + match value.kind { + ValueKind::Table(ref incoming_map) => { + // Pull out another table + let target = map + .entry(id.clone()) + .or_insert_with(|| Map::::new().into()); + + // Continue the deep merge + for (key, val) in incoming_map { + Self::Identifier(key.clone()).set(target, val.clone()); + } } - } - _ => { - if let ValueKind::Table(ref mut map) = root.kind { + _ => { // Just do a simple set if let Some(existing) = map.get_mut(id) { *existing = value; } else { map.insert(id.clone(), value); } - } else { - unreachable!(); } } + } else { + unreachable!(); } } From ec36bff45eb339077e23d47d2edabc0bcd03cf6c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:10:55 -0600 Subject: [PATCH 12/16] refactor(path): Fully delegate lookup to get_mut_forcibly --- src/path/mod.rs | 76 ++++++------------------------------------------- 1 file changed, 8 insertions(+), 68 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 8a390e33..3f693ecf 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -162,77 +162,17 @@ impl Expression { } pub(crate) fn set(&self, root: &mut Value, value: Value) { - match *self { - Self::Identifier(ref id) => { - // Ensure that root is a table - if !matches!(root.kind, ValueKind::Table(_)) { - *root = Map::::new().into(); - } - - if let ValueKind::Table(ref mut map) = root.kind { - match value.kind { - ValueKind::Table(ref incoming_map) => { - // Pull out another table - let target = map - .entry(id.clone()) - .or_insert_with(|| Map::::new().into()); - - // Continue the deep merge - for (key, val) in incoming_map { - Self::Identifier(key.clone()).set(target, val.clone()); - } - } - - _ => { - // Just do a simple set - if let Some(existing) = map.get_mut(id) { - *existing = value; - } else { - map.insert(id.clone(), value); - } - } - } - } else { - unreachable!(); - } - } - - Self::Child(ref expr, ref key) => { - let parent = expr.get_mut_forcibly(root); - if !matches!(parent.kind, ValueKind::Table(_)) { - // Didn't find a table. Oh well. Make a table and do this anyway - *parent = Map::::new().into(); + let parent = self.get_mut_forcibly(root); + match value.kind { + ValueKind::Table(ref incoming_map) => { + // Continue the deep merge + for (key, val) in incoming_map { + Self::root(key.clone()).set(parent, val.clone()); } - Self::Identifier(key.clone()).set(parent, value); } - Self::Subscript(ref expr, index) => { - let parent = expr.get_mut_forcibly(root); - if !matches!(parent.kind, ValueKind::Array(_)) { - *parent = Vec::::new().into(); - } - - if let ValueKind::Array(ref mut array) = parent.kind { - let uindex = match abs_index(index, array.len()) { - Ok(uindex) => { - if uindex >= array.len() { - array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); - } - uindex - } - Err(insertion) => { - array.splice( - 0..0, - (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), - ); - 0 - } - }; - - array[uindex] = value; - } else { - unreachable!() - } + _ => { + *parent = value; } } } From 02e62db4f57004bd6afe9ff58671bfa6e537282c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:16:38 -0600 Subject: [PATCH 13/16] refactor(path): Make lookups more consistent --- src/path/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 3f693ecf..25b317d1 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -60,10 +60,10 @@ fn abs_index(index: isize, len: usize) -> Result { impl Expression { pub(crate) fn get(self, root: &Value) -> Option<&Value> { match self { - Self::Identifier(id) => { + Self::Identifier(key) => { match root.kind { // `x` access on a table is equivalent to: map[x] - ValueKind::Table(ref map) => map.get(&id), + ValueKind::Table(ref map) => map.get(&key), // all other variants return None _ => None, @@ -103,13 +103,13 @@ impl Expression { pub(crate) fn get_mut_forcibly<'a>(&self, root: &'a mut Value) -> &'a mut Value { match *self { - Self::Identifier(ref id) => { + Self::Identifier(ref key) => { if !matches!(root.kind, ValueKind::Table(_)) { *root = Map::::new().into(); } if let ValueKind::Table(ref mut map) = root.kind { - map.entry(id.clone()) + map.entry(key.clone()) .or_insert_with(|| Value::new(None, ValueKind::Nil)) } else { unreachable!() From 8323fa2a0e7aa4deb8de85b9c2053e2659c6dfa4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:18:07 -0600 Subject: [PATCH 14/16] refactor(path): Make names more consistent --- src/path/mod.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 25b317d1..0f87b878 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -72,8 +72,8 @@ impl Expression { Self::Child(expr, key) => { match expr.get(root) { - Some(value) => { - match value.kind { + Some(child) => { + match child.kind { // Access on a table is identical to Identifier, it just forwards ValueKind::Table(ref map) => map.get(&key), @@ -87,7 +87,7 @@ impl Expression { } Self::Subscript(expr, index) => match expr.get(root) { - Some(value) => match value.kind { + Some(child) => match child.kind { ValueKind::Array(ref array) => { let index = abs_index(index, array.len()).ok()?; array.get(index) @@ -117,12 +117,12 @@ impl Expression { } Self::Child(ref expr, ref key) => { - let value = expr.get_mut_forcibly(root); - if !matches!(value.kind, ValueKind::Table(_)) { - *value = Map::::new().into(); + let child = expr.get_mut_forcibly(root); + if !matches!(child.kind, ValueKind::Table(_)) { + *child = Map::::new().into(); } - if let ValueKind::Table(ref mut map) = value.kind { + if let ValueKind::Table(ref mut map) = child.kind { map.entry(key.clone()) .or_insert_with(|| Value::new(None, ValueKind::Nil)) } else { @@ -131,12 +131,12 @@ impl Expression { } Self::Subscript(ref expr, index) => { - let value = expr.get_mut_forcibly(root); - if !matches!(value.kind, ValueKind::Array(_)) { - *value = Vec::::new().into(); + let child = expr.get_mut_forcibly(root); + if !matches!(child.kind, ValueKind::Array(_)) { + *child = Vec::::new().into(); } - if let ValueKind::Array(ref mut array) = value.kind { + if let ValueKind::Array(ref mut array) = child.kind { let uindex = match abs_index(index, array.len()) { Ok(uindex) => { if uindex >= array.len() { From c397a10cde368d4184dd0850b36dfb633317942f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:22:58 -0600 Subject: [PATCH 15/16] refactor(path): Flatten with let-else --- src/path/mod.rs | 61 ++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 0f87b878..db79a813 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -107,56 +107,55 @@ impl Expression { if !matches!(root.kind, ValueKind::Table(_)) { *root = Map::::new().into(); } - - if let ValueKind::Table(ref mut map) = root.kind { - map.entry(key.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)) - } else { + let ValueKind::Table(ref mut map) = root.kind else { unreachable!() - } + }; + + map.entry(key.clone()) + .or_insert_with(|| Value::new(None, ValueKind::Nil)) } Self::Child(ref expr, ref key) => { let child = expr.get_mut_forcibly(root); + if !matches!(child.kind, ValueKind::Table(_)) { *child = Map::::new().into(); } - - if let ValueKind::Table(ref mut map) = child.kind { - map.entry(key.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)) - } else { + let ValueKind::Table(ref mut map) = child.kind else { unreachable!() - } + }; + + map.entry(key.clone()) + .or_insert_with(|| Value::new(None, ValueKind::Nil)) } Self::Subscript(ref expr, index) => { let child = expr.get_mut_forcibly(root); + if !matches!(child.kind, ValueKind::Array(_)) { *child = Vec::::new().into(); } + let ValueKind::Array(ref mut array) = child.kind else { + unreachable!() + }; - if let ValueKind::Array(ref mut array) = child.kind { - let uindex = match abs_index(index, array.len()) { - Ok(uindex) => { - if uindex >= array.len() { - array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); - } - uindex + let uindex = match abs_index(index, array.len()) { + Ok(uindex) => { + if uindex >= array.len() { + array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); } - Err(insertion) => { - array.splice( - 0..0, - (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), - ); - 0 - } - }; + uindex + } + Err(insertion) => { + array.splice( + 0..0, + (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), + ); + 0 + } + }; - &mut array[uindex] - } else { - unreachable!() - } + &mut array[uindex] } } } From 80ca240073c17eadab62bda64d0bbfb1aebdac48 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 13:25:01 -0600 Subject: [PATCH 16/16] refactor(path): Switch from recursive to iterative structures --- src/path/mod.rs | 168 +++++++++++++++++++++------------------------ src/path/parser.rs | 105 ++++++++++++++-------------- 2 files changed, 127 insertions(+), 146 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index db79a813..333a4d57 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -7,15 +7,17 @@ use crate::value::{Value, ValueKind}; mod parser; #[derive(Debug, Eq, PartialEq, Clone, Hash)] -pub(crate) enum Expression { - Identifier(String), - Child(Box, String), - Subscript(Box, isize), +pub(crate) struct Expression { + root: String, + postfix: Vec, } impl Expression { pub(crate) fn root(root: String) -> Self { - Expression::Identifier(root) + Self { + root, + postfix: Vec::new(), + } } } @@ -29,6 +31,12 @@ impl FromStr for Expression { } } +#[derive(Debug, Eq, PartialEq, Clone, Hash)] +enum Postfix { + Key(String), + Index(isize), +} + #[derive(Debug)] struct ParseError(String); @@ -59,105 +67,83 @@ fn abs_index(index: isize, len: usize) -> Result { impl Expression { pub(crate) fn get(self, root: &Value) -> Option<&Value> { - match self { - Self::Identifier(key) => { - match root.kind { - // `x` access on a table is equivalent to: map[x] - ValueKind::Table(ref map) => map.get(&key), - - // all other variants return None - _ => None, + let ValueKind::Table(map) = &root.kind else { + return None; + }; + let mut child = map.get(&self.root)?; + for postfix in &self.postfix { + match postfix { + Postfix::Key(key) => { + let ValueKind::Table(map) = &child.kind else { + return None; + }; + child = map.get(key)?; } - } - - Self::Child(expr, key) => { - match expr.get(root) { - Some(child) => { - match child.kind { - // Access on a table is identical to Identifier, it just forwards - ValueKind::Table(ref map) => map.get(&key), - - // all other variants return None - _ => None, - } - } - - _ => None, + Postfix::Index(rel_index) => { + let ValueKind::Array(array) = &child.kind else { + return None; + }; + let index = abs_index(*rel_index, array.len()).ok()?; + child = array.get(index)?; } } - - Self::Subscript(expr, index) => match expr.get(root) { - Some(child) => match child.kind { - ValueKind::Array(ref array) => { - let index = abs_index(index, array.len()).ok()?; - array.get(index) - } - - _ => None, - }, - - _ => None, - }, } + Some(child) } pub(crate) fn get_mut_forcibly<'a>(&self, root: &'a mut Value) -> &'a mut Value { - match *self { - Self::Identifier(ref key) => { - if !matches!(root.kind, ValueKind::Table(_)) { - *root = Map::::new().into(); - } - let ValueKind::Table(ref mut map) = root.kind else { - unreachable!() - }; - - map.entry(key.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)) - } - - Self::Child(ref expr, ref key) => { - let child = expr.get_mut_forcibly(root); - - if !matches!(child.kind, ValueKind::Table(_)) { - *child = Map::::new().into(); - } - let ValueKind::Table(ref mut map) = child.kind else { - unreachable!() - }; - - map.entry(key.clone()) - .or_insert_with(|| Value::new(None, ValueKind::Nil)) - } - - Self::Subscript(ref expr, index) => { - let child = expr.get_mut_forcibly(root); + if !matches!(root.kind, ValueKind::Table(_)) { + *root = Map::::new().into(); + } + let ValueKind::Table(map) = &mut root.kind else { + unreachable!() + }; + let mut child = map + .entry(self.root.clone()) + .or_insert_with(|| Value::new(None, ValueKind::Nil)); + for postfix in &self.postfix { + match postfix { + Postfix::Key(key) => { + if !matches!(child.kind, ValueKind::Table(_)) { + *child = Map::::new().into(); + } + let ValueKind::Table(ref mut map) = child.kind else { + unreachable!() + }; - if !matches!(child.kind, ValueKind::Array(_)) { - *child = Vec::::new().into(); + child = map + .entry(key.clone()) + .or_insert_with(|| Value::new(None, ValueKind::Nil)); } - let ValueKind::Array(ref mut array) = child.kind else { - unreachable!() - }; - - let uindex = match abs_index(index, array.len()) { - Ok(uindex) => { - if uindex >= array.len() { - array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); - } - uindex - } - Err(insertion) => { - array.splice( - 0..0, - (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), - ); - 0 + Postfix::Index(rel_index) => { + if !matches!(child.kind, ValueKind::Array(_)) { + *child = Vec::::new().into(); } - }; + let ValueKind::Array(ref mut array) = child.kind else { + unreachable!() + }; + + let uindex = match abs_index(*rel_index, array.len()) { + Ok(uindex) => { + if uindex >= array.len() { + array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); + } + uindex + } + Err(insertion) => { + array.splice( + 0..0, + (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), + ); + 0 + } + }; - &mut array[uindex] + child = &mut array[uindex]; + } } } + child } pub(crate) fn set(&self, root: &mut Value, value: Value) { diff --git a/src/path/parser.rs b/src/path/parser.rs index f4246e0c..3b45fb77 100644 --- a/src/path/parser.rs +++ b/src/path/parser.rs @@ -17,6 +17,7 @@ use winnow::token::any; use winnow::token::take_while; use crate::path::Expression; +use crate::path::Postfix; pub(crate) fn from_str(input: &str) -> Result> { path.parse(input) @@ -24,33 +25,22 @@ pub(crate) fn from_str(input: &str) -> Result PResult { let root = ident.parse_next(i)?; - let expr = repeat(0.., postfix) - .fold( - || root.clone(), - |prev, cur| match cur { - Child::Key(k) => Expression::Child(Box::new(prev), k), - Child::Index(k) => Expression::Subscript(Box::new(prev), k), - }, - ) - .parse_next(i)?; + let postfix = repeat(0.., postfix).parse_next(i)?; + let expr = Expression { root, postfix }; Ok(expr) } -fn ident(i: &mut &str) -> PResult { - raw_ident.map(Expression::Identifier).parse_next(i) -} - -fn postfix(i: &mut &str) -> PResult { +fn postfix(i: &mut &str) -> PResult { dispatch! {any; '[' => cut_err( seq!( - integer.map(Child::Index), + integer.map(Postfix::Index), _: ']'.context(StrContext::Expected(StrContextValue::CharLiteral(']'))), ) .map(|(i,)| i) .context(StrContext::Label("subscript")) ), - '.' => cut_err(raw_ident.map(Child::Key)), + '.' => cut_err(ident.map(Postfix::Key)), _ => cut_err( fail .context(StrContext::Label("postfix")) @@ -61,14 +51,9 @@ fn postfix(i: &mut &str) -> PResult { .parse_next(i) } -enum Child { - Key(String), - Index(isize), -} - -fn raw_ident(i: &mut &str) -> PResult { +fn ident(i: &mut &str) -> PResult { take_while(1.., ('a'..='z', 'A'..='Z', '0'..='9', '_', '-')) - .map(ToString::to_string) + .map(ToOwned::to_owned) .context(StrContext::Label("identifier")) .context(StrContext::Expected(StrContextValue::Description( "ASCII alphanumeric", @@ -104,9 +89,10 @@ mod test { assert_data_eq!( parsed.to_debug(), str![[r#" -Identifier( - "abcd", -) +Expression { + root: "abcd", + postfix: [], +} "#]] ); @@ -118,9 +104,10 @@ Identifier( assert_data_eq!( parsed.to_debug(), str![[r#" -Identifier( - "abcd-efgh", -) +Expression { + root: "abcd-efgh", + postfix: [], +} "#]] ); @@ -132,12 +119,14 @@ Identifier( assert_data_eq!( parsed.to_debug(), str![[r#" -Child( - Identifier( - "abcd", - ), - "efgh", -) +Expression { + root: "abcd", + postfix: [ + Key( + "efgh", + ), + ], +} "#]] ); @@ -146,15 +135,17 @@ Child( assert_data_eq!( parsed.to_debug(), str![[r#" -Child( - Child( - Identifier( - "abcd", +Expression { + root: "abcd", + postfix: [ + Key( + "efgh", + ), + Key( + "ijkl", ), - "efgh", - ), - "ijkl", -) + ], +} "#]] ); @@ -166,12 +157,14 @@ Child( assert_data_eq!( parsed.to_debug(), str![[r#" -Subscript( - Identifier( - "abcd", - ), - 12, -) +Expression { + root: "abcd", + postfix: [ + Index( + 12, + ), + ], +} "#]] ); @@ -183,12 +176,14 @@ Subscript( assert_data_eq!( parsed.to_debug(), str![[r#" -Subscript( - Identifier( - "abcd", - ), - -1, -) +Expression { + root: "abcd", + postfix: [ + Index( + -1, + ), + ], +} "#]] );