From 3fb0697a713cdd5b0c22b3c511419ba3a281746a Mon Sep 17 00:00:00 2001 From: Aspen Smith Date: Fri, 9 Feb 2024 12:59:04 -0500 Subject: fix(tvix/eval): Propagate catchables in NixAttrs::construct Correctly propagate the case where the *key* of an attrset is a Value::Catchable (eg { "${builtins.throw "c"}" = "b"; }) in `NixAttrs::construct`, by converting the return type to `Result, ErrorKind>` (ugh!!) and correctly handling that everywhere (including an `expect` in the Deserialize impl for NixAttrs, since afaict this is impossible to hit when deserializing from stuff like JSON). Change-Id: Ic4bc611fbfdab27c0bd8a40759689a87c4004a17 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10786 Reviewed-by: raitobezarius Tested-by: BuildkiteCI --- .../tests/tvix_tests/eval-okay-attr-key-catchable.exp | 1 + .../tests/tvix_tests/eval-okay-attr-key-catchable.nix | 1 + tvix/eval/src/value/attrs.rs | 18 +++++++++++++----- tvix/eval/src/value/attrs/tests.rs | 18 ++++++++++++------ tvix/eval/src/vm/mod.rs | 6 ++++-- 5 files changed, 31 insertions(+), 13 deletions(-) create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.exp create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.nix (limited to 'tvix/eval/src') diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.exp new file mode 100644 index 0000000000..c508d5366f --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.exp @@ -0,0 +1 @@ +false diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.nix new file mode 100644 index 0000000000..b9d835bcc5 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-attr-key-catchable.nix @@ -0,0 +1 @@ +(builtins.tryEval { "${builtins.throw "a"}" = "b"; }).success diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index 6fd43e064c..23a2acb0b9 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/tvix/eval/src/value/attrs.rs @@ -19,6 +19,7 @@ use super::thunk::ThunkSet; use super::TotalDisplay; use super::Value; use crate::errors::ErrorKind; +use crate::CatchableErrorKind; lazy_static! { static ref NAME_S: NixString = "name".into(); @@ -170,7 +171,9 @@ impl<'de> Deserialize<'de> for NixAttrs { stack_array.push(value); } - NixAttrs::construct(stack_array.len() / 2, stack_array).map_err(A::Error::custom) + Ok(NixAttrs::construct(stack_array.len() / 2, stack_array) + .map_err(A::Error::custom)? + .expect("Catchable values are unreachable here")) } } @@ -333,7 +336,10 @@ impl NixAttrs { /// Implement construction logic of an attribute set, to encapsulate /// logic about attribute set optimisations inside of this module. - pub fn construct(count: usize, mut stack_slice: Vec) -> Result { + pub fn construct( + count: usize, + mut stack_slice: Vec, + ) -> Result, ErrorKind> { debug_assert!( stack_slice.len() == count * 2, "construct_attrs called with count == {}, but slice.len() == {}", @@ -343,13 +349,13 @@ impl NixAttrs { // Optimisation: Empty attribute set if count == 0 { - return Ok(NixAttrs(AttrsRep::Empty)); + return Ok(Ok(NixAttrs(AttrsRep::Empty))); } // Optimisation: KV pattern if count == 2 { if let Some(kv) = attempt_optimise_kv(&mut stack_slice) { - return Ok(kv); + return Ok(Ok(kv)); } } @@ -369,11 +375,13 @@ impl NixAttrs { continue; } + Value::Catchable(err) => return Ok(Err(err)), + other => return Err(ErrorKind::InvalidAttributeName(other)), } } - Ok(attrs) + Ok(Ok(attrs)) } /// Construct an optimized "KV"-style attribute set given the value for the diff --git a/tvix/eval/src/value/attrs/tests.rs b/tvix/eval/src/value/attrs/tests.rs index d7e2f113eb..534b78a00d 100644 --- a/tvix/eval/src/value/attrs/tests.rs +++ b/tvix/eval/src/value/attrs/tests.rs @@ -4,7 +4,9 @@ use super::*; #[test] fn test_empty_attrs() { - let attrs = NixAttrs::construct(0, vec![]).expect("empty attr construction should succeed"); + let attrs = NixAttrs::construct(0, vec![]) + .expect("empty attr construction should succeed") + .unwrap(); assert!( matches!(attrs, NixAttrs(AttrsRep::Empty)), @@ -15,7 +17,8 @@ fn test_empty_attrs() { #[test] fn test_simple_attrs() { let attrs = NixAttrs::construct(1, vec![Value::from("key"), Value::from("value")]) - .expect("simple attr construction should succeed"); + .expect("simple attr construction should succeed") + .unwrap(); assert!( matches!(attrs, NixAttrs(AttrsRep::Im(_))), @@ -39,7 +42,8 @@ fn test_kv_attrs() { meaning_val.clone(), ], ) - .expect("constructing K/V pair attrs should succeed"); + .expect("constructing K/V pair attrs should succeed") + .unwrap(); match kv_attrs { NixAttrs(AttrsRep::KV { name, value }) @@ -55,7 +59,7 @@ fn test_kv_attrs() { #[test] fn test_empty_attrs_iter() { - let attrs = NixAttrs::construct(0, vec![]).unwrap(); + let attrs = NixAttrs::construct(0, vec![]).unwrap().unwrap(); assert!(attrs.iter().next().is_none()); } @@ -75,7 +79,8 @@ fn test_kv_attrs_iter() { meaning_val.clone(), ], ) - .expect("constructing K/V pair attrs should succeed"); + .expect("constructing K/V pair attrs should succeed") + .unwrap(); let mut iter = kv_attrs.iter().collect::>().into_iter(); let (k, v) = iter.next().unwrap(); @@ -90,7 +95,8 @@ fn test_kv_attrs_iter() { #[test] fn test_map_attrs_iter() { let attrs = NixAttrs::construct(1, vec![Value::from("key"), Value::from("value")]) - .expect("simple attr construction should succeed"); + .expect("simple attr construction should succeed") + .unwrap(); let mut iter = attrs.iter().collect::>().into_iter(); let (k, v) = iter.next().unwrap(); diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index d23bef6743..88d616fe3a 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -932,9 +932,11 @@ where fn run_attrset(&mut self, frame: &CallFrame, count: usize) -> EvalResult<()> { let attrs = NixAttrs::construct(count, self.stack.split_off(self.stack.len() - count * 2)) - .with_span(frame, self)?; + .with_span(frame, self)? + .map(Value::attrs) + .into(); - self.stack.push(Value::attrs(attrs)); + self.stack.push(attrs); Ok(()) } -- cgit 1.4.1