From 4fba57c2c90f2e7b02da9187e59f8d64deef3fb2 Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Sat, 30 Dec 2023 21:36:48 +0100 Subject: refactor(tvix/eval): remove code and location from struct Instead, it's passed in the evaluate/compile_only functions, which feels more naturally. It lets us set up the Evaluation struct long before we actually feed it with data to evaluate. Now that Evaluation::new() would be accepting an empty list of arguments, we can simply implement Default, making things a bit more idiomatic. Change-Id: I4369658634909a0c504fdffa18242a130daa0239 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10475 Tested-by: BuildkiteCI Reviewed-by: tazjin Autosubmit: flokli --- tvix/cli/src/main.rs | 8 ++-- tvix/eval/src/lib.rs | 89 +++++++++++++++++++++-------------------- tvix/eval/src/tests/mod.rs | 14 ++++--- tvix/eval/src/tests/one_offs.rs | 6 +-- tvix/eval/tests/nix_oracle.rs | 4 +- tvix/glue/benches/eval.rs | 4 +- tvix/glue/src/builtins/mod.rs | 4 +- tvix/glue/src/tvix_store_io.rs | 4 +- tvix/serde/src/de.rs | 4 +- 9 files changed, 70 insertions(+), 67 deletions(-) (limited to 'tvix') diff --git a/tvix/cli/src/main.rs b/tvix/cli/src/main.rs index f1c7a735d1..149b08fa8e 100644 --- a/tvix/cli/src/main.rs +++ b/tvix/cli/src/main.rs @@ -62,7 +62,7 @@ struct Args { /// and the result itself. The return value indicates whether /// evaluation succeeded. fn interpret(code: &str, path: Option, args: &Args, explain: bool) -> bool { - let mut eval = tvix_eval::Evaluation::new_impure(code, path); + let mut eval = tvix_eval::Evaluation::new_impure(); eval.strict = args.strict; @@ -101,7 +101,7 @@ fn interpret(code: &str, path: Option, args: &Args, explain: bool) -> b eval.runtime_observer = Some(&mut runtime_observer); } - eval.evaluate() + eval.evaluate(code, path) }; if args.display_ast { @@ -135,7 +135,7 @@ fn interpret(code: &str, path: Option, args: &Args, explain: bool) -> b /// Interpret the given code snippet, but only run the Tvix compiler /// on it and return errors and warnings. fn lint(code: &str, path: Option, args: &Args) -> bool { - let mut eval = tvix_eval::Evaluation::new_impure(code, path); + let mut eval = tvix_eval::Evaluation::new_impure(); eval.strict = args.strict; let source_map = eval.source_map(); @@ -150,7 +150,7 @@ fn lint(code: &str, path: Option, args: &Args) -> bool { eprintln!("warning: --trace-runtime has no effect with --compile-only!"); } - let result = eval.compile_only(); + let result = eval.compile_only(code, path); if args.display_ast { if let Some(ref expr) = result.expr { diff --git a/tvix/eval/src/lib.rs b/tvix/eval/src/lib.rs index c7c628f70e..37db2b7bf4 100644 --- a/tvix/eval/src/lib.rs +++ b/tvix/eval/src/lib.rs @@ -68,21 +68,10 @@ pub use crate::io::StdIO; /// /// Public fields are intended to be set by the caller. Setting all /// fields is optional. -pub struct Evaluation<'code, 'co, 'ro> { - /// The Nix source code to be evaluated. - code: &'code str, - - /// Optional location of the source code (i.e. path to the file it was read - /// from). Used for error reporting, and for resolving relative paths in - /// impure functions. - location: Option, - +pub struct Evaluation<'co, 'ro> { /// Source code map used for error reporting. source_map: SourceCode, - /// Top-level file reference for this code inside the source map. - file: Arc, - /// Set of all builtins that should be available during the /// evaluation. /// @@ -142,27 +131,15 @@ pub struct EvaluationResult { pub expr: Option, } -impl<'code, 'co, 'ro> Evaluation<'code, 'co, 'ro> { - /// Initialise an `Evaluation` for the given Nix source code snippet, and - /// an optional code location. - pub fn new(code: &'code str, location: Option) -> Self { +impl<'co, 'ro> Default for Evaluation<'co, 'ro> { + fn default() -> Self { let source_map = SourceCode::default(); - let location_str = location - .as_ref() - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_else(|| "[code]".into()); - - let file = source_map.add_file(location_str, code.into()); - let mut builtins = builtins::pure_builtins(); builtins.extend(builtins::placeholders()); // these are temporary - Evaluation { - code, - location, + Self { source_map, - file, builtins, src_builtins: vec![], io_handle: Box::new(DummyIO {}), @@ -173,15 +150,21 @@ impl<'code, 'co, 'ro> Evaluation<'code, 'co, 'ro> { runtime_observer: None, } } +} +impl<'co, 'ro> Evaluation<'co, 'ro> { #[cfg(feature = "impure")] /// Initialise an `Evaluation` for the given snippet, with all /// impure features turned on by default. - pub fn new_impure(code: &'code str, location: Option) -> Self { - let mut eval = Self::new(code, location); - eval.enable_import = true; + pub fn new_impure() -> Self { + let mut eval = Self { + enable_import: true, + io_handle: Box::new(StdIO), + ..Default::default() + }; + eval.builtins.extend(builtins::impure_builtins()); - eval.io_handle = Box::new(StdIO); + eval } @@ -191,21 +174,30 @@ impl<'code, 'co, 'ro> Evaluation<'code, 'co, 'ro> { self.source_map.clone() } - /// Only compile the provided source code. This does not *run* the - /// code, it only provides analysis (errors and warnings) of the - /// compiler. - pub fn compile_only(mut self) -> EvaluationResult { + /// Only compile the provided source code, at an optional location of the + /// source code (i.e. path to the file it was read from; used for error + /// reporting, and for resolving relative paths in impure functions) + /// This does not *run* the code, it only provides analysis (errors and + /// warnings) of the compiler. + pub fn compile_only(mut self, code: &str, location: Option) -> EvaluationResult { let mut result = EvaluationResult::default(); let source = self.source_map(); + let location_str = location + .as_ref() + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_else(|| "[code]".into()); + + let file = source.add_file(location_str, code.into()); + let mut noop_observer = observer::NoOpObserver::default(); let compiler_observer = self.compiler_observer.take().unwrap_or(&mut noop_observer); parse_compile_internal( &mut result, - self.code, - self.file.clone(), - self.location, + code, + file, + location, source, self.builtins, self.src_builtins, @@ -216,11 +208,20 @@ impl<'code, 'co, 'ro> Evaluation<'code, 'co, 'ro> { result } - /// Evaluate the provided source code. - pub fn evaluate(mut self) -> EvaluationResult { + /// Evaluate the provided source code, at an optional location of the source + /// code (i.e. path to the file it was read from; used for error reporting, + /// and for resolving relative paths in impure functions) + pub fn evaluate(mut self, code: &str, location: Option) -> EvaluationResult { let mut result = EvaluationResult::default(); let source = self.source_map(); + let location_str = location + .as_ref() + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_else(|| "[code]".into()); + + let file = source.add_file(location_str, code.into()); + let mut noop_observer = observer::NoOpObserver::default(); let compiler_observer = self.compiler_observer.take().unwrap_or(&mut noop_observer); @@ -231,9 +232,9 @@ impl<'code, 'co, 'ro> Evaluation<'code, 'co, 'ro> { let (lambda, globals) = match parse_compile_internal( &mut result, - self.code, - self.file.clone(), - self.location, + code, + file.clone(), + location, source, self.builtins, self.src_builtins, @@ -255,7 +256,7 @@ impl<'code, 'co, 'ro> Evaluation<'code, 'co, 'ro> { Err(err) => { result.warnings.push(EvalWarning { kind: WarningKind::InvalidNixPath(err.to_string()), - span: self.file.span, + span: file.span, }); None } diff --git a/tvix/eval/src/tests/mod.rs b/tvix/eval/src/tests/mod.rs index b7bea6094d..7ec7028834 100644 --- a/tvix/eval/src/tests/mod.rs +++ b/tvix/eval/src/tests/mod.rs @@ -53,11 +53,11 @@ fn eval_test(code_path: &str, expect_success: bool) { return; } - let mut eval = crate::Evaluation::new_impure(&code, Some(code_path.into())); + let mut eval = crate::Evaluation::new_impure(); eval.strict = true; eval.builtins.extend(mock_builtins::builtins()); - let result = eval.evaluate(); + let result = eval.evaluate(&code, Some(code_path.into())); let failed = match result.value { Some(Value::Catchable(_)) => true, _ => !result.errors.is_empty(), @@ -106,11 +106,13 @@ fn eval_test(code_path: &str, expect_success: bool) { fn identity(code_path: &str) { let code = std::fs::read_to_string(code_path).expect("should be able to read test code"); - let mut eval = crate::Evaluation::new(&code, None); - eval.strict = true; - eval.io_handle = Box::new(crate::StdIO); + let eval = crate::Evaluation { + strict: true, + io_handle: Box::new(crate::StdIO), + ..Default::default() + }; - let result = eval.evaluate(); + let result = eval.evaluate(&code, None); assert!( result.errors.is_empty(), "evaluation of identity test failed: {:?}", diff --git a/tvix/eval/src/tests/one_offs.rs b/tvix/eval/src/tests/one_offs.rs index 23bc9465d6..6024058a12 100644 --- a/tvix/eval/src/tests/one_offs.rs +++ b/tvix/eval/src/tests/one_offs.rs @@ -5,10 +5,10 @@ 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_impure("builtins.testSourceBuiltin", None); + let mut eval = Evaluation::new_impure(); eval.src_builtins.push(("testSourceBuiltin", "42")); - let result = eval.evaluate(); + let result = eval.evaluate("builtins.testSourceBuiltin", None); assert!( result.errors.is_empty(), "evaluation failed: {:?}", @@ -25,7 +25,7 @@ fn test_source_builtin() { #[test] fn skip_broken_bytecode() { - let result = Evaluation::new(/* code = */ "x", None).evaluate(); + let result = Evaluation::default().evaluate(/* code = */ "x", None); assert_eq!(result.errors.len(), 1); diff --git a/tvix/eval/tests/nix_oracle.rs b/tvix/eval/tests/nix_oracle.rs index b777d289e0..670926d9c8 100644 --- a/tvix/eval/tests/nix_oracle.rs +++ b/tvix/eval/tests/nix_oracle.rs @@ -51,12 +51,12 @@ fn nix_eval(expr: &str, strictness: Strictness) -> String { #[track_caller] fn compare_eval(expr: &str, strictness: Strictness) { let nix_result = nix_eval(expr, strictness); - let mut eval = tvix_eval::Evaluation::new(expr, None); + let mut eval = tvix_eval::Evaluation::default(); eval.strict = matches!(strictness, Strictness::Strict); eval.io_handle = Box::new(tvix_eval::StdIO); let tvix_result = eval - .evaluate() + .evaluate(expr, None) .value .expect("tvix evaluation should succeed") .to_string(); diff --git a/tvix/glue/benches/eval.rs b/tvix/glue/benches/eval.rs index 4aa9b3e5c2..a466a0e0fc 100644 --- a/tvix/glue/benches/eval.rs +++ b/tvix/glue/benches/eval.rs @@ -26,7 +26,7 @@ fn interpret(code: &str) { // TODO: this is a bit annoying. // It'd be nice if we could set this up once and then run evaluate() with a // piece of code. b/262 - let mut eval = tvix_eval::Evaluation::new_impure(code, None); + let mut eval = tvix_eval::Evaluation::new_impure(); let known_paths: Rc> = Default::default(); add_derivation_builtins(&mut eval, known_paths.clone()); @@ -47,7 +47,7 @@ fn interpret(code: &str) { ), )); - let result = eval.evaluate(); + let result = eval.evaluate(code, None); assert!(result.errors.is_empty()); } diff --git a/tvix/glue/src/builtins/mod.rs b/tvix/glue/src/builtins/mod.rs index 6b39fd362e..86924cefef 100644 --- a/tvix/glue/src/builtins/mod.rs +++ b/tvix/glue/src/builtins/mod.rs @@ -39,14 +39,14 @@ mod tests { /// Takes care of setting up the evaluator so it knows about the // `derivation` builtin. fn eval(str: &str) -> EvaluationResult { - let mut eval = tvix_eval::Evaluation::new_impure(str, None); + let mut eval = tvix_eval::Evaluation::new_impure(); let known_paths: Rc> = Default::default(); add_derivation_builtins(&mut eval, known_paths.clone()); // run the evaluation itself. - eval.evaluate() + eval.evaluate(str, None) } #[test] diff --git a/tvix/glue/src/tvix_store_io.rs b/tvix/glue/src/tvix_store_io.rs index 70ec21e743..41cae97ebf 100644 --- a/tvix/glue/src/tvix_store_io.rs +++ b/tvix/glue/src/tvix_store_io.rs @@ -375,7 +375,7 @@ mod tests { /// Takes care of setting up the evaluator so it knows about the // `derivation` builtin. fn eval(str: &str) -> EvaluationResult { - let mut eval = tvix_eval::Evaluation::new_impure(str, None); + let mut eval = tvix_eval::Evaluation::new_impure(); let blob_service = Arc::new(MemoryBlobService::default()); let directory_service = Arc::new(MemoryDirectoryService::default()); @@ -397,7 +397,7 @@ mod tests { add_derivation_builtins(&mut eval, known_paths.clone()); // run the evaluation itself. - eval.evaluate() + eval.evaluate(str, None) } /// Helper function that takes a &Path, and invokes a tvix evaluator coercing that path to a string diff --git a/tvix/serde/src/de.rs b/tvix/serde/src/de.rs index 2e8a9618e6..500c9c9840 100644 --- a/tvix/serde/src/de.rs +++ b/tvix/serde/src/de.rs @@ -46,12 +46,12 @@ where F: FnOnce(&mut Evaluation), { // First step is to evaluate the Nix code ... - let mut eval = Evaluation::new(src, None); + let mut eval = Evaluation::default(); config(&mut eval); eval.strict = true; let source = eval.source_map(); - let result = eval.evaluate(); + let result = eval.evaluate(src, None); if !result.errors.is_empty() { return Err(Error::NixErrors { -- cgit 1.4.1