From 4b9178fa2ae4cab718225f6136791df1d11814ee Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Tue, 4 Oct 2022 23:58:21 +0300 Subject: [PATCH] feat(tvix/eval): insert `import` into the builtins itself Adding `import` to builtins causes causes a bootstrap cycle because the `import` builtin needs to be initialised with the set of globals before being inserted into the globals, which also must contain itself. To break out of the cycle this hack wraps the builtins passed to the compiler in an `Rc` (probably sensible anyways, as they will end up getting cloned a bunch), containing a RefCell which gives us mutable access to the builtins. This opens up a potentially dangerous footgun in which we could mutate the builtins at runtime leading to different compiler invocations seeing different builtins, so it'd be nice to have some kind of "finalised" status for them or some such, but I'm not sure how to represent that atm. Change-Id: I25f8d4d2a7e8472d401c8ba2f4bbf9d86ab2abcb Reviewed-on: https://cl.tvl.fyi/c/depot/+/6867 Tested-by: BuildkiteCI Reviewed-by: grfn --- corp/tvixbolt/src/main.rs | 4 +++- tvix/eval/src/builtins/impure.rs | 8 ++++++-- tvix/eval/src/compiler/mod.rs | 14 +++++++++----- tvix/eval/src/eval.rs | 20 ++++++++++++++------ 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/corp/tvixbolt/src/main.rs b/corp/tvixbolt/src/main.rs index a389a33f0..f81ae070a 100644 --- a/corp/tvixbolt/src/main.rs +++ b/corp/tvixbolt/src/main.rs @@ -1,4 +1,6 @@ +use std::cell::RefCell; use std::fmt::Write; +use std::rc::Rc; use serde::{Deserialize, Serialize}; use tvix_eval::observer::TracingObserver; @@ -256,7 +258,7 @@ fn eval(trace: bool, code: &str) -> Output { &root_expr, Some("/nixbolt".into()), file.clone(), - tvix_eval::global_builtins(), + Rc::new(RefCell::new(tvix_eval::global_builtins())), &mut compilation_observer, ) .unwrap(); diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs index 675bdd509..7c98fcb4e 100644 --- a/tvix/eval/src/builtins/impure.rs +++ b/tvix/eval/src/builtins/impure.rs @@ -1,4 +1,5 @@ use std::{ + cell::RefCell, collections::{BTreeMap, HashMap}, rc::Rc, time::{SystemTime, UNIX_EPOCH}, @@ -43,7 +44,10 @@ pub(super) fn builtins() -> BTreeMap { /// it needs to capture the [crate::SourceCode] structure to correctly track /// source code locations while invoking a compiler. // TODO: need to be able to pass through a CompilationObserver, too. -pub fn builtins_import(source: SourceCode) -> Builtin { +pub fn builtins_import( + globals: Rc>>, + source: SourceCode, +) -> Builtin { Builtin::new( "import", &[true], @@ -83,7 +87,7 @@ pub fn builtins_import(source: SourceCode) -> Builtin { &parsed.tree().expr().unwrap(), Some(path.clone()), file, - HashMap::new(), // TODO: pass through globals + globals.clone(), &mut NoOpObserver::default(), ) .map_err(|err| ErrorKind::ImportCompilerError { diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index eb617b227..472f4aaf3 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -21,6 +21,7 @@ use codemap::Span; use path_clean::PathClean; use rnix::ast::{self, AstToken}; use smol_str::SmolStr; +use std::cell::RefCell; use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::rc::Rc; @@ -103,7 +104,7 @@ impl<'observer> Compiler<'observer> { pub(crate) fn new( location: Option, file: Arc, - globals: HashMap<&'static str, Value>, + globals: Rc>>, observer: &'observer mut dyn CompilerObserver, ) -> EvalResult { let mut root_dir = match location { @@ -124,11 +125,13 @@ impl<'observer> Compiler<'observer> { root_dir.pop(); } + let globals = globals.borrow(); + Ok(Self { root_dir, file, observer, - globals: prepare_globals(globals), + globals: prepare_globals(&globals), contexts: vec![LambdaCtx::new()], warnings: vec![], errors: vec![], @@ -1103,7 +1106,7 @@ fn optimise_tail_call(chunk: &mut Chunk) { /// Note that all builtin functions are *not* considered part of the /// language in this sense and MUST be supplied as additional global /// values, including the `builtins` set itself. -fn prepare_globals(additional: HashMap<&'static str, Value>) -> GlobalsMap { +fn prepare_globals(additional: &HashMap<&'static str, Value>) -> GlobalsMap { let mut globals: GlobalsMap = HashMap::new(); globals.insert( @@ -1127,7 +1130,8 @@ fn prepare_globals(additional: HashMap<&'static str, Value>) -> GlobalsMap { }), ); - for (ident, value) in additional.into_iter() { + for (ident, value) in additional.iter() { + let value: Value = value.clone(); globals.insert( ident, Rc::new(move |compiler, span| compiler.emit_constant(value.clone(), &span)), @@ -1141,7 +1145,7 @@ pub fn compile( expr: &ast::Expr, location: Option, file: Arc, - globals: HashMap<&'static str, Value>, + globals: Rc>>, observer: &mut dyn CompilerObserver, ) -> EvalResult { let mut c = Compiler::new(location, file, globals, observer)?; diff --git a/tvix/eval/src/eval.rs b/tvix/eval/src/eval.rs index aed429228..4e50b7279 100644 --- a/tvix/eval/src/eval.rs +++ b/tvix/eval/src/eval.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::{cell::RefCell, path::PathBuf, rc::Rc}; use crate::{ builtins::global_builtins, @@ -59,13 +59,21 @@ pub fn interpret(code: &str, location: Option, options: Options) -> Eva } // TODO: encapsulate this import weirdness in builtins - let mut builtins = global_builtins(); + + let builtins = Rc::new(RefCell::new(global_builtins())); #[cfg(feature = "impure")] - builtins.insert( - "import", - Value::Builtin(crate::builtins::impure::builtins_import(source.clone())), - ); + { + // We need to insert import into the builtins, but the + // builtins passed to import must have import *in it*. + let import = Value::Builtin(crate::builtins::impure::builtins_import( + builtins.clone(), + source.clone(), + )); + + builtins.borrow_mut().insert("import", import); + // TODO: also add it into the inner builtins set + }; let result = if options.dump_bytecode { crate::compiler::compile(