From dfe137786c98f7e610cf95dd7cbc1fc476a766fd Mon Sep 17 00:00:00 2001 From: Aspen Smith Date: Fri, 5 Jul 2024 20:29:41 -0400 Subject: refactor(tvix/eval): Builderize Evaluation Make constructing of a new Evaluation use the builder pattern rather than setting public mutable fields. This is currently a pure refactor (no functionality has changed) but has a few advantages: - We've encapsulated the internals of the fields in Evaluation, meaning we can change them without too much breakage of clients - We have type safety that prevents us from ever changing the fields of an Evaluation after it's built (which matters more in a world where we reuse Evaluations). More importantly, this paves the road for doing different things with the construction of an Evaluation - notably, sharing certain things like the GlobalsMap across subsequent evaluations in eg the REPL. Fixes: b/262 Change-Id: I4a27116faac14cdd144fc7c992d14ae095a1aca4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11956 Tested-by: BuildkiteCI Autosubmit: aspen Reviewed-by: flokli --- tvix/eval/src/lib.rs | 259 ++++++++++++++++++++++++++++++--------- tvix/eval/src/tests/nix_tests.rs | 13 +- tvix/eval/src/tests/one_offs.rs | 9 +- 3 files changed, 216 insertions(+), 65 deletions(-) (limited to 'tvix/eval/src') diff --git a/tvix/eval/src/lib.rs b/tvix/eval/src/lib.rs index 97690d13610e..bfdd30154acf 100644 --- a/tvix/eval/src/lib.rs +++ b/tvix/eval/src/lib.rs @@ -64,6 +64,192 @@ pub use crate::value::{Builtin, CoercionKind, NixAttrs, NixList, NixString, Valu #[cfg(feature = "impure")] pub use crate::io::StdIO; +/// Builder for building an [`Evaluation`]. +/// +/// Construct an [`EvaluationBuilder`] by calling one of: +/// +/// - [`Evaluation::builder`] / [`EvaluationBuilder::new`] +/// - [`Evaluation::builder_impure`] [`EvaluationBuilder::new_impure`] +/// - [`Evaluation::builder_pure`] [`EvaluationBuilder::new_pure`] +/// +/// Then configure the fields by calling the various methods on [`EvaluationBuilder`], and finally +/// call [`build`](Self::build) to construct an [`Evaluation`] +pub struct EvaluationBuilder<'co, 'ro, 'env, IO> { + source_map: SourceCode, + builtins: Vec<(&'static str, Value)>, + src_builtins: Vec<(&'static str, &'static str)>, + env: Option<&'env HashMap>, + io_handle: IO, + enable_import: bool, + strict: bool, + nix_path: Option, + compiler_observer: Option<&'co mut dyn CompilerObserver>, + runtime_observer: Option<&'ro mut dyn RuntimeObserver>, +} + +// NOTE(aspen): The methods here are intentionally incomplete; feel free to add new ones (ideally +// with similar naming conventions to the ones already present) but don't expose fields publically! +impl<'co, 'ro, 'env, IO> EvaluationBuilder<'co, 'ro, 'env, IO> { + pub fn new(io_handle: IO) -> Self { + let mut builtins = builtins::pure_builtins(); + builtins.extend(builtins::placeholders()); // these are temporary + + Self { + source_map: SourceCode::default(), + enable_import: false, + io_handle, + builtins, + src_builtins: vec![], + env: None, + strict: false, + nix_path: None, + compiler_observer: None, + runtime_observer: None, + } + } + + pub fn build(self) -> Evaluation<'co, 'ro, 'env, IO> { + Evaluation { + source_map: self.source_map, + builtins: self.builtins, + src_builtins: self.src_builtins, + env: self.env, + io_handle: self.io_handle, + enable_import: self.enable_import, + strict: self.strict, + nix_path: self.nix_path, + compiler_observer: self.compiler_observer, + runtime_observer: self.runtime_observer, + } + } + + pub fn io_handle(self, io_handle: IO2) -> EvaluationBuilder<'co, 'ro, 'env, IO2> { + EvaluationBuilder { + io_handle, + source_map: self.source_map, + builtins: self.builtins, + src_builtins: self.src_builtins, + env: self.env, + enable_import: self.enable_import, + strict: self.strict, + nix_path: self.nix_path, + compiler_observer: self.compiler_observer, + runtime_observer: self.runtime_observer, + } + } + + pub fn with_enable_import(self, enable_import: bool) -> Self { + Self { + enable_import, + ..self + } + } + + pub fn disable_import(self) -> Self { + self.with_enable_import(false) + } + + pub fn enable_import(self) -> Self { + self.with_enable_import(true) + } + + pub fn add_builtins(mut self, builtins: I) -> Self + where + I: IntoIterator, + { + self.builtins.extend(builtins); + self + } + + pub fn with_strict(self, strict: bool) -> Self { + Self { strict, ..self } + } + + pub fn strict(self) -> Self { + self.with_strict(true) + } + + pub fn add_src_builtin(mut self, name: &'static str, src: &'static str) -> Self { + self.src_builtins.push((name, src)); + self + } + + pub fn nix_path(self, nix_path: Option) -> Self { + Self { nix_path, ..self } + } + + pub fn env(self, env: Option<&'env HashMap>) -> Self { + Self { env, ..self } + } + + pub fn compiler_observer( + self, + compiler_observer: Option<&'co mut dyn CompilerObserver>, + ) -> Self { + Self { + compiler_observer, + ..self + } + } + + pub fn set_compiler_observer( + &mut self, + compiler_observer: Option<&'co mut dyn CompilerObserver>, + ) { + self.compiler_observer = compiler_observer; + } + + pub fn runtime_observer(self, runtime_observer: Option<&'ro mut dyn RuntimeObserver>) -> Self { + Self { + runtime_observer, + ..self + } + } + + pub fn set_runtime_observer(&mut self, runtime_observer: Option<&'ro mut dyn RuntimeObserver>) { + self.runtime_observer = runtime_observer; + } +} + +impl<'co, 'ro, 'env, IO> EvaluationBuilder<'co, 'ro, 'env, IO> { + pub fn source_map(&self) -> &SourceCode { + &self.source_map + } +} + +impl<'co, 'ro, 'env> EvaluationBuilder<'co, 'ro, 'env, Box> { + /// Initialize an `Evaluation`, without the import statement available, and + /// all IO operations stubbed out. + pub fn new_pure() -> Self { + Self::new(Box::new(DummyIO) as Box).with_enable_import(false) + } + + #[cfg(feature = "impure")] + /// Configure an `Evaluation` to have impure features available + /// with the given I/O implementation. + /// + /// If no I/O implementation is supplied, [`StdIO`] is used by + /// default. + pub fn enable_impure(mut self, io: Option>) -> Self { + self.io_handle = io.unwrap_or_else(|| Box::new(StdIO) as Box); + self.enable_import = true; + self.builtins.extend(builtins::impure_builtins()); + + // Make `NIX_PATH` resolutions work by default, unless the + // user already overrode this with something else. + if self.nix_path.is_none() { + self.nix_path = std::env::var("NIX_PATH").ok(); + } + self + } + + #[cfg(feature = "impure")] + /// Initialise an `Evaluation`, with all impure features turned on by default. + pub fn new_impure() -> Self { + Self::new_pure().enable_impure(None) + } +} + /// An `Evaluation` represents how a piece of Nix code is evaluated. It can be /// instantiated and configured directly, or it can be accessed through the /// various simplified helper methods available below. @@ -79,42 +265,42 @@ pub struct Evaluation<'co, 'ro, 'env, IO> { /// /// This defaults to all pure builtins. Users might want to add /// the set of impure builtins, or other custom builtins. - pub builtins: Vec<(&'static str, Value)>, + builtins: Vec<(&'static str, Value)>, /// Set of builtins that are implemented in Nix itself and should /// be compiled and inserted in the builtins set. - pub src_builtins: Vec<(&'static str, &'static str)>, + src_builtins: Vec<(&'static str, &'static str)>, /// Top-level variables to define in the evaluation - pub env: Option<&'env HashMap>, + env: Option<&'env HashMap>, /// Implementation of file-IO to use during evaluation, e.g. for /// impure builtins. /// /// Defaults to [`DummyIO`] if not set explicitly. - pub io_handle: IO, + io_handle: IO, /// Determines whether the `import` builtin should be made /// available. Note that this depends on the `io_handle` being /// able to read the files specified as arguments to `import`. - pub enable_import: bool, + enable_import: bool, /// Determines whether the returned value should be strictly /// evaluated, that is whether its list and attribute set elements /// should be forced recursively. - pub strict: bool, + strict: bool, /// (optional) Nix search path, e.g. the value of `NIX_PATH` used /// for resolving items on the search path (such as ``). - pub nix_path: Option, + nix_path: Option, /// (optional) compiler observer for reporting on compilation /// details, like the emitted bytecode. - pub compiler_observer: Option<&'co mut dyn CompilerObserver>, + compiler_observer: Option<&'co mut dyn CompilerObserver>, /// (optional) runtime observer, for reporting on execution steps /// of Nix code. - pub runtime_observer: Option<&'ro mut dyn RuntimeObserver>, + runtime_observer: Option<&'ro mut dyn RuntimeObserver>, } /// Result of evaluating a piece of Nix code. If evaluation succeeded, a value @@ -136,61 +322,20 @@ pub struct EvaluationResult { pub expr: Option, } -impl<'co, 'ro, 'env, IO> Evaluation<'co, 'ro, 'env, IO> -where - IO: AsRef + 'static, -{ - /// Initialize an `Evaluation`. - pub fn new(io_handle: IO, enable_import: bool) -> Self { - let mut builtins = builtins::pure_builtins(); - builtins.extend(builtins::placeholders()); // these are temporary - - Self { - source_map: SourceCode::default(), - enable_import, - io_handle, - builtins, - src_builtins: vec![], - env: None, - strict: false, - nix_path: None, - compiler_observer: None, - runtime_observer: None, - } +impl<'co, 'ro, 'env, IO> Evaluation<'co, 'ro, 'env, IO> { + pub fn builder(io_handle: IO) -> EvaluationBuilder<'co, 'ro, 'env, IO> { + EvaluationBuilder::new(io_handle) } } impl<'co, 'ro, 'env> Evaluation<'co, 'ro, 'env, Box> { - /// Initialize an `Evaluation`, without the import statement available, and - /// all IO operations stubbed out. - pub fn new_pure() -> Self { - Self::new(Box::new(DummyIO) as Box, false) - } - #[cfg(feature = "impure")] - /// Configure an `Evaluation` to have impure features available - /// with the given I/O implementation. - /// - /// If no I/O implementation is supplied, [`StdIO`] is used by - /// default. - pub fn enable_impure(&mut self, io: Option>) { - self.io_handle = io.unwrap_or_else(|| Box::new(StdIO) as Box); - self.enable_import = true; - self.builtins.extend(builtins::impure_builtins()); - - // Make `NIX_PATH` resolutions work by default, unless the - // user already overrode this with something else. - if self.nix_path.is_none() { - self.nix_path = std::env::var("NIX_PATH").ok(); - } + pub fn builder_impure() -> EvaluationBuilder<'co, 'ro, 'env, Box> { + EvaluationBuilder::new_impure() } - #[cfg(feature = "impure")] - /// Initialise an `Evaluation`, with all impure features turned on by default. - pub fn new_impure() -> Self { - let mut eval = Self::new_pure(); - eval.enable_impure(None); - eval + pub fn builder_pure() -> EvaluationBuilder<'co, 'ro, 'env, Box> { + EvaluationBuilder::new_pure() } } diff --git a/tvix/eval/src/tests/nix_tests.rs b/tvix/eval/src/tests/nix_tests.rs index 17968e4bdb09..cdaed193f206 100644 --- a/tvix/eval/src/tests/nix_tests.rs +++ b/tvix/eval/src/tests/nix_tests.rs @@ -48,9 +48,10 @@ fn eval_test(code_path: PathBuf, expect_success: bool) { let code = std::fs::read_to_string(&code_path).expect("should be able to read test code"); - let mut eval = crate::Evaluation::new_impure(); - eval.strict = true; - eval.builtins.extend(mock_builtins::builtins()); + let eval = crate::Evaluation::builder_impure() + .strict() + .add_builtins(mock_builtins::builtins()) + .build(); let result = eval.evaluate(code, Some(code_path.clone())); let failed = match result.value { @@ -128,8 +129,10 @@ fn identity(#[files("src/tests/tvix_tests/identity-*.nix")] code_path: PathBuf) let code = std::fs::read_to_string(code_path).expect("should be able to read test code"); - let mut eval = crate::Evaluation::new(Box::new(crate::StdIO) as Box, false); - eval.strict = true; + let eval = crate::Evaluation::builder(Box::new(crate::StdIO) as Box) + .disable_import() + .strict() + .build(); let result = eval.evaluate(&code, None); assert!( diff --git a/tvix/eval/src/tests/one_offs.rs b/tvix/eval/src/tests/one_offs.rs index 21e9144baf64..49ed713fc96c 100644 --- a/tvix/eval/src/tests/one_offs.rs +++ b/tvix/eval/src/tests/one_offs.rs @@ -5,8 +5,9 @@ fn test_source_builtin() { // Test an evaluation with a source-only builtin. The test ensures // that the artificially constructed thunking is correct. - let mut eval = Evaluation::new_pure(); - eval.src_builtins.push(("testSourceBuiltin", "42")); + let eval = Evaluation::builder_pure() + .add_src_builtin("testSourceBuiltin", "42") + .build(); let result = eval.evaluate("builtins.testSourceBuiltin", None); assert!( @@ -25,7 +26,9 @@ fn test_source_builtin() { #[test] fn skip_broken_bytecode() { - let result = Evaluation::new_pure().evaluate(/* code = */ "x", None); + let result = Evaluation::builder_pure() + .build() + .evaluate(/* code = */ "x", None); assert_eq!(result.errors.len(), 1); -- cgit 1.4.1