From e0a867cabff021348cc283b25467cfd40b8eb15a Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Tue, 16 Jan 2024 14:19:16 +0200 Subject: refactor(tvix/eval): generalize EvalIO container Don't restrict to a Box. There's still one or two places where we do restrict, this will be solved by b/262. Change-Id: Ic8d927d6ea81fa12d90b1e4352f35ffaafbd1adf Reviewed-on: https://cl.tvl.fyi/c/depot/+/10639 Tested-by: BuildkiteCI Reviewed-by: raitobezarius --- tvix/eval/src/lib.rs | 19 +++++++++++++------ tvix/eval/src/nix_search_path.rs | 38 +++++++++++++++++++------------------- tvix/eval/src/tests/mod.rs | 4 ++-- tvix/eval/src/vm/generators.rs | 9 ++++++++- tvix/eval/src/vm/mod.rs | 39 ++++++++++++++++++++++++--------------- 5 files changed, 66 insertions(+), 43 deletions(-) (limited to 'tvix/eval/src') diff --git a/tvix/eval/src/lib.rs b/tvix/eval/src/lib.rs index f852b1a324..72cfefee49 100644 --- a/tvix/eval/src/lib.rs +++ b/tvix/eval/src/lib.rs @@ -68,7 +68,7 @@ pub use crate::io::StdIO; /// /// Public fields are intended to be set by the caller. Setting all /// fields is optional. -pub struct Evaluation<'co, 'ro> { +pub struct Evaluation<'co, 'ro, IO> { /// Source code map used for error reporting. source_map: SourceCode, @@ -87,7 +87,7 @@ pub struct Evaluation<'co, 'ro> { /// impure builtins. /// /// Defaults to [`DummyIO`] if not set explicitly. - pub io_handle: Box, + pub io_handle: IO, /// Determines whether the `import` builtin should be made /// available. Note that this depends on the `io_handle` being @@ -131,7 +131,9 @@ pub struct EvaluationResult { pub expr: Option, } -impl<'co, 'ro> Default for Evaluation<'co, 'ro> { +/// TODO: this approach of creating the struct, then mutating its values +/// unnecessarily restricts the type of IO (b/262) +impl<'co, 'ro> Default for Evaluation<'co, 'ro, Box> { fn default() -> Self { let source_map = SourceCode::default(); @@ -142,7 +144,7 @@ impl<'co, 'ro> Default for Evaluation<'co, 'ro> { source_map, builtins, src_builtins: vec![], - io_handle: Box::new(DummyIO {}), + io_handle: Box::new(DummyIO {}) as Box, enable_import: false, strict: false, nix_path: None, @@ -152,7 +154,7 @@ impl<'co, 'ro> Default for Evaluation<'co, 'ro> { } } -impl<'co, 'ro> Evaluation<'co, 'ro> { +impl<'co, 'ro> Evaluation<'co, 'ro, Box> { #[cfg(feature = "impure")] /// Initialise an `Evaluation`, with all impure features turned on by default. pub fn new_impure() -> Self { @@ -166,7 +168,12 @@ impl<'co, 'ro> Evaluation<'co, 'ro> { eval } +} +impl<'co, 'ro, IO> Evaluation<'co, 'ro, IO> +where + IO: AsRef + 'static, +{ /// Clone the reference to the contained source code map. This is used after /// an evaluation for pretty error printing. pub fn source_map(&self) -> SourceCode { @@ -233,7 +240,7 @@ impl<'co, 'ro> Evaluation<'co, 'ro> { let compiler_observer = self.compiler_observer.take().unwrap_or(&mut noop_observer); // Insert a storeDir builtin *iff* a store directory is present. - if let Some(store_dir) = self.io_handle.store_dir() { + if let Some(store_dir) = self.io_handle.as_ref().store_dir() { self.builtins.push(("storeDir", store_dir.into())); } diff --git a/tvix/eval/src/nix_search_path.rs b/tvix/eval/src/nix_search_path.rs index f86cffd6d6..2f6d444475 100644 --- a/tvix/eval/src/nix_search_path.rs +++ b/tvix/eval/src/nix_search_path.rs @@ -68,11 +68,10 @@ impl NixSearchPathEntry { /// /// For prefixed path, an entry matches if the prefix does. // TODO(tazjin): verify these rules in the C++ impl, seems fishy. - fn resolve( - &self, - io: &mut dyn EvalIO, - lookup_path: &Path, - ) -> Result, ErrorKind> { + fn resolve(&self, io: IO, lookup_path: &Path) -> Result, ErrorKind> + where + IO: AsRef, + { let path = match self { NixSearchPathEntry::Path(parent) => canonicalise(parent.join(lookup_path))?, @@ -85,7 +84,7 @@ impl NixSearchPathEntry { } }; - if io.path_exists(&path).map_err(|e| ErrorKind::IO { + if io.as_ref().path_exists(&path).map_err(|e| ErrorKind::IO { path: Some(path.clone()), error: e.into(), })? { @@ -124,17 +123,18 @@ pub struct NixSearchPath { impl NixSearchPath { /// Attempt to resolve the given `path` within this [`NixSearchPath`] using the /// path resolution rules for `<...>`-style paths - pub fn resolve

