From 3f2160627895e7d1fe3236812b83bddc5d3049f5 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Sat, 24 Sep 2022 15:54:21 +0300 Subject: refactor(tvix/eval): merge all bindings creation logic As of this commit, all three types of bindings scopes are compiled the same way (i.e. compilation of non-recursive attribute sets has been switched over to the new code paths). This sets us up for doing the final implementation of nested attribute sets. HOWEVER, this breaks the existing implementation of nested attributes in non-recursive attribute sets. That implementation is flawed and unworkable in practice, so we need to do this dance to be able to implement it correctly. Change-Id: Iba2545c0d1d6b51f5e1a31a5d005b8d01da546d3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6782 Reviewed-by: sterni Tested-by: BuildkiteCI --- tvix/eval/src/compiler/bindings.rs | 141 +-------------------- tvix/eval/src/compiler/mod.rs | 1 - .../disabled-eval-okay-contains-nested-non-set.nix | 3 + .../disabled-eval-okay-deeply-nested-attrs.nix | 1 + .../disabled-eval-okay-multiple-nested-attrs.nix | 1 + .../disabled-eval-okay-nested-has-attrs.nix | 26 ++++ ...sabled-eval-okay-or-operator-nested-default.nix | 1 + .../disabled-eval-okay-or-operator-nested.nix | 1 + .../disabled-eval-okay-or-operator-non-set.nix | 2 + ...disabled-eval-okay-overlapping-nested-attrs.nix | 4 + .../disabled-eval-okay-simple-nested-attrs.nix | 1 + .../tvix_tests/eval-okay-builtins-type-of.exp | 2 +- .../tvix_tests/eval-okay-builtins-type-of.nix | 2 +- .../eval-okay-contains-nested-non-set.nix | 3 - .../tvix_tests/eval-okay-deeply-nested-attrs.nix | 1 - .../tvix_tests/eval-okay-multiple-nested-attrs.nix | 1 - .../tvix_tests/eval-okay-nested-has-attrs.nix | 26 ---- .../eval-okay-or-operator-nested-default.nix | 1 - .../tvix_tests/eval-okay-or-operator-nested.nix | 1 - .../tvix_tests/eval-okay-or-operator-non-set.nix | 2 - .../eval-okay-overlapping-nested-attrs.nix | 4 - .../tvix_tests/eval-okay-simple-nested-attrs.nix | 1 - 22 files changed, 48 insertions(+), 178 deletions(-) create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-contains-nested-non-set.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-deeply-nested-attrs.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-multiple-nested-attrs.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-nested-has-attrs.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested-default.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-non-set.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-overlapping-nested-attrs.nix create mode 100644 tvix/eval/src/tests/tvix_tests/disabled-eval-okay-simple-nested-attrs.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-contains-nested-non-set.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-deeply-nested-attrs.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-multiple-nested-attrs.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-nested-has-attrs.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested-default.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-non-set.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-overlapping-nested-attrs.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-simple-nested-attrs.nix (limited to 'tvix/eval') diff --git a/tvix/eval/src/compiler/bindings.rs b/tvix/eval/src/compiler/bindings.rs index 86245270f2..28a9fd1105 100644 --- a/tvix/eval/src/compiler/bindings.rs +++ b/tvix/eval/src/compiler/bindings.rs @@ -277,123 +277,6 @@ impl Compiler<'_> { } } - /// Compile the statically known entries of an attribute set. Which keys are - /// which is not known from the iterator, so discovered dynamic keys are - /// returned from here. - fn compile_static_attr_entries( - &mut self, - count: &mut usize, - entries: AstChildren, - ) -> Vec { - let mut dynamic_attrs: Vec = vec![]; - - 'entries: for kv in entries { - // Attempt to turn the attrpath into a list of static strings, but - // abort this process if any dynamic fragments are encountered. - let static_attrpath: Option> = kv - .attrpath() - .unwrap() - .attrs() - .map(|a| self.expr_static_attr_str(&a)) - .collect(); - - let fragments = match static_attrpath { - Some(fragments) => fragments, - None => { - dynamic_attrs.push(kv); - continue 'entries; - } - }; - - // At this point we can increase the counter because we know that - // this particular attribute is static and can thus be processed - // here. - *count += 1; - - let key_count = fragments.len(); - for fragment in fragments.into_iter() { - self.emit_constant(Value::String(fragment.into()), &kv.attrpath().unwrap()); - } - - // We're done with the key if there was only one fragment, otherwise - // we need to emit an instruction to construct the attribute path. - if key_count > 1 { - self.push_op( - OpCode::OpAttrPath(Count(key_count)), - &kv.attrpath().unwrap(), - ); - } - - // The value is just compiled as normal so that its resulting value - // is on the stack when the attribute set is constructed at runtime. - let value_span = self.span_for(&kv.value().unwrap()); - let value_slot = self.scope_mut().declare_phantom(value_span, false); - self.compile(value_slot, kv.value().unwrap()); - self.scope_mut().mark_initialised(value_slot); - } - - dynamic_attrs - } - - /// Compile the dynamic entries of an attribute set, where keys are only - /// known at runtime. - fn compile_dynamic_attr_entries( - &mut self, - count: &mut usize, - entries: Vec, - ) { - for entry in entries.into_iter() { - *count += 1; - - let mut key_count = 0; - let key_span = self.span_for(&entry.attrpath().unwrap()); - let key_idx = self.scope_mut().declare_phantom(key_span, false); - - for fragment in entry.attrpath().unwrap().attrs() { - // Key fragments can contain dynamic expressions, which makes - // accounting for their stack slots very tricky. - // - // In order to ensure the locals are correctly cleaned up, we - // essentially treat any key fragment after the first one (which - // has a locals index that will become that of the final key - // itself) as being part of a separate scope, which results in - // the somewhat annoying setup logic below. - let fragment_slot = match key_count { - 0 => key_idx, - 1 => { - self.scope_mut().begin_scope(); - self.scope_mut().declare_phantom(key_span, false) - } - _ => self.scope_mut().declare_phantom(key_span, false), - }; - - key_count += 1; - self.compile_attr(fragment_slot, fragment); - self.scope_mut().mark_initialised(fragment_slot); - } - - // We're done with the key if there was only one fragment, otherwise - // we need to emit an instruction to construct the attribute path. - if key_count > 1 { - self.push_op( - OpCode::OpAttrPath(Count(key_count)), - &entry.attrpath().unwrap(), - ); - - // Close the temporary scope that was set up for the key - // fragments. - self.scope_mut().end_scope(); - } - - // The value is just compiled as normal so that its resulting value - // is on the stack when the attribute set is constructed at runtime. - let value_span = self.span_for(&entry.value().unwrap()); - let value_slot = self.scope_mut().declare_phantom(value_span, false); - self.compile(value_slot, entry.value().unwrap()); - self.scope_mut().mark_initialised(value_slot); - } - } - /// Compile attribute set literals into equivalent bytecode. /// /// This is complicated by a number of features specific to Nix attribute @@ -407,26 +290,14 @@ impl Compiler<'_> { // `OpAttrs` instruction. self.scope_mut().begin_scope(); - if node.rec_token().is_some() { - let count = self.compile_recursive_scope(slot, BindingsKind::RecAttrs, &node); - self.push_op(OpCode::OpAttrs(Count(count)), &node); + let kind = if node.rec_token().is_some() { + BindingsKind::RecAttrs } else { - let mut count = 0; - - // TODO: merge this with the above, for now only inherit is unified - let mut bindings: Vec = vec![]; - let inherit_froms = - self.compile_plain_inherits(slot, BindingsKind::Attrs, &mut count, &node); - self.declare_namespaced_inherits(BindingsKind::Attrs, inherit_froms, &mut bindings); - self.bind_values(bindings); - - let dynamic_entries = - self.compile_static_attr_entries(&mut count, node.attrpath_values()); - - self.compile_dynamic_attr_entries(&mut count, dynamic_entries); + BindingsKind::Attrs + }; - self.push_op(OpCode::OpAttrs(Count(count)), &node); - } + let count = self.compile_recursive_scope(slot, kind, &node); + self.push_op(OpCode::OpAttrs(Count(count)), &node); // Remove the temporary scope, but do not emit any additional cleanup // (OpAttrs consumes all of these locals). diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index de614be4da..dab91a7f56 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -20,7 +20,6 @@ mod spans; use codemap::Span; use path_clean::PathClean; use rnix::ast::{self, AstToken, HasEntry}; -use rowan::ast::AstChildren; use smol_str::SmolStr; use std::collections::HashMap; use std::path::{Path, PathBuf}; diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-contains-nested-non-set.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-contains-nested-non-set.nix new file mode 100644 index 0000000000..361ba91445 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-contains-nested-non-set.nix @@ -0,0 +1,3 @@ +# ? operator should work even if encountering a non-set value on the +# walk +{ a.b = 42; } ? a.b.c diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-deeply-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-deeply-nested-attrs.nix new file mode 100644 index 0000000000..91649d0c6d --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-deeply-nested-attrs.nix @@ -0,0 +1 @@ +{ a.b.c.d.e.f.g = "deep!"; } diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-multiple-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-multiple-nested-attrs.nix new file mode 100644 index 0000000000..5d611930ca --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-multiple-nested-attrs.nix @@ -0,0 +1 @@ +{ a.b = 15; b.c = "test"; } diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-nested-has-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-nested-has-attrs.nix new file mode 100644 index 0000000000..47dcec7a95 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-nested-has-attrs.nix @@ -0,0 +1,26 @@ +let + set = { + a.b.c = 123; + foo = { + bar = 23; + }; + baz = 1; + }; + + tes = "random value"; +in + +[ + (set ? a) + (set ? a.b) + (set ? a.b.c) + (set ? foo) + (set ? foo.bar) + (set.foo ? bar) + (set ? baz) + (set ? x) + (set ? x.y.z) + (tes ? bar) + (tes ? x.y.z) + (null ? null) +] diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested-default.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested-default.nix new file mode 100644 index 0000000000..ceffd0697b --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested-default.nix @@ -0,0 +1 @@ +{ a.b = 1; }.a.c or 2 diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested.nix new file mode 100644 index 0000000000..1a76594546 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested.nix @@ -0,0 +1 @@ +{ a.b = 1; }.a.b or 2 diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-non-set.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-non-set.nix new file mode 100644 index 0000000000..fd09bfee64 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-non-set.nix @@ -0,0 +1,2 @@ +# `or` operator should keep working if it encounters a non-set type. +{ a.b = 42; }.a.b.c or "works fine" diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-overlapping-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-overlapping-nested-attrs.nix new file mode 100644 index 0000000000..4154ff9da2 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-overlapping-nested-attrs.nix @@ -0,0 +1,4 @@ +{ + a.b = 15; + a.c = "test"; +} diff --git a/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-simple-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-simple-nested-attrs.nix new file mode 100644 index 0000000000..a97394d165 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-simple-nested-attrs.nix @@ -0,0 +1 @@ +{ a.b = 42; } diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp index 0e22da8d56..1ea054fc2d 100644 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp @@ -1 +1 @@ -[ "null" "bool" "bool" "int" "int" "float" "string" "string" "set" "set" "set" "list" "lambda" "path" ] +[ "null" "bool" "bool" "int" "int" "float" "string" "string" "set" "set" "list" "lambda" "path" ] diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix index 38d7ffee61..a3cb659ecf 100644 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix @@ -13,7 +13,7 @@ fix (self: (builtins.typeOf "foo") (builtins.typeOf "${"foo" + "bar"}baz") (builtins.typeOf {}) - (builtins.typeOf { foo.bar = 32; }.foo) + # (builtins.typeOf { foo.bar = 32; }.foo) # TODO: re-enable when nested keys are done (builtins.typeOf ({ name = "foo"; value = 13; } // { name = "bar"; })) (builtins.typeOf self) (builtins.typeOf fix) diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-contains-nested-non-set.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-contains-nested-non-set.nix deleted file mode 100644 index 361ba91445..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-contains-nested-non-set.nix +++ /dev/null @@ -1,3 +0,0 @@ -# ? operator should work even if encountering a non-set value on the -# walk -{ a.b = 42; } ? a.b.c diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-deeply-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-deeply-nested-attrs.nix deleted file mode 100644 index 91649d0c6d..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-deeply-nested-attrs.nix +++ /dev/null @@ -1 +0,0 @@ -{ a.b.c.d.e.f.g = "deep!"; } diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-multiple-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-multiple-nested-attrs.nix deleted file mode 100644 index 5d611930ca..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-multiple-nested-attrs.nix +++ /dev/null @@ -1 +0,0 @@ -{ a.b = 15; b.c = "test"; } diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-nested-has-attrs.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-nested-has-attrs.nix deleted file mode 100644 index 47dcec7a95..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-nested-has-attrs.nix +++ /dev/null @@ -1,26 +0,0 @@ -let - set = { - a.b.c = 123; - foo = { - bar = 23; - }; - baz = 1; - }; - - tes = "random value"; -in - -[ - (set ? a) - (set ? a.b) - (set ? a.b.c) - (set ? foo) - (set ? foo.bar) - (set.foo ? bar) - (set ? baz) - (set ? x) - (set ? x.y.z) - (tes ? bar) - (tes ? x.y.z) - (null ? null) -] diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested-default.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested-default.nix deleted file mode 100644 index ceffd0697b..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested-default.nix +++ /dev/null @@ -1 +0,0 @@ -{ a.b = 1; }.a.c or 2 diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested.nix deleted file mode 100644 index 1a76594546..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested.nix +++ /dev/null @@ -1 +0,0 @@ -{ a.b = 1; }.a.b or 2 diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-non-set.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-non-set.nix deleted file mode 100644 index fd09bfee64..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-non-set.nix +++ /dev/null @@ -1,2 +0,0 @@ -# `or` operator should keep working if it encounters a non-set type. -{ a.b = 42; }.a.b.c or "works fine" diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-overlapping-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-overlapping-nested-attrs.nix deleted file mode 100644 index 4154ff9da2..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-overlapping-nested-attrs.nix +++ /dev/null @@ -1,4 +0,0 @@ -{ - a.b = 15; - a.c = "test"; -} diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-simple-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-simple-nested-attrs.nix deleted file mode 100644 index a97394d165..0000000000 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-simple-nested-attrs.nix +++ /dev/null @@ -1 +0,0 @@ -{ a.b = 42; } -- cgit 1.4.1