feat(tvix/eval): Validate closed formals r/5154

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

7 files changed, 133 insertions, 13 deletions

diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs
index 259431842956..9394dce48453 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 33b12daa5d82..a3fe3876093c 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 ee1684d7cf0f..6b2783716cc8 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 000000000000..937604c563e9
--- /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 e9d5a239a3cf..6ee3efee679b 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<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(),
+        }
+    }
+}
diff --git a/tvix/eval/src/value/function.rs b/tvix/eval/src/value/function.rs
index e58aab19c0e7..a3aa9325f598 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<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
diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs
index 7fcdb9ea739b..65662ed555d3 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));