refactor(tvix/eval): remove Value::Json and related functionality

Currently Value::Json is used in combination with VMRequest::ToJson to
recursively convert tvix Value to serde_json::Value. This functionality
is used in builtins.toJSON as well as derivation __structuredAttrs.

Both Value::Json and VMRequest::ToJson were removed in this commit.

Related functionality in vm.rs is also removed: vm.rs does not know
about JSON anymore.

Recursively converting to serde_json now happens without going through
the VM.

Thrown errors that are part of the value of toJSON are now directly
propagated as ErrorKind, were-as previously there was a split between
CatchableErrorKind and ErrorKind, where eventually CatchableErrorKind
would be converted to ErrorKind::Catchable.

Change-Id: I066f064926c491e4c087a984f07af43d19124cfe
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12732
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI

tvix/docs/src/TODO.md

@@ -10,15 +10,6 @@ Feel free to add new ideas. Before picking something, ask in `#tvix-dev` to make
 sure noone is working on this, or has some specific design in mind already.
 
 ## Cleanups
-### Evaluator
- - There's not really a good reason why the `tvix_eval::Value::Json` enum kind
-   exists.
-  `builtins.toJSON` should simply produce a string with context, and everything
-   else should be a hidden implementation detail and should not be leaked to
-   `Value`.
-   This is a hack, as we wanted to use `serde_json` as is, but should be cleaned
-   up.
-
 ### Nix language test suite
  - Think about how to merge, but "categorize" `tvix_tests` in `glue` and `eval`.
    We currently only have this split as they need a different feature set /

tvix/eval/src/builtins/mod.rs

