From 2a3d49810482b36de9f2d3087e5064545183dbb3 Mon Sep 17 00:00:00 2001 From: Griffin Smith Date: Wed, 12 Oct 2022 23:53:03 -0400 Subject: feat(tvix/eval): Validate closed formals Validate "closed formals" (formal parameters without an ellipsis) via a new ValidateClosedFormals op, which checks the arguments (in an attr set at the top of the stack) against the formal parameters on the Lambda in the current frame, and returns a new UnexpectedArgument error (including the span of the formals themselves!!) if any arguments aren't allowed Change-Id: Idcc47a59167a83be1832a6229f137d84e426c56c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7002 Reviewed-by: tazjin Tested-by: BuildkiteCI --- tvix/eval/src/compiler/mod.rs | 13 +++-- tvix/eval/src/errors.rs | 33 +++++++++++- tvix/eval/src/opcode.rs | 6 +++ .../tests/tvix_tests/eval-fail-closed-formals.nix | 1 + tvix/eval/src/value/attrs.rs | 58 ++++++++++++++++++++-- tvix/eval/src/value/function.rs | 20 ++++++++ tvix/eval/src/vm.rs | 15 ++++++ 7 files changed, 133 insertions(+), 13 deletions(-) create mode 100644 tvix/eval/src/tests/tvix_tests/eval-fail-closed-formals.nix (limited to 'tvix/eval') diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index 2594318429..9394dce484 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -787,6 +787,11 @@ impl Compiler<'_> { self.scope_mut().mark_initialised(set_idx); self.emit_force(pattern); + let ellipsis = pattern.ellipsis_token().is_some(); + if !ellipsis { + self.push_op(OpCode::OpValidateClosedFormals, pattern); + } + // Similar to `let ... in ...`, we now do multiple passes over // the bindings to first declare them, then populate them, and // then finalise any necessary recursion into the scope. @@ -837,16 +842,10 @@ impl Compiler<'_> { } } - // TODO: strictly check if all keys have been consumed if - // there is no ellipsis. - let ellipsis = pattern.ellipsis_token().is_some(); - if !ellipsis { - self.emit_warning(pattern, WarningKind::NotImplemented("closed formals")); - } - Formals { arguments, ellipsis, + span, } } diff --git a/tvix/eval/src/errors.rs b/tvix/eval/src/errors.rs index 33b12daa5d..a3fe387609 100644 --- a/tvix/eval/src/errors.rs +++ b/tvix/eval/src/errors.rs @@ -1,5 +1,5 @@ use crate::spans::ToSpan; -use crate::value::CoercionKind; +use crate::value::{CoercionKind, NixString}; use std::io; use std::path::PathBuf; use std::rc::Rc; @@ -132,6 +132,12 @@ pub enum ErrorKind { /// Errors converting JSON to a value FromJsonError(String), + /// An unexpected argument was supplied to a function that takes formal parameters + UnexpectedArgument { + arg: NixString, + formals_span: Span, + }, + /// Tvix internal warning for features triggered by users that are /// not actually implemented yet, and without which eval can not /// proceed. @@ -357,6 +363,14 @@ to a missing value in the attribute set(s) included via `with`."#, write!(f, "Error converting JSON to a Nix value: {msg}") } + ErrorKind::UnexpectedArgument { arg, .. } => { + write!( + f, + "Unexpected argument `{}` supplied to function", + arg.as_str() + ) + } + ErrorKind::NotImplemented(feature) => { write!(f, "feature not yet implemented in Tvix: {}", feature) } @@ -606,6 +620,7 @@ impl Error { ErrorKind::DuplicateAttrsKey { .. } => "in this attribute set", ErrorKind::InvalidAttributeName(_) => "in this attribute set", ErrorKind::PathResolution(_) => "in this path literal", + ErrorKind::UnexpectedArgument { .. } => "in this function call", // The spans for some errors don't have any more descriptive stuff // in them, or we don't utilise it yet. @@ -675,6 +690,7 @@ impl Error { ErrorKind::ImportCompilerError { .. } => "E028", ErrorKind::IO { .. } => "E029", ErrorKind::FromJsonError { .. } => "E030", + ErrorKind::UnexpectedArgument { .. } => "E031", // Placeholder error while Tvix is under construction. ErrorKind::NotImplemented(_) => "E999", @@ -720,6 +736,21 @@ impl Error { labels } + ErrorKind::UnexpectedArgument { formals_span, .. } => { + vec![ + SpanLabel { + label: self.span_label(), + span: self.span, + style: SpanStyle::Primary, + }, + SpanLabel { + label: Some("the accepted arguments".into()), + span: *formals_span, + style: SpanStyle::Secondary, + }, + ] + } + // All other errors pretty much have the same shape. _ => { vec![SpanLabel { diff --git a/tvix/eval/src/opcode.rs b/tvix/eval/src/opcode.rs index ee1684d7cf..6b2783716c 100644 --- a/tvix/eval/src/opcode.rs +++ b/tvix/eval/src/opcode.rs @@ -98,6 +98,12 @@ pub enum OpCode { OpAttrsTrySelect, OpHasAttr, + /// Throw an error if the attribute set at the top of the stack has any attributes + /// other than those listed in the formals of the current lambda + /// + /// Panics if the current frame is not a lambda with formals + OpValidateClosedFormals, + // `with`-handling OpPushWith(StackIdx), OpPopWith, diff --git a/tvix/eval/src/tests/tvix_tests/eval-fail-closed-formals.nix b/tvix/eval/src/tests/tvix_tests/eval-fail-closed-formals.nix new file mode 100644 index 0000000000..937604c563 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-fail-closed-formals.nix @@ -0,0 +1 @@ +({x}: x) {x = 1; y = 2;} diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index e9d5a239a3..6ee3efee67 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/tvix/eval/src/value/attrs.rs @@ -202,7 +202,7 @@ impl NixAttrs { self.0.contains(key) } - /// Provide an iterator over all values of the attribute set. + /// Construct an iterator over all the key-value pairs in the attribute set. #[allow(clippy::needless_lifetimes)] pub fn iter<'a>(&'a self) -> Iter> { Iter(match &self.0 { @@ -215,11 +215,20 @@ impl NixAttrs { } => KeyValue::KV { name, value, - at: IterKV::Name, + at: IterKV::default(), }, }) } + /// Construct an iterator over all the keys of the attribute set + pub fn keys(&self) -> Keys { + Keys(match &self.0 { + AttrsRep::Empty => KeysInner::Empty, + AttrsRep::Map(m) => KeysInner::Map(m.keys()), + AttrsRep::KV { .. } => KeysInner::KV(IterKV::default()), + }) + } + /// 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 { @@ -405,13 +414,24 @@ fn set_attr(attrs: &mut NixAttrs, key: NixString, value: Value) -> Result<(), Er /// Internal helper type to track the iteration status of an iterator /// over the name/value representation. -#[derive(Debug)] +#[derive(Debug, Default)] pub enum IterKV { + #[default] Name, Value, Done, } +impl IterKV { + fn next(&mut self) { + match *self { + Self::Name => *self = Self::Value, + Self::Value => *self = Self::Done, + Self::Done => {} + } + } +} + /// Iterator representation over the keys *and* values of an attribute /// set. #[derive(Debug)] @@ -443,12 +463,12 @@ impl<'a> Iterator for Iter> { KeyValue::KV { name, value, at } => match at { IterKV::Name => { - *at = IterKV::Value; + at.next(); Some((NixString::NAME_REF, name)) } IterKV::Value => { - *at = IterKV::Done; + at.next(); Some((NixString::VALUE_REF, value)) } @@ -457,3 +477,31 @@ impl<'a> Iterator for Iter> { } } } + +enum KeysInner<'a> { + Empty, + KV(IterKV), + Map(btree_map::Keys<'a, NixString, Value>), +} + +pub struct Keys<'a>(KeysInner<'a>); + +impl<'a> Iterator for Keys<'a> { + type Item = &'a NixString; + + fn next(&mut self) -> Option { + match &mut self.0 { + KeysInner::Empty => None, + KeysInner::KV(at @ IterKV::Name) => { + at.next(); + Some(NixString::NAME_REF) + } + KeysInner::KV(at @ IterKV::Value) => { + at.next(); + Some(NixString::VALUE_REF) + } + KeysInner::KV(IterKV::Done) => None, + KeysInner::Map(m) => m.next(), + } + } +} diff --git a/tvix/eval/src/value/function.rs b/tvix/eval/src/value/function.rs index e58aab19c0..a3aa9325f5 100644 --- a/tvix/eval/src/value/function.rs +++ b/tvix/eval/src/value/function.rs @@ -2,9 +2,12 @@ use std::{ cell::{Ref, RefCell, RefMut}, collections::HashMap, + hash::Hash, rc::Rc, }; +use codemap::Span; + use crate::{ chunk::Chunk, upvalues::{UpvalueCarrier, Upvalues}, @@ -19,6 +22,23 @@ pub(crate) struct Formals { /// Do the formals of this function accept extra arguments pub(crate) ellipsis: bool, + + /// The span of the formals themselves, to use to emit errors + pub(crate) span: Span, +} + +impl Formals { + /// Returns true if the given arg is a valid argument to these formals. + /// + /// This is true if it is either listed in the list of arguments, or the formals have an + /// ellipsis + pub(crate) fn contains(&self, arg: &Q) -> bool + where + Q: ?Sized + Hash + Eq, + NixString: std::borrow::Borrow, + { + self.ellipsis || self.arguments.contains_key(&arg) + } } /// The opcodes for a thunk or closure, plus the number of diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs index 7fcdb9ea73..65662ed555 100644 --- a/tvix/eval/src/vm.rs +++ b/tvix/eval/src/vm.rs @@ -500,6 +500,21 @@ impl<'o> VM<'o> { self.push(Value::Bool(result)); } + OpCode::OpValidateClosedFormals => { + let formals = self.frame().lambda.formals.as_ref().expect( + "OpValidateClosedFormals called within the frame of a lambda without formals", + ); + let args = self.peek(0).to_attrs().map_err(|err| self.error(err))?; + for arg in args.keys() { + if !formals.contains(arg) { + return Err(self.error(ErrorKind::UnexpectedArgument { + arg: arg.clone(), + formals_span: formals.span, + })); + } + } + } + OpCode::OpList(Count(count)) => { let list = NixList::construct(count, self.stack.split_off(self.stack.len() - count)); -- cgit 1.4.1