feat(tvix/eval): Give names to builtin arguments r/5268

Refactor the arguments of a Builtin to be a vec of a new BuiltinArgument
struct, which contains the old strictness boolean and also a static
`name` str - this is automatically determined via the ident for the
corresponding function argument in the proc-macro case, and passed in in
the cases where we're still manually calling Builtin::new.

Currently this name is unused, but in the future this can be used as
part of a documentation system for builtins.

Change-Id: Ib9dadb15b69bf8c9ea1983a4f4f197294a2394a6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7204
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI

6 files changed, 111 insertions, 57 deletions

diff --git a/tvix/eval/builtin-macros/src/lib.rs b/tvix/eval/builtin-macros/src/lib.rs
index 8e2c4b3f8776..8276e421c5d8 100644
--- a/tvix/eval/builtin-macros/src/lib.rs
+++ b/tvix/eval/builtin-macros/src/lib.rs
@@ -5,7 +5,9 @@ use proc_macro2::Span;
 use quote::{quote_spanned, ToTokens};
 use syn::parse::Parse;
 use syn::spanned::Spanned;
-use syn::{parse_macro_input, parse_quote, FnArg, Ident, Item, ItemMod, LitStr, PatType};
+use syn::{
+    parse_macro_input, parse_quote, FnArg, Ident, Item, ItemMod, LitStr, Pat, PatIdent, PatType,
+};
 
 struct BuiltinArgs {
     name: LitStr,
@@ -89,31 +91,54 @@ pub fn builtins(_args: TokenStream, item: TokenStream) -> TokenStream {
                     .into();
                 }
 
-                let strictness = f
+                let builtin_arguments = f
                     .sig
                     .inputs
                     .iter_mut()
                     .skip(1)
-                    .map(|input| {
-                        let mut lazy = false;
-                        if let FnArg::Typed(PatType { attrs, .. }) = input {
-                            attrs.retain(|attr| {
-                                attr.path.get_ident().into_iter().any(|id| {
-                                    if id == "lazy" {
-                                        lazy = true;
-                                        false
-                                    } else {
-                                        true
-                                    }
-                                })
-                            });
-                        }
-                        !lazy
+                    .map(|arg| {
+                        let mut strict = true;
+                        let name = match arg {
+                            FnArg::Receiver(_) => {
+                                return Err(quote_spanned!(arg.span() => {
+                                    compile_error!("Unexpected receiver argument in builtin")
+                                }))
+                            }
+                            FnArg::Typed(PatType { attrs, pat, .. }) => {
+                                attrs.retain(|attr| {
+                                    attr.path.get_ident().into_iter().any(|id| {
+                                        if id == "lazy" {
+                                            strict = false;
+                                            false
+                                        } else {
+                                            true
+                                        }
+                                    })
+                                });
+                                match pat.as_ref() {
+                                    Pat::Ident(PatIdent { ident, .. }) => ident.to_string(),
+                                    _ => "unknown".to_string(),
+                                }
+                            }
+                        };
+
+                        Ok(quote_spanned!(arg.span() => {
+                            crate::internal::BuiltinArgument {
+                                strict: #strict,
+                                name: #name,
+                            }
+                        }))
                     })
-                    .collect::<Vec<_>>();
+                    .collect::<Result<Vec<_>, _>>();
+
+                let builtin_arguments = match builtin_arguments {
+                    Ok(args) => args,
+                    Err(err) => return err.into(),
+                };
 
                 let fn_name = f.sig.ident.clone();
-                let args = (0..(f.sig.inputs.len() - 1))
+                let num_args = f.sig.inputs.len() - 1;
+                let args = (0..num_args)
                     .map(|n| Ident::new(&format!("arg_{n}"), Span::call_site()))
                     .collect::<Vec<_>>();
                 let mut reversed_args = args.clone();
@@ -122,7 +147,7 @@ pub fn builtins(_args: TokenStream, item: TokenStream) -> TokenStream {
                 builtins.push(quote_spanned! { builtin_attr.span() => {
                     crate::internal::Builtin::new(
                         #name,
-                        &[#(#strictness),*],
+                        &[#(#builtin_arguments),*],
                         |mut args: Vec<crate::Value>, vm: &mut crate::internal::VM| {
                             #(let #reversed_args = args.pop().unwrap();)*
                             #fn_name(vm, #(#args),*)
diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs
index 006cd9d7ac93..d4e7c4170364 100644
--- a/tvix/eval/src/builtins/impure.rs
+++ b/tvix/eval/src/builtins/impure.rs
@@ -12,7 +12,7 @@ use crate::{
     compiler::GlobalsMap,
     errors::ErrorKind,
     observer::NoOpObserver,
-    value::{Builtin, NixAttrs, Thunk},
+    value::{Builtin, BuiltinArgument, NixAttrs, Thunk},
     vm::VM,
     SourceCode, Value,
 };
@@ -111,7 +111,10 @@ pub fn builtins_import(globals: &Weak<GlobalsMap>, source: SourceCode) -> Builti
 
     Builtin::new(
         "import",
-        &[true],
+        &[BuiltinArgument {
+            strict: true,
+            name: "path",
+        }],
         move |mut args: Vec<Value>, vm: &mut VM| {
             let mut path = super::coerce_value_to_path(&args.pop().unwrap(), vm)?;
             if path.is_dir() {
diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index caa575fcf3bc..9b6e29db0a3d 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -5,6 +5,7 @@
 
 use crate::compiler::{GlobalsMap, GlobalsMapFunc};
 use crate::source::SourceCode;
+use crate::value::BuiltinArgument;
 use std::cmp::{self, Ordering};
 use std::collections::{BTreeMap, HashMap, HashSet};
 use std::path::PathBuf;
@@ -907,7 +908,16 @@ fn placeholders() -> Vec<Builtin> {
     vec![
         Builtin::new(
             "addErrorContext",
-            &[false, false],
+            &[
+                BuiltinArgument {
+                    strict: false,
+                    name: "context",
+                },
+                BuiltinArgument {
+                    strict: false,
+                    name: "value",
+                },
+            ],
             |mut args: Vec<Value>, vm: &mut VM| {
                 vm.emit_warning(WarningKind::NotImplemented("builtins.addErrorContext"));
                 Ok(args.pop().unwrap())
@@ -915,7 +925,10 @@ fn placeholders() -> Vec<Builtin> {
         ),
         Builtin::new(
             "unsafeDiscardStringContext",
-            &[true],
+            &[BuiltinArgument {
+                strict: true,
+                name: "s",
+            }],
             |mut args: Vec<Value>, vm: &mut VM| {
                 vm.emit_warning(WarningKind::NotImplemented(
                     "builtins.unsafeDiscardStringContext",
@@ -923,28 +936,35 @@ fn placeholders() -> Vec<Builtin> {
                 Ok(args.pop().unwrap())
             },
         ),
-        Builtin::new("derivation", &[true], |args: Vec<Value>, vm: &mut VM| {
-            vm.emit_warning(WarningKind::NotImplemented("builtins.derivation"));
-
-            // We do not implement derivations yet, so this function sets mock
-            // values on the fields that a real derivation would contain.
-            //
-            // Crucially this means we do not yet *validate* the values either.
-            let attrs = unwrap_or_clone_rc(args[0].to_attrs()?);
-            let attrs = attrs.update(NixAttrs::from_map(BTreeMap::from([
-                (
-                    "outPath".into(),
-                    "/nix/store/00000000000000000000000000000000-mock".into(),
-                ),
-                (
-                    "drvPath".into(),
-                    "/nix/store/00000000000000000000000000000000-mock.drv".into(),
-                ),
-                ("type".into(), "derivation".into()),
-            ])));
-
-            Ok(Value::Attrs(Rc::new(attrs)))
-        }),
+        Builtin::new(
+            "derivation",
+            &[BuiltinArgument {
+                strict: true,
+                name: "attrs",
+            }],
+            |args: Vec<Value>, vm: &mut VM| {
+                vm.emit_warning(WarningKind::NotImplemented("builtins.derivation"));
+
+                // We do not implement derivations yet, so this function sets mock
+                // values on the fields that a real derivation would contain.
+                //
+                // Crucially this means we do not yet *validate* the values either.
+                let attrs = unwrap_or_clone_rc(args[0].to_attrs()?);
+                let attrs = attrs.update(NixAttrs::from_map(BTreeMap::from([
+                    (
+                        "outPath".into(),
+                        "/nix/store/00000000000000000000000000000000-mock".into(),
+                    ),
+                    (
+                        "drvPath".into(),
+                        "/nix/store/00000000000000000000000000000000-mock.drv".into(),
+                    ),
+                    ("type".into(), "derivation".into()),
+                ])));
+
+                Ok(Value::Attrs(Rc::new(attrs)))
+            },
+        ),
     ]
 }
 // we set TVIX_CURRENT_SYSTEM in build.rs
diff --git a/tvix/eval/src/lib.rs b/tvix/eval/src/lib.rs
index 106f7d851cd3..3e160fa92cf6 100644
--- a/tvix/eval/src/lib.rs
+++ b/tvix/eval/src/lib.rs
@@ -37,7 +37,7 @@ pub use crate::vm::run_lambda;
 /// Internal-only parts of `tvix-eval`, exported for use in macros, but not part of the public
 /// interface of the crate.
 pub mod internal {
-    pub use crate::value::Builtin;
+    pub use crate::value::{Builtin, BuiltinArgument};
     pub use crate::vm::VM;
 }
 
diff --git a/tvix/eval/src/value/builtin.rs b/tvix/eval/src/value/builtin.rs
index fc8043176211..944efd85eb99 100644
--- a/tvix/eval/src/value/builtin.rs
+++ b/tvix/eval/src/value/builtin.rs
@@ -25,6 +25,14 @@ use std::{
 pub trait BuiltinFn: Fn(Vec<Value>, &mut VM) -> Result<Value, ErrorKind> {}
 impl<F: Fn(Vec<Value>, &mut VM) -> Result<Value, ErrorKind>> BuiltinFn for F {}
 
+/// Description of a single argument passed to a builtin
+pub struct BuiltinArgument {
+    /// Whether the argument should be forced before the underlying builtin function is called
+    pub strict: bool,
+    /// The name of the argument, to be used in docstrings and error messages
+    pub name: &'static str,
+}
+
 /// Represents a single built-in function which directly executes Rust
 /// code that operates on a Nix value.
 ///
@@ -40,10 +48,8 @@ impl<F: Fn(Vec<Value>, &mut VM) -> Result<Value, ErrorKind>> BuiltinFn for F {}
 #[derive(Clone)]
 pub struct Builtin {
     name: &'static str,
-    /// Array reference that describes how many arguments there are (usually 1
-    /// or 2) and whether they need to be forced. `true` causes the
-    /// corresponding argument to be forced before `func` is called.
-    strict_args: &'static [bool],
+    /// Array of arguments to the builtin.
+    arguments: &'static [BuiltinArgument],
     func: Rc<dyn BuiltinFn>,
 
     /// Partially applied function arguments.
@@ -53,12 +59,12 @@ pub struct Builtin {
 impl Builtin {
     pub fn new<F: BuiltinFn + 'static>(
         name: &'static str,
-        strict_args: &'static [bool],
+        arguments: &'static [BuiltinArgument],
         func: F,
     ) -> Self {
         Builtin {
             name,
-            strict_args,
+            arguments,
             func: Rc::new(func),
             partials: vec![],
         }
@@ -74,9 +80,9 @@ impl Builtin {
     pub fn apply(mut self, vm: &mut VM, arg: Value) -> Result<Value, ErrorKind> {
         self.partials.push(arg);
 
-        if self.partials.len() == self.strict_args.len() {
-            for (idx, force) in self.strict_args.iter().enumerate() {
-                if *force {
+        if self.partials.len() == self.arguments.len() {
+            for (idx, BuiltinArgument { strict, .. }) in self.arguments.iter().enumerate() {
+                if *strict {
                     self.partials[idx].force(vm)?;
                 }
             }
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index db72081e8dcf..f0bb3628d831 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -20,7 +20,7 @@ use crate::errors::ErrorKind;
 use crate::opcode::StackIdx;
 use crate::vm::VM;
 pub use attrs::NixAttrs;
-pub use builtin::Builtin;
+pub use builtin::{Builtin, BuiltinArgument};
 pub(crate) use function::Formals;
 pub use function::{Closure, Lambda};
 pub use list::NixList;