@@ -442,17 +442,20 @@ mod pure_builtins {
     #[builtin("fromJSON")]
     async fn builtin_from_json(co: GenCo, json: Value) -> Result<Value, ErrorKind> {
         let json_str = json.to_str()?;
-
         serde_json::from_slice(&json_str).map_err(|err| err.into())
     }
 
     #[builtin("toJSON")]
     async fn builtin_to_json(co: GenCo, val: Value) -> Result<Value, ErrorKind> {
-        match val.into_contextful_json(&co).await? {
-            Err(cek) => Ok(Value::from(cek)),
-            Ok((json_value, ctx)) => {
-                let json_str = serde_json::to_string(&json_value)?;
-                Ok(NixString::new_context_from(ctx, json_str).into())
+        match val.into_contextful_json(&co).await {
+            Err(ErrorKind::CatchableError(catchable)) => Ok(Value::Catchable(Box::new(catchable))),
+            Err(err) => Err(err),
+            Ok((json, context)) => {
+                let json_str = serde_json::to_string(&json)
+                    .map_err(|err| ErrorKind::JsonError(err.to_string()))?;
+                Ok(Value::String(NixString::new_context_from(
+                    context, json_str,
+                )))
             }
         }
     }

tvix/eval/src/builtins/to_xml.rs

@@ -122,7 +122,6 @@ fn value_variant_to_xml<W: Write>(w: &mut XmlEmitter<W>, value: &Value) -> Resul
         | Value::Blueprint(_)
         | Value::DeferredUpvalue(_)
         | Value::UnresolvedPath(_)
-        | Value::Json(..)
         | Value::FinaliseRequest(_) => {
             return Err(ErrorKind::TvixBug {
                 msg: "internal value variant encountered in builtins.toXML",

tvix/eval/src/value/json.rs

@@ -4,7 +4,7 @@
 /// as there is internal Nix logic that must happen within the
 /// serialisation methods.
 use super::{CoercionKind, Value};
-use crate::errors::{CatchableErrorKind, ErrorKind};
+use crate::errors::ErrorKind;
 use crate::generators::{self, GenCo};
 use crate::NixContext;
 
@@ -17,10 +17,7 @@ impl Value {
     /// Transforms the structure into a JSON
     /// and accumulate all encountered context in the second's element
     /// of the return type.
-    pub async fn into_contextful_json(
-        self,
-        co: &GenCo,
-    ) -> Result<Result<(Json, NixContext), CatchableErrorKind>, ErrorKind> {
+    pub async fn into_contextful_json(self, co: &GenCo) -> Result<(Json, NixContext), ErrorKind> {
         let self_forced = generators::request_force(co, self).await;
         let mut context = NixContext::new();
 
@@ -46,13 +43,9 @@ impl Value {
                 let mut out = vec![];
 
                 for val in l.into_iter() {
-                    match generators::request_to_json(co, val).await {
-                        Ok((v, ctx)) => {
-                            context.extend(ctx.into_iter());
-                            out.push(v)
-                        }
-                        Err(cek) => return Ok(Err(cek)),
-                    }
+                    let (json_item, ctx) = Box::pin(val.into_contextful_json(co)).await?;
+                    context.extend(ctx.into_iter());
+                    out.push(json_item);
                 }
 
                 Json::Array(out)
@@ -75,14 +68,13 @@ impl Value {
                         )
                         .await?
                     {
-                        Value::Catchable(cek) => return Ok(Err(*cek)),
+                        Value::Catchable(cek) => return Err(ErrorKind::CatchableError(*cek)),
                         Value::String(s) => {
                             // We need a fresh context here because `__toString` will discard
                             // everything.
                             let mut fresh = NixContext::new();
                             fresh.mimic(&s);
-
-                            return Ok(Ok((Json::String(s.to_str()?.to_owned()), fresh)));
+                            return Ok((Json::String(s.to_str()?.to_owned()), fresh));
                         }
                         _ => panic!("Value::coerce_to_string_() returned a non-string!"),
                     }
@@ -92,27 +84,23 @@ impl Value {
                 // serialise to a JSON serialisation of that inner
                 // value (regardless of what it is!).
                 if let Some(out_path) = attrs.select("outPath") {
-                    return Ok(generators::request_to_json(co, out_path.clone()).await);
+                    let (json_out_path, ctx) =
+                        Box::pin(out_path.clone().into_contextful_json(co)).await?;
+                    context.extend(ctx.into_iter());
+                    return Ok((json_out_path, context));
                 }
 
                 let mut out = Map::with_capacity(attrs.len());
                 for (name, value) in attrs.into_iter_sorted() {
-                    out.insert(
-                        name.to_str()?.to_owned(),
-                        match generators::request_to_json(co, value).await {
-                            Ok((v, ctx)) => {
-                                context.extend(ctx.into_iter());
-                                v
-                            }
-                            Err(cek) => return Ok(Err(cek)),
-                        },
-                    );
+                    let (json_value, ctx) = Box::pin(value.into_contextful_json(co)).await?;
+                    context.extend(ctx.into_iter());
+                    out.insert(name.to_str()?.to_owned(), json_value);
                 }
 
                 Json::Object(out)
             }
 
-            Value::Catchable(c) => return Ok(Err(*c)),
+            Value::Catchable(c) => return Err(ErrorKind::CatchableError(*c)),
 
             val @ Value::Closure(_)
             | val @ Value::Thunk(_)
@@ -121,34 +109,10 @@ impl Value {
             | val @ Value::Blueprint(_)
             | val @ Value::DeferredUpvalue(_)
             | val @ Value::UnresolvedPath(_)
-            | val @ Value::Json(..)
             | val @ Value::FinaliseRequest(_) => {
                 return Err(ErrorKind::NotSerialisableToJson(val.type_of()))
             }
         };
-
-        Ok(Ok((value, context)))
-    }
-
-    /// Generator version of the above, which wraps responses in
-    /// [`Value::Json`].
-    pub(crate) async fn into_contextful_json_generator(
-        self,
-        co: GenCo,
-    ) -> Result<Value, ErrorKind> {
-        match self.into_contextful_json(&co).await? {
-            Err(cek) => Ok(Value::from(cek)),
-            Ok((json, ctx)) => Ok(Value::Json(Box::new((json, ctx)))),
-        }
-    }
-
-    /// Transforms the structure into a JSON
-    /// All the accumulated context is ignored, use [`into_contextful_json`]
-    /// to obtain the resulting context of the JSON object.
-    pub async fn into_json(
-        self,
-        co: &GenCo,
-    ) -> Result<Result<Json, CatchableErrorKind>, ErrorKind> {
-        Ok(self.into_contextful_json(co).await?.map(|(json, _)| json))
+        Ok((value, context))
     }
 }

tvix/eval/src/value/mod.rs

@@ -76,8 +76,6 @@ pub enum Value {
     DeferredUpvalue(StackIdx),
     #[serde(skip)]
     UnresolvedPath(Box<PathBuf>),
-    #[serde(skip)]
-    Json(Box<(serde_json::Value, NixContext)>),
 
     #[serde(skip)]
     FinaliseRequest(bool),
@@ -299,7 +297,6 @@ impl Value {
                 | Value::Blueprint(_)
                 | Value::DeferredUpvalue(_)
                 | Value::UnresolvedPath(_)
-                | Value::Json(..)
                 | Value::FinaliseRequest(_) => panic!(
                     "Tvix bug: internal value left on stack: {}",
                     value.type_of()
@@ -449,7 +446,6 @@ impl Value {
                 | (Value::Blueprint(_), _)
                 | (Value::DeferredUpvalue(_), _)
                 | (Value::UnresolvedPath(_), _)
-                | (Value::Json(..), _)
                 | (Value::FinaliseRequest(_), _) => {
                     panic!("tvix bug: .coerce_to_string() called on internal value")
                 }
@@ -686,7 +682,6 @@ impl Value {
             Value::Blueprint(_) => "internal[blueprint]",
             Value::DeferredUpvalue(_) => "internal[deferred_upvalue]",
             Value::UnresolvedPath(_) => "internal[unresolved_path]",
-            Value::Json(..) => "internal[json]",
             Value::FinaliseRequest(_) => "internal[finaliser_sentinel]",
             Value::Catchable(_) => "internal[catchable]",
         }
@@ -882,7 +877,6 @@ impl Value {
             | Value::Blueprint(_)
             | Value::DeferredUpvalue(_)
             | Value::UnresolvedPath(_)
-            | Value::Json(..)
             | Value::FinaliseRequest(_) => "an internal Tvix evaluator value".into(),
         }
     }
@@ -1002,7 +996,6 @@ impl TotalDisplay for Value {
             Value::Blueprint(_) => f.write_str("internal[blueprint]"),
             Value::DeferredUpvalue(_) => f.write_str("internal[deferred_upvalue]"),
             Value::UnresolvedPath(_) => f.write_str("internal[unresolved_path]"),
-            Value::Json(..) => f.write_str("internal[json]"),
             Value::FinaliseRequest(_) => f.write_str("internal[finaliser_sentinel]"),
 
             // Delegate thunk display to the type, as it must handle

tvix/eval/src/vm/generators.rs

@@ -118,10 +118,6 @@ pub enum VMRequest {
     /// [`VM::catch_result`] for an explanation of how this works.
     TryForce(Value),
 
-    /// Request serialisation of a value to JSON, according to the
-    /// slightly odd Nix evaluation rules.
-    ToJson(Value),
-
     /// Request the VM for the file type of the given path.
     ReadFileType(PathBuf),
 }
@@ -180,7 +176,6 @@ impl Display for VMRequest {
             VMRequest::ReadDir(p) => write!(f, "read_dir({})", p.to_string_lossy()),
             VMRequest::Span => write!(f, "span"),
             VMRequest::TryForce(v) => write!(f, "try_force({})", v.type_of()),
-            VMRequest::ToJson(v) => write!(f, "to_json({})", v.type_of()),
             VMRequest::ReadFileType(p) => write!(f, "read_file_type({})", p.to_string_lossy()),
         }
     }
@@ -497,14 +492,6 @@ where
                             return Ok(false);
                         }
 
-                        VMRequest::ToJson(value) => {
-                            self.reenqueue_generator(name, span, generator);
-                            self.enqueue_generator("to_json", span, |co| {
-                                value.into_contextful_json_generator(co)
-                            });
-                            return Ok(false);
-                        }
-
                         VMRequest::ReadFileType(path) => {
                             let file_type = self
                                 .io_handle
@@ -798,20 +785,6 @@ pub(crate) async fn request_span(co: &GenCo) -> Span {
     }
 }
 
-pub(crate) async fn request_to_json(
-    co: &GenCo,
-    value: Value,
-) -> Result<(serde_json::Value, NixContext), CatchableErrorKind> {
-    match co.yield_(VMRequest::ToJson(value)).await {
-        VMResponse::Value(Value::Json(json_with_ctx)) => Ok(*json_with_ctx),
-        VMResponse::Value(Value::Catchable(cek)) => Err(*cek),
-        msg => panic!(
-            "Tvix bug: VM responded with incorrect generator message: {}",
-            msg
-        ),
-    }
-}
-
 #[cfg_attr(not(feature = "impure"), allow(unused))]
 pub(crate) async fn request_read_file_type(co: &GenCo, path: PathBuf) -> FileType {
     match co.yield_(VMRequest::ReadFileType(path)).await {

tvix/glue/src/builtins/derivation.rs

@@ -300,7 +300,7 @@ pub(crate) mod derivation_builtins {
                     // Remove the original default `out` output.
                     drv.outputs.clear();
 
-                    let mut output_names = vec![];
+                    let mut output_names = Vec::with_capacity(outputs.len());
 
                     for output in outputs {
                         let output_name = generators::request_force(&co, output)
@@ -379,11 +379,7 @@ pub(crate) mod derivation_builtins {
                             return Ok(val);
                         }
 
-                        let (val_json, context) = match val.into_contextful_json(&co).await? {
-                            Ok(v) => v,
-                            Err(cek) => return Ok(Value::from(cek)),
-                        };
-
+                        let (val_json, context) = val.into_contextful_json(&co).await?;
                         input_context.extend(context.into_iter());
 
                         // No need to check for dups, we only iterate over every attribute name once

tvix/serde/src/de.rs

@@ -105,7 +105,6 @@ impl<'de> de::Deserializer<'de> for NixDeserializer {
             | Value::Blueprint(_)
             | Value::DeferredUpvalue(_)
             | Value::UnresolvedPath(_)
-            | Value::Json(..)
             | Value::Catchable(_)
             | Value::FinaliseRequest(_) => Err(Error::Unserializable {
                 value_type: self.value.type_of(),