From 939cebd0f17b8e8ec6a4664f9f7e0a5e1c6e3957 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Sat, 4 Mar 2023 02:12:06 +0300 Subject: fix(tvix/eval): implement cppnix JSON-serialisation semantics This drops the usage of serde::Serialize, as the trait can not be used to implement the correct semantics (function colouring!). Instead, a manual JSON serialisation function is written which correctly handles toString, outPath and other similar weirdnesses. Unexpectedly, the eval-okay-tojson test from the C++ Nix test suite now passes, too. This fixes an issue where serialising data structures containing derivations to JSON would fail. Change-Id: I5c39e3d8356ee93a07eda481410f88610f6dd9f8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8209 Reviewed-by: raitobezarius Tested-by: BuildkiteCI --- tvix/eval/src/builtins/mod.rs | 32 +-------- tvix/eval/src/builtins/to_xml.rs | 3 +- tvix/eval/src/errors.rs | 9 +++ tvix/eval/src/tests/nix_tests/eval-okay-tojson.exp | 1 + tvix/eval/src/tests/nix_tests/eval-okay-tojson.nix | 13 ++++ .../nix_tests/notyetpassing/eval-okay-tojson.exp | 1 - .../nix_tests/notyetpassing/eval-okay-tojson.nix | 13 ---- tvix/eval/src/value/attrs.rs | 21 +----- tvix/eval/src/value/json.rs | 84 ++++++++++++++++++++++ tvix/eval/src/value/list.rs | 4 +- tvix/eval/src/value/mod.rs | 18 +++-- tvix/eval/src/value/thunk.rs | 11 --- tvix/eval/src/vm/generators.rs | 23 ++++++ tvix/serde/src/de.rs | 3 +- 14 files changed, 152 insertions(+), 84 deletions(-) create mode 100644 tvix/eval/src/tests/nix_tests/eval-okay-tojson.exp create mode 100644 tvix/eval/src/tests/nix_tests/eval-okay-tojson.nix delete mode 100644 tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.exp delete mode 100644 tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.nix create mode 100644 tvix/eval/src/value/json.rs (limited to 'tvix') diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 535bf8b2a305..e24285f5c23a 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -359,36 +359,8 @@ mod pure_builtins { #[builtin("toJSON")] async fn builtin_to_json(co: GenCo, val: Value) -> Result { - let mut val = val; // shadow mutably, not supported by macro - loop { - if let Value::Attrs(attrs) = &val { - // Attribute sets with a callable `__toString` attribute - // serialise to the string-coerced version of the result of - // calling that. - if let Some(s) = attrs.try_to_string(&co, CoercionKind::Weak).await { - return Ok(Value::String(serde_json::to_string(&s)?.into())); - } - - // Attribute sets with an `outPath` attribute - // serialise to a JSON serialisation of that inner - // value (regardless of what it is!). - if let Some(out_path) = attrs.select("outPath") { - val = out_path.clone(); - continue; - } - - // Attribute set should be serialised normally (by - // traversing it and serialising keys/values). - break; - } - - break; - } - - // All thunks need to be evaluated before serialising, as the - // data structure is fully traversed by the Serializer. - let val = generators::request_deep_force(&co, val, SharedThunkSet::default()).await; - let json_str = serde_json::to_string(&val)?; + let json_value = val.to_json(&co).await?; + let json_str = serde_json::to_string(&json_value)?; Ok(json_str.into()) } diff --git a/tvix/eval/src/builtins/to_xml.rs b/tvix/eval/src/builtins/to_xml.rs index 750bb27aa226..375785b7a63e 100644 --- a/tvix/eval/src/builtins/to_xml.rs +++ b/tvix/eval/src/builtins/to_xml.rs @@ -126,7 +126,8 @@ fn value_variant_to_xml(w: &mut EventWriter, value: &Value) -> Resu Value::AttrNotFound | Value::Blueprint(_) | Value::DeferredUpvalue(_) - | Value::UnresolvedPath(_) => { + | Value::UnresolvedPath(_) + | Value::Json(_) => { return Err(ErrorKind::TvixBug { msg: "internal value variant encountered in builtins.toXML", metadata: Some(Rc::new(value.clone())), diff --git a/tvix/eval/src/errors.rs b/tvix/eval/src/errors.rs index ec697fa84288..2f00ecab8543 100644 --- a/tvix/eval/src/errors.rs +++ b/tvix/eval/src/errors.rs @@ -134,6 +134,9 @@ pub enum ErrorKind { /// Errors converting JSON to a value FromJsonError(String), + /// Nix value that can not be serialised to JSON. + NotSerialisableToJson(&'static str), + /// Errors converting TOML to a value FromTomlError(String), @@ -442,6 +445,10 @@ to a missing value in the attribute set(s) included via `with`."#, write!(f, "Error converting JSON to a Nix value: {msg}") } + ErrorKind::NotSerialisableToJson(_type) => { + write!(f, "a {} cannot be converted to JSON", _type) + } + ErrorKind::FromTomlError(msg) => { write!(f, "Error converting TOML to a Nix value: {msg}") } @@ -761,6 +768,7 @@ impl Error { | ErrorKind::ImportCompilerError { .. } | ErrorKind::IO { .. } | ErrorKind::FromJsonError(_) + | ErrorKind::NotSerialisableToJson(_) | ErrorKind::FromTomlError(_) | ErrorKind::Xml(_) | ErrorKind::TvixError(_) @@ -809,6 +817,7 @@ impl Error { ErrorKind::DivisionByZero => "E033", ErrorKind::Xml(_) => "E034", ErrorKind::FromTomlError(_) => "E035", + ErrorKind::NotSerialisableToJson(_) => "E036", // Special error code for errors from other Tvix // components. We may want to introduce a code namespacing diff --git a/tvix/eval/src/tests/nix_tests/eval-okay-tojson.exp b/tvix/eval/src/tests/nix_tests/eval-okay-tojson.exp new file mode 100644 index 000000000000..e92aae3235f2 --- /dev/null +++ b/tvix/eval/src/tests/nix_tests/eval-okay-tojson.exp @@ -0,0 +1 @@ +"{\"a\":123,\"b\":-456,\"c\":\"foo\",\"d\":\"foo\\n\\\"bar\\\"\",\"e\":true,\"f\":false,\"g\":[1,2,3],\"h\":[\"a\",[\"b\",{\"foo\\nbar\":{}}]],\"i\":3,\"j\":1.44,\"k\":\"foo\"}" diff --git a/tvix/eval/src/tests/nix_tests/eval-okay-tojson.nix b/tvix/eval/src/tests/nix_tests/eval-okay-tojson.nix new file mode 100644 index 000000000000..ce67943bead5 --- /dev/null +++ b/tvix/eval/src/tests/nix_tests/eval-okay-tojson.nix @@ -0,0 +1,13 @@ +builtins.toJSON + { a = 123; + b = -456; + c = "foo"; + d = "foo\n\"bar\""; + e = true; + f = false; + g = [ 1 2 3 ]; + h = [ "a" [ "b" { "foo\nbar" = {}; } ] ]; + i = 1 + 2; + j = 1.44; + k = { __toString = self: self.a; a = "foo"; }; + } diff --git a/tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.exp b/tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.exp deleted file mode 100644 index e92aae3235f2..000000000000 --- a/tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.exp +++ /dev/null @@ -1 +0,0 @@ -"{\"a\":123,\"b\":-456,\"c\":\"foo\",\"d\":\"foo\\n\\\"bar\\\"\",\"e\":true,\"f\":false,\"g\":[1,2,3],\"h\":[\"a\",[\"b\",{\"foo\\nbar\":{}}]],\"i\":3,\"j\":1.44,\"k\":\"foo\"}" diff --git a/tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.nix b/tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.nix deleted file mode 100644 index ce67943bead5..000000000000 --- a/tvix/eval/src/tests/nix_tests/notyetpassing/eval-okay-tojson.nix +++ /dev/null @@ -1,13 +0,0 @@ -builtins.toJSON - { a = 123; - b = -456; - c = "foo"; - d = "foo\n\"bar\""; - e = true; - f = false; - g = [ 1 2 3 ]; - h = [ "a" [ "b" { "foo\nbar" = {}; } ] ]; - i = 1 + 2; - j = 1.44; - k = { __toString = self: self.a; a = "foo"; }; - } diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index e4584e3aa2f7..bacfd22217bd 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/tvix/eval/src/value/attrs.rs @@ -10,8 +10,7 @@ use std::iter::FromIterator; use imbl::{ordmap, OrdMap}; use lazy_static::lazy_static; use serde::de::{Deserializer, Error, Visitor}; -use serde::ser::SerializeMap; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use super::string::NixString; use super::thunk::ThunkSet; @@ -151,24 +150,6 @@ impl TotalDisplay for NixAttrs { } } -impl Serialize for NixAttrs { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - match &self.0 { - AttrsRep::Empty => serializer.serialize_map(Some(0))?.end(), - AttrsRep::KV { name, value } => { - let mut map = serializer.serialize_map(Some(2))?; - map.serialize_entry("name", name)?; - map.serialize_entry("value", value)?; - map.end() - } - AttrsRep::Im(map) => map.serialize(serializer), - } - } -} - impl<'de> Deserialize<'de> for NixAttrs { fn deserialize(deserializer: D) -> Result where diff --git a/tvix/eval/src/value/json.rs b/tvix/eval/src/value/json.rs new file mode 100644 index 000000000000..33e16ebffcd0 --- /dev/null +++ b/tvix/eval/src/value/json.rs @@ -0,0 +1,84 @@ +/// Implementation of Value serialisation *to* JSON. +/// +/// This can not be implemented through standard serde-derive methods, +/// as there is internal Nix logic that must happen within the +/// serialisation methods. +use super::{CoercionKind, Value}; +use crate::generators::{self, GenCo}; +use crate::ErrorKind; + +use serde_json::value::to_value; +use serde_json::Value as Json; +use serde_json::{Map, Number}; // name clash with *our* `Value` + +impl Value { + pub(crate) async fn to_json(self, co: &GenCo) -> Result { + let self_forced = generators::request_force(co, self).await; + + let value = match self_forced { + Value::Null => Json::Null, + Value::Bool(b) => Json::Bool(b), + Value::Integer(i) => Json::Number(Number::from(i)), + Value::Float(f) => to_value(f)?, + Value::String(s) => Json::String(s.as_str().into()), + + Value::Path(p) => { + let imported = generators::request_path_import(co, *p).await; + Json::String(imported.to_string_lossy().to_string()) + } + + Value::List(l) => { + let mut out = vec![]; + + for val in l.into_iter() { + out.push(generators::request_to_json(co, val).await); + } + + Json::Array(out) + } + + Value::Attrs(attrs) => { + // Attribute sets with a callable `__toString` attribute + // serialise to the string-coerced version of the result of + // calling that. + if let Some(s) = attrs.try_to_string(&co, CoercionKind::Weak).await { + return Ok(Json::String(s.as_str().to_string())); + } + + // Attribute sets with an `outPath` attribute + // 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 mut out = Map::with_capacity(attrs.len()); + for (name, value) in attrs.into_iter_sorted() { + out.insert( + name.as_str().to_string(), + generators::request_to_json(co, value).await, + ); + } + + Json::Object(out) + } + + val @ Value::Closure(_) + | val @ Value::Thunk(_) + | val @ Value::Builtin(_) + | val @ Value::AttrNotFound + | val @ Value::Blueprint(_) + | val @ Value::DeferredUpvalue(_) + | val @ Value::UnresolvedPath(_) + | val @ Value::Json(_) => return Err(ErrorKind::NotSerialisableToJson(val.type_of())), + }; + + Ok(value) + } + + /// Generator version of the above, which wraps responses in + /// Value::Json. + pub(crate) async fn to_json_generator(self, co: GenCo) -> Result { + Ok(Value::Json(self.to_json(&co).await?)) + } +} diff --git a/tvix/eval/src/value/list.rs b/tvix/eval/src/value/list.rs index 0bdc61c91d68..cfaefff82195 100644 --- a/tvix/eval/src/value/list.rs +++ b/tvix/eval/src/value/list.rs @@ -4,7 +4,7 @@ use std::rc::Rc; use imbl::{vector, Vector}; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use crate::generators; use crate::generators::GenCo; @@ -16,7 +16,7 @@ use super::TotalDisplay; use super::Value; #[repr(transparent)] -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize)] pub struct NixList(Rc>); impl TotalDisplay for NixList { diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 4e93b36d55d3..41b1e5ac8a16 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -9,13 +9,14 @@ use std::pin::Pin; use std::rc::Rc; use lexical_core::format::CXX_LITERAL; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; #[cfg(feature = "arbitrary")] mod arbitrary; mod attrs; mod builtin; mod function; +mod json; mod list; mod path; mod string; @@ -39,7 +40,7 @@ pub use self::thunk::{SharedThunkSet, ThunkSet}; use lazy_static::lazy_static; #[warn(variant_size_differences)] -#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Clone, Debug, Deserialize)] #[serde(untagged)] pub enum Value { Null, @@ -76,6 +77,8 @@ pub enum Value { DeferredUpvalue(StackIdx), #[serde(skip)] UnresolvedPath(Box), + #[serde(skip)] + Json(serde_json::Value), } lazy_static! { @@ -231,7 +234,8 @@ impl Value { Value::AttrNotFound | Value::Blueprint(_) | Value::DeferredUpvalue(_) - | Value::UnresolvedPath(_) => panic!( + | Value::UnresolvedPath(_) + | Value::Json(_) => panic!( "Tvix bug: internal value left on stack: {}", value.type_of() ), @@ -322,7 +326,8 @@ impl Value { (Value::AttrNotFound, _) | (Value::Blueprint(_), _) | (Value::DeferredUpvalue(_), _) - | (Value::UnresolvedPath(_), _) => { + | (Value::UnresolvedPath(_), _) + | (Value::Json(_), _) => { panic!("tvix bug: .coerce_to_string() called on internal value") } } @@ -512,6 +517,7 @@ impl Value { Value::Blueprint(_) => "internal[blueprint]", Value::DeferredUpvalue(_) => "internal[deferred_upvalue]", Value::UnresolvedPath(_) => "internal[unresolved_path]", + Value::Json(_) => "internal[json]", } } @@ -643,7 +649,8 @@ impl Value { Value::AttrNotFound | Value::Blueprint(_) | Value::DeferredUpvalue(_) - | Value::UnresolvedPath(_) => "an internal Tvix evaluator value".into(), + | Value::UnresolvedPath(_) + | Value::Json(_) => "an internal Tvix evaluator value".into(), } } } @@ -756,6 +763,7 @@ 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]"), // Delegate thunk display to the type, as it must handle // the case of already evaluated or cyclic thunks. diff --git a/tvix/eval/src/value/thunk.rs b/tvix/eval/src/value/thunk.rs index 42e8ec869ac5..e990a5cad5cc 100644 --- a/tvix/eval/src/value/thunk.rs +++ b/tvix/eval/src/value/thunk.rs @@ -25,8 +25,6 @@ use std::{ rc::Rc, }; -use serde::Serialize; - use crate::{ errors::ErrorKind, spans::LightSpan, @@ -269,15 +267,6 @@ impl TotalDisplay for Thunk { } } -impl Serialize for Thunk { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - self.value().serialize(serializer) - } -} - /// A wrapper type for tracking which thunks have already been seen in a /// context. This is necessary for cycle detection. /// diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index 7a1ba0153d24..9b055eb1c826 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/tvix/eval/src/vm/generators.rs @@ -114,6 +114,10 @@ pub enum GeneratorRequest { /// Request evaluation of `builtins.tryEval` from the VM. See /// [`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), } /// Human-readable representation of a generator message, used by observers. @@ -160,6 +164,7 @@ impl Display for GeneratorRequest { GeneratorRequest::ReadDir(p) => write!(f, "read_dir({})", p.to_string_lossy()), GeneratorRequest::Span => write!(f, "span"), GeneratorRequest::TryForce(v) => write!(f, "try_force({})", v.type_of()), + GeneratorRequest::ToJson(v) => write!(f, "to_json({})", v.type_of()), } } } @@ -440,6 +445,14 @@ impl<'o> VM<'o> { self.enqueue_generator("force", span, |co| value.force(co)); return Ok(false); } + + GeneratorRequest::ToJson(value) => { + self.reenqueue_generator(name, span.clone(), generator); + self.enqueue_generator("to_json", span, |co| { + value.to_json_generator(co) + }); + return Ok(false); + } } } @@ -708,6 +721,16 @@ pub(crate) async fn request_span(co: &GenCo) -> LightSpan { } } +pub(crate) async fn request_to_json(co: &GenCo, value: Value) -> serde_json::Value { + match co.yield_(GeneratorRequest::ToJson(value)).await { + GeneratorResponse::Value(Value::Json(json)) => json, + msg => panic!( + "Tvix bug: VM responded with incorrect generator message: {}", + msg + ), + } +} + /// Call the given value as if it was an attribute set containing a functor. The /// arguments must already be prepared on the stack when a generator frame from /// this function is invoked. diff --git a/tvix/serde/src/de.rs b/tvix/serde/src/de.rs index e6bcf41cf231..0285c386ca2e 100644 --- a/tvix/serde/src/de.rs +++ b/tvix/serde/src/de.rs @@ -91,7 +91,8 @@ impl<'de> de::Deserializer<'de> for NixDeserializer { | Value::AttrNotFound | Value::Blueprint(_) | Value::DeferredUpvalue(_) - | Value::UnresolvedPath(_) => Err(Error::Unserializable { + | Value::UnresolvedPath(_) + | Value::Json(_) => Err(Error::Unserializable { value_type: self.value.type_of(), }), } -- cgit 1.4.1