From 8616f13a71e3fc186e01bdb4ada60503f233be99 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Wed, 26 Oct 2022 05:16:04 -0700 Subject: feat(tvix/eval): builtins.import without RefCell CL/6867 added support for builtins.import, which required a cyclic reference import->globals->builtins->import. This was implemented using a RefCell, which makes it possible to mutate the builtins during evaluation. The commit message for CL/6867 expressed a desire to eliminate this possibility: 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. This CL replaces the RefCell with Rc::new_cyclic(), making the globals/builtins immutable once again. At VM runtime (once opcodes start executing) everything is the same as before this CL, except that the Rc> introduced by CL/6867 is turned into an rc::Weak<>. The function passed to Rc::new_cyclic works very similarly to overlays in nixpkgs: a function takes its own result as an argument. However instead of laziness "breaking the cycle", Rust's Rc::new_cyclic() instead uses an rc::Weak. This is done to prevent memory leaks rather than divergence. This CL also resolves the following TODO from CL/6867: // TODO: encapsulate this import weirdness in builtins The main disadvantage of this CL is the fact that the VM now must ensure that it holds a strong reference to the globals while a program is executing; failure to do so will cause a panic when the weak reference in the builtins is upgrade()d. In theory it should be possible to create strong reference cycles the same way Rc::new_cyclic() creates weak cycles, but these cycles would cause a permanent memory leak -- without either an rc::Weak or RefCell there is no way to break the cycle. At some point we will have to implement some form of cycle collection; whatever library we choose for that purpose is likely to provide an "immutable strong reference cycle" primitive similar to Rc::new_cyclic(), and we should be able to simply drop it in. Signed-off-by: Adam Joseph Change-Id: I34bb5821628eb97e426bdb880b02e2097402adb7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7097 Tested-by: BuildkiteCI Reviewed-by: tazjin --- tvix/eval/src/builtins/mod.rs | 135 ++++++++++++++++++++++++------------------ 1 file changed, 76 insertions(+), 59 deletions(-) (limited to 'tvix/eval/src/builtins/mod.rs') diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 91fe27aa4695..7dd7ee946d32 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -3,9 +3,12 @@ //! See //tvix/eval/docs/builtins.md for a some context on the //! available builtins in Nix. +use crate::compiler::{GlobalsMap, GlobalsMapFunc}; +use crate::source::SourceCode; use std::cmp::{self, Ordering}; use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::PathBuf; +use std::rc::Rc; use regex::Regex; @@ -690,70 +693,84 @@ fn placeholders() -> Vec { // we set TVIX_CURRENT_SYSTEM in build.rs pub const CURRENT_PLATFORM: &str = env!("TVIX_CURRENT_SYSTEM"); -fn builtins_set() -> NixAttrs { - let mut map: BTreeMap = BTreeMap::new(); - - // Pure-value builtins - map.insert( - "nixVersion".into(), - Value::String("2.3-compat-tvix-0.1".into()), - ); - - map.insert("langVersion".into(), Value::Integer(6)); +/// Set of Nix builtins that are globally available. +pub fn global_builtins(source: SourceCode) -> GlobalsMapFunc { + Box::new(move |globals: &std::rc::Weak| { + let mut map: BTreeMap<&'static str, Value> = BTreeMap::new(); - map.insert( - "currentSystem".into(), - crate::systems::llvm_triple_to_nix_double(CURRENT_PLATFORM).into(), - ); + // Pure-value builtins + map.insert("nixVersion", Value::String("2.3-compat-tvix-0.1".into())); - let mut add_builtins = |builtins: Vec| { - for builtin in builtins { - map.insert(builtin.name().into(), Value::Builtin(builtin)); - } - }; + map.insert("langVersion", Value::Integer(6)); - add_builtins(pure_builtins()); - add_builtins(placeholders()); + map.insert( + "currentSystem", + crate::systems::llvm_triple_to_nix_double(CURRENT_PLATFORM).into(), + ); - #[cfg(feature = "impure")] - { - map.extend(impure::builtins()); - } - - NixAttrs::from_map(map) -} - -/// Set of Nix builtins that are globally available. -pub fn global_builtins() -> HashMap<&'static str, Value> { - let builtins = builtins_set(); - let mut globals: HashMap<&'static str, Value> = HashMap::new(); - - // known global builtins from the builtins set. - for global in &[ - "abort", - "baseNameOf", - "derivation", - "derivationStrict", - "dirOf", - "fetchGit", - "fetchMercurial", - "fetchTarball", - "fromTOML", - "import", - "isNull", - "map", - "placeholder", - "removeAttrs", - "scopedImport", - "throw", - "toString", - ] { - if let Some(builtin) = builtins.select(global) { - globals.insert(global, builtin.clone()); + let mut add_builtins = |builtins: Vec| { + for builtin in builtins { + map.insert(builtin.name(), Value::Builtin(builtin)); + } + }; + + add_builtins(pure_builtins()); + add_builtins(placeholders()); + + #[cfg(feature = "impure")] + { + map.extend(impure::builtins()); + + // 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( + globals, + source.clone(), + )); + + map.insert("import", import); + }; + + let mut globals: GlobalsMap = HashMap::new(); + + let builtins = Rc::new(NixAttrs::from_map( + map.into_iter().map(|(k, v)| (k.into(), v)).collect(), + )); + + // known global builtins from the builtins set. + for global in &[ + "abort", + "baseNameOf", + "derivation", + "derivationStrict", + "dirOf", + "fetchGit", + "fetchMercurial", + "fetchTarball", + "fromTOML", + "import", + "isNull", + "map", + "placeholder", + "removeAttrs", + "scopedImport", + "throw", + "toString", + ] { + if let Some(builtin) = builtins.select(global) { + let builtin = builtin.clone(); + globals.insert( + global, + Rc::new(move |c, s| c.emit_constant(builtin.clone(), &s)), + ); + } } - } - globals.insert("builtins", Value::attrs(builtins)); + globals.insert( + "builtins", + Rc::new(move |c, s| c.emit_constant(Value::attrs(builtins.as_ref().clone()), &s)), + ); - globals + globals + }) } -- cgit 1.4.1