( + pub fn resolve( &self, - io: &mut dyn EvalIO, + io: IO, path: P, ) -> Result, ErrorKind> where P: AsRef, + IO: AsRef, { let path = path.as_ref(); for entry in &self.entries { - if let Some(p) = entry.resolve(io, path)? { + if let Some(p) = entry.resolve(&io, path)? { return Ok(Ok(p)); } } @@ -204,8 +204,8 @@ mod tests { #[test] fn simple_dir() { let nix_search_path = NixSearchPath::from_str("./.").unwrap(); - let mut io = StdIO {}; - let res = nix_search_path.resolve(&mut io, "src").unwrap(); + let io = Box::new(StdIO {}) as Box; + let res = nix_search_path.resolve(&io, "src").unwrap(); assert_eq!( res.unwrap().to_path_buf(), current_dir().unwrap().join("src").clean() @@ -215,8 +215,8 @@ mod tests { #[test] fn failed_resolution() { let nix_search_path = NixSearchPath::from_str("./.").unwrap(); - let mut io = StdIO {}; - let err = nix_search_path.resolve(&mut io, "nope").unwrap(); + let io = Box::new(StdIO {}) as Box; + let err = nix_search_path.resolve(&io, "nope").unwrap(); assert!( matches!(err, Err(CatchableErrorKind::NixPathResolution(..))), "err = {err:?}" @@ -226,16 +226,16 @@ mod tests { #[test] fn second_in_path() { let nix_search_path = NixSearchPath::from_str("./.:/").unwrap(); - let mut io = StdIO {}; - let res = nix_search_path.resolve(&mut io, "etc").unwrap(); + let io = Box::new(StdIO {}) as Box; + let res = nix_search_path.resolve(&io, "etc").unwrap(); assert_eq!(res.unwrap().to_path_buf(), Path::new("/etc")); } #[test] fn prefix() { let nix_search_path = NixSearchPath::from_str("/:tvix=.").unwrap(); - let mut io = StdIO {}; - let res = nix_search_path.resolve(&mut io, "tvix/src").unwrap(); + let io = Box::new(StdIO {}) as Box; + let res = nix_search_path.resolve(&io, "tvix/src").unwrap(); assert_eq!( res.unwrap().to_path_buf(), current_dir().unwrap().join("src").clean() @@ -245,8 +245,8 @@ mod tests { #[test] fn matching_prefix() { let nix_search_path = NixSearchPath::from_str("/:tvix=.").unwrap(); - let mut io = StdIO {}; - let res = nix_search_path.resolve(&mut io, "tvix").unwrap(); + let io = Box::new(StdIO {}) as Box; + let res = nix_search_path.resolve(&io, "tvix").unwrap(); assert_eq!(res.unwrap().to_path_buf(), current_dir().unwrap().clean()); } } diff --git a/tvix/eval/src/tests/mod.rs b/tvix/eval/src/tests/mod.rs index 7509379fda..e3d3445a45 100644 --- a/tvix/eval/src/tests/mod.rs +++ b/tvix/eval/src/tests/mod.rs @@ -1,4 +1,4 @@ -use crate::value::Value; +use crate::{value::Value, EvalIO}; use builtin_macros::builtins; use pretty_assertions::assert_eq; use rstest::rstest; @@ -119,7 +119,7 @@ fn identity(#[files("src/tests/tvix_tests/identity-*.nix")] code_path: PathBuf) let eval = crate::Evaluation { strict: true, - io_handle: Box::new(crate::StdIO), + io_handle: Box::new(crate::StdIO) as Box, ..Default::default() }; diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index f2ce73a0c7..3d2fd9d266 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/tvix/eval/src/vm/generators.rs @@ -224,7 +224,10 @@ pub fn pin_generator( Box::pin(f) } -impl<'o> VM<'o> { +impl<'o, IO> VM<'o, IO> +where + IO: AsRef + 'static, +{ /// Helper function to re-enqueue the current generator while it /// is awaiting a value. fn reenqueue_generator(&mut self, name: &'static str, span: LightSpan, generator: Generator) { @@ -411,6 +414,7 @@ impl<'o> VM<'o> { VMRequest::PathImport(path) => { let imported = self .io_handle + .as_ref() .import_path(&path) .map_err(|e| ErrorKind::IO { path: Some(path), @@ -424,6 +428,7 @@ impl<'o> VM<'o> { VMRequest::ReadToString(path) => { let content = self .io_handle + .as_ref() .read_to_string(&path) .map_err(|e| ErrorKind::IO { path: Some(path), @@ -437,6 +442,7 @@ impl<'o> VM<'o> { VMRequest::PathExists(path) => { let exists = self .io_handle + .as_ref() .path_exists(&path) .map_err(|e| ErrorKind::IO { path: Some(path), @@ -451,6 +457,7 @@ impl<'o> VM<'o> { VMRequest::ReadDir(path) => { let dir = self .io_handle + .as_ref() .read_dir(&path) .map_err(|e| ErrorKind::IO { path: Some(path), diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 398a02cc78..5238fdc066 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -48,7 +48,7 @@ trait GetSpan { fn get_span(self) -> Span; } -impl<'o> GetSpan for &VM<'o> { +impl<'o, IO> GetSpan for &VM<'o, IO> { fn get_span(self) -> Span { self.reasonable_span.span() } @@ -75,12 +75,12 @@ impl GetSpan for Span { /// Internal helper trait for ergonomically converting from a `Result` to a `Result` using the current span of a call frame, /// and chaining the VM's frame stack around it for printing a cause chain. -trait WithSpan { - fn with_span(self, top_span: S, vm: &VM) -> Result; +trait WithSpan { + fn with_span(self, top_span: S, vm: &VM) -> Result; } -impl WithSpan for Result { - fn with_span(self, top_span: S, vm: &VM) -> Result { +impl WithSpan for Result { + fn with_span(self, top_span: S, vm: &VM) -> Result { match self { Ok(something) => Ok(something), Err(kind) => { @@ -149,7 +149,7 @@ impl CallFrame { /// Construct an error result from the given ErrorKind and the source span /// of the current instruction. - pub fn error(&self, vm: &VM, kind: ErrorKind) -> Result { + pub fn error(&self, vm: &VM, kind: ErrorKind) -> Result { Err(kind).with_span(self, vm) } @@ -249,7 +249,7 @@ impl ImportCache { } } -struct VM<'o> { +struct VM<'o, IO> { /// VM's frame stack, representing the execution contexts the VM is working /// through. Elements are usually pushed when functions are called, or /// thunks are being forced. @@ -280,7 +280,7 @@ struct VM<'o> { /// Implementation of I/O operations used for impure builtins and /// features like `import`. - io_handle: Box, + io_handle: IO, /// Runtime observer which can print traces of runtime operations. observer: &'o mut dyn RuntimeObserver, @@ -324,10 +324,13 @@ struct VM<'o> { try_eval_frames: Vec, } -impl<'o> VM<'o> { +impl<'o, IO> VM<'o, IO> +where + IO: AsRef + 'static, +{ pub fn new( nix_search_path: NixSearchPath, - io_handle: Box, + io_handle: IO, observer: &'o mut dyn RuntimeObserver, globals: Rc, reasonable_span: LightSpan, @@ -858,7 +861,7 @@ impl<'o> VM<'o> { Value::UnresolvedPath(path) => { let resolved = self .nix_search_path - .resolve(&mut *self.io_handle, *path) + .resolve(&self.io_handle, *path) .with_span(&frame, self)?; self.stack.push(resolved.into()); } @@ -914,7 +917,10 @@ impl<'o> VM<'o> { } /// Implementation of helper functions for the runtime logic above. -impl<'o> VM<'o> { +impl<'o, IO> VM<'o, IO> +where + IO: AsRef + 'static, +{ pub(crate) fn stack_pop(&mut self) -> Value { self.stack.pop().expect("runtime stack empty") } @@ -1301,14 +1307,17 @@ async fn final_deep_force(co: GenCo) -> Result { Ok(generators::request_deep_force(&co, value).await) } -pub fn run_lambda( +pub fn run_lambda( nix_search_path: NixSearchPath, - io_handle: Box, + io_handle: IO, observer: &mut dyn RuntimeObserver, globals: Rc, lambda: Rc, strict: bool, -) -> EvalResult { +) -> EvalResult +where + IO: AsRef + 'static, +{ // Retain the top-level span of the expression in this lambda, as // synthetic "calls" in deep_force will otherwise not have a span // to fall back to. -- cgit 1.4.1