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 <tazjin@tvl.su>
Tested-by: BuildkiteCI
This commit is contained in:
Griffin Smith 2022-10-12 23:53:03 -04:00 committed by tazjin
parent e63d14419f
commit 2a3d498104
7 changed files with 133 additions and 13 deletions

View file

@ -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,
}
}

View file

@ -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 {

View file

@ -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,

View file

@ -0,0 +1 @@
({x}: x) {x = 1; y = 2;}

View file

@ -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<KeyValue<'a>> {
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<Value>) -> Result<Self, ErrorKind> {
@ -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<'a>> {
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<KeyValue<'a>> {
}
}
}
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<Self::Item> {
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(),
}
}
}

View file

@ -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<Q>(&self, arg: &Q) -> bool
where
Q: ?Sized + Hash + Eq,
NixString: std::borrow::Borrow<Q>,
{
self.ellipsis || self.arguments.contains_key(&arg)
}
}
/// The opcodes for a thunk or closure, plus the number of

View file

@ -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));