[PATCH 0/2] Implement serde::Deserialize for Value

[PATCH 0/2] Implement serde::Deserialize for Value

[Ryan Lahfa]

As per IRC discussions, this tries to bring an impl of
serde::Deserialize for Value.

Various questions are:

- Do we want to keep the ugly #[serde(skip)] for the other values?
  - I think it's quite neat to know it _should be possible_ to serialize
    as-is a Value structure, but I feel like there's two policies of
    serialization.
    A public-facing one and an internal one.
- Do we want to introduce `serde_test` to perform ad-hoc unit testing of
  the deserialization process?
- Performance testing? It seems like the #[derive] stuff is generating
  fast enough deserialization, similar to handwritten ones.
- Conversion from ErrorKind to serde errors?
  - I see there is a Error paired with a ErrorKind and a Span, I'm not
    sure we can get the Span in the deserialization process without
    being too involved.
  - I am not sure I see a way to convert a "partial" string and let
    serde fill the rest, with the span itself for example.

Ryan Lahfa (2):
  feat(tvix/eval): implement From<f64> for Value
  feat(tvix/eval): implement serde::Deserialize for Value

 tvix/eval/Cargo.toml                          |  2 +-
 tvix/eval/src/builtins/mod.rs                 |  4 +-
 .../tests/tvix_tests/eval-okay-fromjson.exp   |  2 +-
 .../tests/tvix_tests/eval-okay-fromjson.nix   |  1 +
 tvix/eval/src/value/attrs.rs                  | 39 ++++++++++-
 tvix/eval/src/value/list.rs                   |  4 +-
 tvix/eval/src/value/mod.rs                    | 65 ++++++-------------
 tvix/eval/src/value/string.rs                 | 34 ++++++++++
 8 files changed, 101 insertions(+), 50 deletions(-)

-- 
2.38.1

[PATCH 1/2] feat(tvix/eval): implement From<f64> for Value

[Ryan Lahfa]

---
 tvix/eval/src/value/mod.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index 9aed486d4..124ce881d 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -530,6 +530,12 @@ impl From<i64> for Value {
     }
 }
 
+impl From<f64> for Value {
+    fn from(i: f64) -> Self {
+        Self::Float(i)
+    }
+}
+
 impl From<PathBuf> for Value {
     fn from(path: PathBuf) -> Self {
         Self::Path(path)
-- 
2.38.1

[PATCH 2/2] feat(tvix/eval): implement serde::Deserialize for Value

[Ryan Lahfa]

---
 tvix/eval/Cargo.toml                          |  2 +-
 tvix/eval/src/builtins/mod.rs                 |  4 +-
 .../tests/tvix_tests/eval-okay-fromjson.exp   |  2 +-
 .../tests/tvix_tests/eval-okay-fromjson.nix   |  1 +
 tvix/eval/src/value/attrs.rs                  | 39 +++++++++++-
 tvix/eval/src/value/list.rs                   |  4 +-
 tvix/eval/src/value/mod.rs                    | 59 +++++--------------
 tvix/eval/src/value/string.rs                 | 34 +++++++++++
 8 files changed, 95 insertions(+), 50 deletions(-)

diff --git a/tvix/eval/Cargo.toml b/tvix/eval/Cargo.toml
index 93d45c216..fdcf0c1e0 100644
--- a/tvix/eval/Cargo.toml
+++ b/tvix/eval/Cargo.toml
@@ -18,7 +18,7 @@ codemap = "0.1.3"
 codemap-diagnostic = "0.1.1"
 proptest = { version = "1.0.0", default_features = false, features = ["std", "alloc", "break-dead-code", "tempfile"], optional = true }
 test-strategy = { version = "0.2.1", optional = true }
-serde = "1.0"
+serde = { version = "1.0", features = [ "rc" ] }
 serde_json = "1.0"
 regex = "1.6.0"
 builtin-macros = { path = "./builtin-macros", package = "tvix-eval-builtin-macros" }
diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index e4e9c14df..c72856088 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -337,8 +337,8 @@ mod pure_builtins {
     #[builtin("fromJSON")]
     fn builtin_from_json(_: &mut VM, json: Value) -> Result<Value, ErrorKind> {
         let json_str = json.to_str()?;
-        let json: serde_json::Value = serde_json::from_str(&json_str)?;
-        json.try_into()
+
+        serde_json::from_str(&json_str).map_err(|err| err.into())
     }
 
     #[builtin("genericClosure")]
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.exp
index 4f75c0923..c855950a3 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.exp
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.exp
@@ -1 +1 @@
-[ { Image = { Animated = false; Height = 600; IDs = [ 116 943 234 38793 true false null -100 ]; Latitude = 37.7668; Longitude = -122.3959; Thumbnail = { Height = 125; Url = "http://www.example.com/image/481989943"; Width = 100; }; Title = "View from 15th Floor"; Width = 800; }; } { name = "a"; value = "b"; } ]
+[ { Image = { Animated = false; Height = 600; IDs = [ 116 943 234 38793 true false null -100 ]; Latitude = 37.7668; Longitude = -122.3959; Thumbnail = { Height = 125; Url = "http://www.example.com/image/481989943"; Width = 100; }; Title = "View from 15th Floor"; Width = 800; }; } { name = "a"; value = "b"; } [ 1 2 3 4 ] ]
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.nix
index ccb83fd0b..e4f621312 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.nix
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-fromjson.nix
@@ -20,4 +20,5 @@
     }
   '')
   (builtins.fromJSON ''{"name": "a", "value": "b"}'')
+  (builtins.fromJSON "[ 1, 2, 3, 4 ]")
 ]
diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs
index ecce34fb4..3e20558ac 100644
--- a/tvix/eval/src/value/attrs.rs
+++ b/tvix/eval/src/value/attrs.rs
@@ -9,6 +9,9 @@ use std::collections::btree_map;
 use std::collections::BTreeMap;
 use std::iter::FromIterator;
 
+use serde::Deserialize;
+use serde::de::{Deserializer, Visitor};
+
 use crate::errors::ErrorKind;
 use crate::vm::VM;
 
@@ -20,7 +23,7 @@ use super::Value;
 #[cfg(test)]
 mod tests;
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Deserialize)]
 enum AttrsRep {
     Empty,
     Map(BTreeMap<NixString, Value>),
@@ -136,6 +139,40 @@ impl TotalDisplay for NixAttrs {
     }
 }
 
+impl<'de> Deserialize<'de> for NixAttrs {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: Deserializer<'de>
+    {
+        struct MapVisitor;
+
+        impl<'de> Visitor<'de> for MapVisitor {
+            type Value = NixAttrs;
+
+            fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+                formatter.write_str("a valid Nix attribute set")
+            }
+
+            fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
+                where
+                    A: serde::de::MapAccess<'de>, {
+                let mut stack_array = Vec::with_capacity(map.size_hint().unwrap_or(0) * 2);
+
+                while let Some((key, value)) = map.next_entry()? {
+                    stack_array.push(key);
+                    stack_array.push(value);
+                }
+
+                // TODO: uncertain how to map the err here.
+                Ok(NixAttrs::construct(stack_array.len() / 2, stack_array).unwrap())
+            }
+        }
+
+        deserializer.deserialize_map(MapVisitor)
+    }
+}
+
+
 #[cfg(feature = "arbitrary")]
 mod arbitrary {
     use super::*;
diff --git a/tvix/eval/src/value/list.rs b/tvix/eval/src/value/list.rs
index 2604b935e..25f974f5f 100644
--- a/tvix/eval/src/value/list.rs
+++ b/tvix/eval/src/value/list.rs
@@ -2,6 +2,8 @@
 use std::ops::Deref;
 use std::rc::Rc;
 
+use serde::Deserialize;
+
 use crate::errors::ErrorKind;
 use crate::vm::VM;
 
@@ -10,7 +12,7 @@ use super::TotalDisplay;
 use super::Value;
 
 #[repr(transparent)]
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Deserialize)]
 pub struct NixList(Rc<Vec<Value>>);
 
 impl TotalDisplay for NixList {
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index 124ce881d..8ec3b5767 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -6,6 +6,8 @@ use std::path::PathBuf;
 use std::rc::Rc;
 use std::{cell::Ref, fmt::Display};
 
+use serde::Deserialize;
+
 #[cfg(feature = "arbitrary")]
 mod arbitrary;
 mod attrs;
@@ -31,30 +33,42 @@ pub use thunk::Thunk;
 use self::thunk::ThunkSet;
 
 #[warn(variant_size_differences)]
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Deserialize)]
+#[serde(untagged)]
 pub enum Value {
     Null,
     Bool(bool),
     Integer(i64),
     Float(f64),
     String(NixString),
+
+    #[serde(skip)]
     Path(PathBuf),
+
     Attrs(Rc<NixAttrs>),
     List(NixList),
+
+    #[serde(skip)]
     Closure(Rc<Closure>), // must use Rc<Closure> here in order to get proper pointer equality
+    #[serde(skip)]
     Builtin(Builtin),
 
     // Internal values that, while they technically exist at runtime,
     // are never returned to or created directly by users.
+    #[serde(skip)]
     Thunk(Thunk),
 
     // See [`compiler::compile_select_or()`] for explanation
+    #[serde(skip)]
     AttrNotFound,
 
     // this can only occur in Chunk::Constants and nowhere else
+    #[serde(skip)]
     Blueprint(Rc<Lambda>),
 
+    #[serde(skip)]
     DeferredUpvalue(StackIdx),
+    #[serde(skip)]
     UnresolvedPath(PathBuf),
 }
 
@@ -548,49 +562,6 @@ impl From<Vec<Value>> for Value {
     }
 }
 
-impl TryFrom<serde_json::Value> for Value {
-    type Error = ErrorKind;
-
-    fn try_from(value: serde_json::Value) -> Result<Self, Self::Error> {
-        // TODO(grfn): Replace with a real serde::Deserialize impl (for perf)
-        match value {
-            serde_json::Value::Null => Ok(Self::Null),
-            serde_json::Value::Bool(b) => Ok(Self::Bool(b)),
-            serde_json::Value::Number(n) => {
-                if let Some(i) = n.as_i64() {
-                    Ok(Self::Integer(i))
-                } else if let Some(f) = n.as_f64() {
-                    Ok(Self::Float(f))
-                } else {
-                    Err(ErrorKind::FromJsonError(format!(
-                        "JSON number not representable as Nix value: {n}"
-                    )))
-                }
-            }
-            serde_json::Value::String(s) => Ok(s.into()),
-            serde_json::Value::Array(a) => Ok(a
-                .into_iter()
-                .map(Value::try_from)
-                .collect::<Result<Vec<_>, _>>()?
-                .into()),
-            serde_json::Value::Object(obj) => {
-                match (obj.len(), obj.get("name"), obj.get("value")) {
-                    (2, Some(name), Some(value)) => Ok(Self::attrs(NixAttrs::from_kv(
-                        name.clone().try_into()?,
-                        value.clone().try_into()?,
-                    ))),
-                    _ => Ok(Self::attrs(NixAttrs::from_iter(
-                        obj.into_iter()
-                            .map(|(k, v)| Ok((k.into(), v.try_into()?)))
-                            .collect::<Result<Vec<(NixString, Value)>, ErrorKind>>()?
-                            .into_iter(),
-                    ))),
-                }
-            }
-        }
-    }
-}
-
 fn type_error(expected: &'static str, actual: &Value) -> ErrorKind {
     ErrorKind::TypeError {
         expected,
diff --git a/tvix/eval/src/value/string.rs b/tvix/eval/src/value/string.rs
index 66697a7f2..f4cc77194 100644
--- a/tvix/eval/src/value/string.rs
+++ b/tvix/eval/src/value/string.rs
@@ -8,6 +8,9 @@ use std::ops::Deref;
 use std::path::Path;
 use std::{borrow::Cow, fmt::Display, str::Chars};
 
+use serde::Deserialize;
+use serde::de::{Deserializer, Visitor};
+
 #[derive(Clone, Debug)]
 enum StringRepr {
     Smol(SmolStr),
@@ -68,6 +71,37 @@ impl Hash for NixString {
     }
 }
 
+impl<'de> Deserialize<'de> for NixString {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: Deserializer<'de>
+    {
+        struct StringVisitor;
+
+        impl<'de> Visitor<'de> for StringVisitor {
+            type Value = NixString;
+
+            fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+                formatter.write_str("a valid Nix string")
+            }
+
+            fn visit_string<E>(self, v: String) -> Result<Self::Value, E>
+                where
+                    E: serde::de::Error, {
+                Ok(v.into())
+            }
+
+            fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+                where
+                    E: serde::de::Error, {
+                Ok(v.into())
+            }
+        }
+
+        deserializer.deserialize_string(StringVisitor)
+    }
+}
+
 #[cfg(feature = "arbitrary")]
 mod arbitrary {
     use super::*;
-- 
2.38.1